[PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals

Fri Jan 25 14:56:29 UTC 2019

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.

/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);
>>>>>>>>