[PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals
David Holmes
david.holmes at oracle.com
Mon Mar 11 07:59:51 UTC 2019
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.
All passed.
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