[PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals
David Holmes
david.holmes at oracle.com
Fri Mar 1 04:18:42 UTC 2019
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.
There's no need for a separate os::posix::init_sigs() API, just do the
signal set initialization in os::posix::init_2();
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> 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> 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> 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> 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