[PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals
David Holmes
david.holmes at oracle.com
Mon Mar 11 06:10:25 UTC 2019
Hi,
Sorry was on a few days vacation.
I will run this through our test system and report back.
David
On 5/03/2019 8:59 pm, Ao Qi wrote:
> On Tue, Mar 5, 2019 at 6:35 PM David Holmes <david.holmes at oracle.com> wrote:
>>
>> On 5/03/2019 8:25 pm, Ao Qi wrote:
>>> Hi David,
>>>
>>> Webrev: http://cr.openjdk.java.net/~aoqi/8170639/webrev.03
>>>
>>> What about this version? Am I understanding correctly? I think this
>>> version is indeed much better.
>>
>> Yes that's what I meant.
>>
>>> I also fixed a problem in my previous
>>> version:( The code was only put into #ifdef SUPPORTS_CLOCK_MONOTONIC
>>> block.
>>
>> Oops missed that. Surprised the declarations in the .hpp file don't also
>> need to be in #ifndef SOLARIS.
>>
>>> Done tests:
>>> build: linux-x86_64-{server, minimal, zero}-{fastdebug, release},
>>> solaris-x86_64-server-release
>>> jtreg: linux-x86_64-server-release: hotspot:tier1
>>>
>>> I am doing other tests. Seems good so far.
>>
>> Please run through the submit repo if you can.
>>
>
> I am afraid I cannot, because I am not a Committer. Maybe I need some
> Committer to help me to run the tests. Could someone help?
>
> Thanks,
> Ao Qi
>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Ao Qi
>>>
>>> On Tue, Mar 5, 2019 at 10:45 AM David Holmes <david.holmes at oracle.com> wrote:
>>>>
>>>> On 5/03/2019 12:22 pm, Ao Qi wrote:
>>>>>
>>>>>
>>>>> David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>>
>>>>> 于2019年3月1日 周五下午12:18写道：
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 28/02/2019 9:01 pm, Ao Qi wrote:
>>>>> > Hi,
>>>>> >
>>>>> > This version is more consolidated than my first version in this
>>>>> > thread: http://cr.openjdk.java.net/~aoqi/8170639/webrev.01/. I filed
>>>>> > my first version here:
>>>>> > http://cr.openjdk.java.net/~aoqi/8170639/webrev.00/
>>>>> >
>>>>> > I fixed the linux and bsd using sigset_t, so jsig is not limited to a
>>>>> > maximum of 64 signals for linux and bsd. The logic of aix and solaris
>>>>> > is not changed.
>>>>>
>>>>> Thanks for doing this. Overall this is a much better approach.
>>>>>
>>>>> I have two requests.
>>>>>
>>>>> There are too many ifdefs for solaris in the os_posix.cpp code. I
>>>>> hadn't
>>>>> realized the difference between Solaris and the other *NIX platforms in
>>>>> this area (allowing for real-time signal chaining). Please restore the
>>>>> Solaris code and put one ifndef solaris around the new code is
>>>>> os_posix.[ch]pp.
>>>>>
>>>>>
>>>>> Hi David,
>>>>>
>>>>> Thanks for your review and advice.
>>>>>
>>>>> I think the best way is to eliminate ifdefs for solaris, but as you said
>>>>> in a previous email [1]: "SIGRTMAX is a macro not a constant so you
>>>>> can't declare a static array using it. You would need to malloc the
>>>>> array as done on Solaris." So I have to use ifdefs for solaris, I did
>>>>> not find a way to eliminate it. I have used one ifndef solaris as you
>>>>> request.
>>>>
>>>> What I was suggesting was to leave os_solaris.cpp alone completely for
>>>> this change, and have the new os_posix.* functions defined in an
>>>> "#ifndef SOLARIS" block.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> PS. I will be away for a few days after today.
>>>>
>>>>> There's no need for a separate os::posix::init_sigs() API, just do the
>>>>> signal set initialization in os::posix::init_2();
>>>>>
>>>>>
>>>>> I have done it.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~aoqi/8170639/webrev.02/
>>>>>
>>>>> Tested:
>>>>> build tests
>>>>> linux-x86_64-minimal-fastdebug
>>>>> linux-x86_64-zero-fastdebug
>>>>> linux-x86_64-server-fastdebug
>>>>> linux-x86_64-minimal-release
>>>>> linux-x86_64-zero-release
>>>>> linux-x86_64-server-release
>>>>> solaris-x86_64-server-release
>>>>> solaris-x86_64-server-fastdebug
>>>>> linux-mips64el-zero-release
>>>>> linux-mips64el-zero-fastdebug
>>>>>
>>>>> jtreg tests
>>>>> linux-x86_64-server-release: tier1,
>>>>> hotspot-tier1(JAVA_OPTIONS=-XX:-UseSignalChaining),
>>>>> hotspot-tier1(JAVA_OPTIONS=-XX:+AllowUserSignalHandlers)
>>>>> solaris-x86_64-server-release: hotspot-tier1,
>>>>> hotspot-tier1(JAVA_OPTIONS=-XX:-UseSignalChaining)
>>>>>
>>>>> Thanks,
>>>>> Ao Qi
>>>>>
>>>>> [1]
>>>>> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-March/027219.html
>>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>> > The tests I did:
>>>>> >
>>>>> > build tests：
>>>>> > linux-x86_64-server-release
>>>>> > linux-x86_64-server-fastdebug
>>>>> > linux-x86_64-zero-release
>>>>> > linux-x86_64-zero-fastdebug
>>>>> > linux-x86_64-minimal-release
>>>>> > linux-x86_64-minimal-fastdebug
>>>>> > solaris-x86_64-server-release
>>>>> > solaris-x86_64-server-fastdebug
>>>>> > linux-mips64el-zero-release
>>>>> > linux-mips64el-zero-fastdebug
>>>>> >
>>>>> > jtreg tests：
>>>>> > linux-x86_64-server-release: tier1, tier2, tier3
>>>>> > linux-x86_64-server-fastdebug: tier1
>>>>> > solaris-x86_64-server-release: hotspot-tier1
>>>>> >
>>>>> > No regression was found. Could you please review this version?
>>>>> >
>>>>> > On Fri, Jan 25, 2019 at 10:56 PM Magnus Ihse Bursie
>>>>> > <magnus.ihse.bursie at oracle.com
>>>>> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
>>>>> >>
>>>>> >> On 2018-12-18 01:33, David Holmes wrote:
>>>>> >>> On 18/12/2018 9:57 am, Ao Qi wrote:
>>>>> >>>> Hi,
>>>>> >>>>
>>>>> >>>> On Tue, Dec 18, 2018 at 4:32 AM David Holmes
>>>>> >>>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>>>> >>>>>
>>>>> >>>>> On 17/12/2018 9:48 pm, Ao Qi wrote:
>>>>> >>>>>> Hi David Holmes,
>>>>> >>>>>>
>>>>> >>>>>> Thanks for your reply. It makes sense. Is it possible to create
>>>>> >>>>>> another bug ID to fix the problem?
>>>>> >>>>>
>>>>> >>>>> I'd prefer to see this fixed properly than point fixed on
>>>>> linux for an
>>>>> >>>>> unsupported platform.
>>>>> >>>>
>>>>> >>>> I thought the problem is "[Linux] jsig is limited to a maximum
>>>>> of 64
>>>>> >>>> signals" and the patch was intended to fixed this problem. It just
>>>>> >>>> didn't use "A better solution", but I think it is better than the
>>>>> >>>> current situation.
>>>>> >>>
>>>>> >>> Better for you but I would prefer to see an overall better
>>>>> solution.
>>>>> >>> It really shouldn't be very difficult. Compare the AIX version with
>>>>> >>> your improved Linux version, reconcile any differences, and
>>>>> make it a
>>>>> >>> shared posix version, then confirm it works on OS X and Solaris.
>>>>> >>>
>>>>> >>>> Is zero/mips (or all platforms having more than 64 signals) a
>>>>> >>>> unsupported platform?
>>>>> >>>
>>>>> >>> Yes. We make allowances for Zero as it can support a vast range of
>>>>> >>> platforms for which no native OpenJDK port exists (in the case
>>>>> of MIPS
>>>>> >>> that port seems inactive), but in general we don't go out of
>>>>> our way
>>>>> >>> to make changes in the main code that only benefit such platforms.
>>>>> >>>
>>>>> >>> I'm subtly trying to coerce you into making the change I'd like
>>>>> to see
>>>>> >>> here. ;-) I can assist with build/test on other POSIX platforms.
>>>>> >> If you are going down that route, I recommend looking at
>>>>> >> src/java.base/unix/native/libjsig/jsig.c for inspiration. It was
>>>>> >> recently unified across all unix platforms we currently support,
>>>>> so any
>>>>> >> special treatment there for a specific platform (I think
>>>>> solaris, macosx
>>>>> >> and aix have some special stuff going on), is likely to apply to the
>>>>> >> signal handling inside the VM as well. On the other hand, general
>>>>> >> solutions that worked in jsig.c is likely to work as well in the VM.
>>>>> >>
>>>>> >
>>>>> > Hi Magnus,
>>>>> >
>>>>> > Thanks for you information and advise. That is very helpful.
>>>>> > Difference between hotspot/os/${os_name}/os_${os_name}.cpp is much
>>>>> > more than the difference between ${os_name}/native/libjsig/jsig.c, so
>>>>> > it is difficult to unify all the jsig related codes into os_posix.hpp
>>>>> > and os_posix.cpp. Maybe I would try to do it in another issue.
>>>>> >
>>>>> > Cheers,
>>>>> > Ao Qi
>>>>> >
>>>>> >> /Magnus
>>>>> >>>
>>>>> >>> Cheers,
>>>>> >>> David
>>>>> >>>
>>>>> >>>> Cheers,
>>>>> >>>> Ao Qi
>>>>> >>>>>
>>>>> >>>>> Thanks,
>>>>> >>>>> David
>>>>> >>>>>
>>>>> >>>>>> Cheers,
>>>>> >>>>>> Ao Qi
>>>>> >>>>>>
>>>>> >>>>>> On Mon, Dec 17, 2018 at 7:30 PM David Holmes
>>>>> >>>>>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>>
>>>>> wrote:
>>>>> >>>>>>>
>>>>> >>>>>>> Thanks for reminding me about the rest of the discussion. The
>>>>> >>>>>>> point of
>>>>> >>>>>>> the RFE I filed was to examine if we could get a
>>>>> consolidated POSIX
>>>>> >>>>>>> version using sigset_t. I'd rather see that than a point
>>>>> fix for
>>>>> >>>>>>> Linux.
>>>>> >>>>>>>
>>>>> >>>>>>> Thanks,
>>>>> >>>>>>> David
>>>>> >>>>>>>
>>>>> >>>>>>> On 17/12/2018 6:50 pm, Ao Qi wrote:
>>>>> >>>>>>>> Hi David Holmes,
>>>>> >>>>>>>>
>>>>> >>>>>>>> I am not sure what is the specific issue of "NSIG not being
>>>>> >>>>>>>> defined".
>>>>> >>>>>>>> Do you mean the issue mentioned in "but I found that NSIG was
>>>>> >>>>>>>> missing
>>>>> >>>>>>>> from signal.h on some architectures, mips being among
>>>>> them"[1]? If
>>>>> >>>>>>>> yes, James Cowgill answered this in [2]. I check it on
>>>>> MIPS, NSIG is
>>>>> >>>>>>>> defined in signal.h and it is 128.
>>>>> >>>>>>>>
>>>>> >>>>>>>> Cheers,
>>>>> >>>>>>>> Ao Qi
>>>>> >>>>>>>>
>>>>> >>>>>>>> [1]
>>>>> >>>>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-November/025401.html
>>>>> >>>>>>>> [2]
>>>>> >>>>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-December/025416.html
>>>>> >>>>>>>>
>>>>> >>>>>>>> On Mon, Dec 17, 2018 at 8:02 AM David Holmes
>>>>> >>>>>>>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>>
>>>>> wrote:
>>>>> >>>>>>>>>
>>>>> >>>>>>>>> Hi,
>>>>> >>>>>>>>>
>>>>> >>>>>>>>> Was the issue of NSIG not being defined resolved?
>>>>> >>>>>>>>>
>>>>> >>>>>>>>> Thanks,
>>>>> >>>>>>>>> David
>>>>> >>>>>>>>>
>>>>> >>>>>>>>> On 14/12/2018 10:23 pm, Ao Qi wrote:
>>>>> >>>>>>>>>> Hi all,
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> Zero does not build on Linux MIPS (actually all Linux of
>>>>> NSIG >
>>>>> >>>>>>>>>> 64)
>>>>> >>>>>>>>>> since OpenJDK 9, because NISG is 128 on MIPS and can not be
>>>>> >>>>>>>>>> encoded in
>>>>> >>>>>>>>>> uint64_t sigs. James Cowgill from Debian tried to push a
>>>>> patch
>>>>> >>>>>>>>>> fixing
>>>>> >>>>>>>>>> this[1] but it seems he failed. I would like to try again to
>>>>> >>>>>>>>>> request
>>>>> >>>>>>>>>> fixing this in http://hg.openjdk.java.net/jdk/jdk. This was
>>>>> >>>>>>>>>> fixed in
>>>>> >>>>>>>>>> AIX implementation.
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> After applying this patch, zero can be built on MIPS (Debian
>>>>> >>>>>>>>>> testing).
>>>>> >>>>>>>>>> I've run the jtreg tests [2] on x86 Linux.
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> [1]
>>>>> >>>>>>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-November/025399.html
>>>>> >>>>>>>>>> [2]
>>>>> >>>>>>>>>>
>>>>> https://download.java.net/openjdk/testresults/12/docs/howtoruntests.html
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> Cheers,
>>>>> >>>>>>>>>> Ao Qi
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> $ hg export -r 52869
>>>>> >>>>>>>>>> # HG changeset patch
>>>>> >>>>>>>>>> # User aoqi
>>>>> >>>>>>>>>> # Date 1544089853 0
>>>>> >>>>>>>>>> # Thu Dec 06 09:50:53 2018 +0000
>>>>> >>>>>>>>>> # Node ID 3eea22b79dc3a4bf26616bde1acb587e3f56e6fe
>>>>> >>>>>>>>>> # Parent b4982a22926b4ddf1a7b1f770e4d42ce8c1dd575
>>>>> >>>>>>>>>> 8170639: [Linux] jsig is limited to a maximum of 64 signals
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> diff -r b4982a22926b -r 3eea22b79dc3
>>>>> >>>>>>>>>> src/hotspot/os/linux/os_linux.cpp
>>>>> >>>>>>>>>> --- a/src/hotspot/os/linux/os_linux.cpp Thu Dec 06 11:54:39
>>>>> >>>>>>>>>> 2018 +0530
>>>>> >>>>>>>>>> +++ b/src/hotspot/os/linux/os_linux.cpp Thu Dec 06 09:50:53
>>>>> >>>>>>>>>> 2018 +0000
>>>>> >>>>>>>>>> @@ -4434,10 +4434,7 @@
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> // For signal-chaining
>>>>> >>>>>>>>>> struct sigaction sigact[NSIG];
>>>>> >>>>>>>>>> -uint64_t sigs = 0;
>>>>> >>>>>>>>>> -#if (64 < NSIG-1)
>>>>> >>>>>>>>>> -#error "Not all signals can be encoded in sigs. Adapt
>>>>> its type!"
>>>>> >>>>>>>>>> -#endif
>>>>> >>>>>>>>>> +sigset_t sigs;
>>>>> >>>>>>>>>> bool os::Linux::libjsig_is_loaded = false;
>>>>> >>>>>>>>>> typedef struct sigaction *(*get_signal_t)(int);
>>>>> >>>>>>>>>> get_signal_t os::Linux::get_signal_action = NULL;
>>>>> >>>>>>>>>> @@ -4516,7 +4513,7 @@
>>>>> >>>>>>>>>> }
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> struct sigaction*
>>>>> os::Linux::get_preinstalled_handler(int
>>>>> >>>>>>>>>> sig) {
>>>>> >>>>>>>>>> - if ((((uint64_t)1 << (sig-1)) & sigs) != 0) {
>>>>> >>>>>>>>>> + if (sigismember(&sigs, sig)) {
>>>>> >>>>>>>>>> return &sigact[sig];
>>>>> >>>>>>>>>> }
>>>>> >>>>>>>>>> return NULL;
>>>>> >>>>>>>>>> @@ -4525,7 +4522,7 @@
>>>>> >>>>>>>>>> void os::Linux::save_preinstalled_handler(int sig,
>>>>> struct
>>>>> >>>>>>>>>> sigaction& oldAct) {
>>>>> >>>>>>>>>> assert(sig > 0 && sig < NSIG, "vm signal out of
>>>>> expected
>>>>> >>>>>>>>>> range");
>>>>> >>>>>>>>>> sigact[sig] = oldAct;
>>>>> >>>>>>>>>> - sigs |= (uint64_t)1 << (sig-1);
>>>>> >>>>>>>>>> + sigaddset(&sigs, sig);
>>>>> >>>>>>>>>> }
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> // for diagnostic
>>>>> >>>>>>>>>> @@ -4616,6 +4613,7 @@
>>>>> >>>>>>>>>> (*begin_signal_setting)();
>>>>> >>>>>>>>>> }
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> + ::sigemptyset(&sigs);
>>>>> >>>>>>>>>> set_signal_handler(SIGSEGV, true);
>>>>> >>>>>>>>>> set_signal_handler(SIGPIPE, true);
>>>>> >>>>>>>>>> set_signal_handler(SIGBUS, true);
>>>>> >>>>>>>>>>
>>>>> >>
>>>>>
More information about the hotspot-runtime-dev
mailing list