[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: [PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals
From: Ao Qi <aoqi () loongson ! cn>
Date: 2019-02-28 11:01:36
Message-ID: CALjzQn4FOMce6uXn3NpnNgh+G_nSXtWh7yr5SSMnKmd0GOg_3g () mail ! gmail ! com
[Download RAW message or body]
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.
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@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@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@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@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);
> >>>>>>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic