[PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals
David Holmes
david.holmes at oracle.com
Mon Mar 11 11:57:19 UTC 2019
On 11/03/2019 6:09 pm, Ao Qi wrote:
> Hi David,
>
> On Mon, Mar 11, 2019 at 3:59 PM David Holmes <david.holmes at oracle.com> wrote:
>>
>> On 11/03/2019 4:10 pm, David Holmes wrote:
>>> Hi,
>>>
>>> Sorry was on a few days vacation.
>>>
>>> I will run this through our test system and report back.
>
> Thank you very much!
>
>>
>> All passed.
>
> Happy to hear that. David, are you okay with this patch? Do I need
> another review?
Yes a second review is needed.
Thanks,
David
>
> Ao Qi
>
>>
>> David
>> -----
>>
>>> 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