RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM (original) (raw)

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Tue Nov 17 15:30:56 UTC 2015

Great, looks good. Thanks a lot!

Best regards, Goetz.

-----Original Message----- From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com] Sent: Dienstag, 17. November 2015 14:42 To: Lindenmaier, Goetz; Thomas Stüfe Cc: David Holmes; hotspot-runtime-dev at openjdk.java.net; serviceability- dev Subject: Re: RFR(M): 8141529: Fix handling of JAVASRSIGNUM

Goetz, Push done. Please, take a look at the changeset: http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/149cc1f9f1aa -Dmitry On 2015-11-17 11:35, Lindenmaier, Goetz wrote: > That's great, thanks a lot! > > Best regards, > Goetz. > >> -----Original Message----- >> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com] >> Sent: Dienstag, 17. November 2015 09:34 >> To: Lindenmaier, Goetz; Thomas Stüfe >> Cc: David Holmes; hotspot-runtime-dev at openjdk.java.net; serviceability- >> dev >> Subject: Re: RFR(M): 8141529: Fix handling of JAVASRSIGNUM >> >> Goetz, >> >> I'll sponsor the push >> >> -Dmitry. >> >> On 2015-11-17 10:52, Lindenmaier, Goetz wrote: >>> David, Dmitry, >>> >>> I think this is reviewed now. Could one of you please sponsor this? >>> The final webrev, including a full changeset patch, is this: >>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-NSIG/webrev.02/ >>> >>> It ran successfully through our nightly tests, tonight. >>> >>> Best regards, >>> Goetz. >>> >>>> -----Original Message----- >>>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com] >>>> Sent: Montag, 16. November 2015 11:53 >>>> To: Lindenmaier, Goetz; Thomas Stüfe >>>> Cc: David Holmes; hotspot-runtime-dev at openjdk.java.net; serviceability- >>>> dev >>>> Subject: Re: RFR(M): 8141529: Fix handling of JAVASRSIGNUM >>>> >>>> Goetz, >>>> >>>> Looks good for me. >>>> >>>> Thank you for a nice cleanup. >>>> >>>> -Dmitry >>>> >>>> >>>> On 2015-11-16 13:25, Lindenmaier, Goetz wrote: >>>>> Hi Thomas, >>>>> >>>>> >>>>> >>>>> thanks for looking at this. >>>>> >>>>> >>>>> >>>>>> MAX2(SIGSEGV, SIGBUS) >>>>> >>>>> I really would like to leave the code as-is. This would be a functional >>>>> >>>>> change, while I only intend to fix issues in this change. Also, as David >>>>> >>>>> explained, it might break for some os implementations. >>>>> >>>>> >>>>> >>>>>> the only way to initialize it is with one of sigemptyset() or sigfillset(). >>>>> >>>>> I added initialization with sigemptyset(). Unfortunately, there is no >>>>> static >>>>> >>>>> initializer for this. >>>>> >>>>> >>>>> >>>>>> I would like to see those removed from os::Aix and put into osaix.cpp >> at >>>> static filescope >>>>> >>>>> I moved these to static scope on the three oses. >>>>> >>>>> >>>>> >>>>> Here is the new webrev: >>>>> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529- NSIG/webrev.02/ >>>>> >>>>> >>>>> >>>>> Best regards, >>>>> >>>>> Goetz. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> *From:*Thomas Stüfe [mailto:thomas.stuefe at gmail.com] >>>>> Sent: Freitag, 13. November 2015 10:54 >>>>> To: Lindenmaier, Goetz >>>>> Cc: Dmitry Samersoff; David Holmes; >>>>> hotspot-runtime-dev at openjdk.java.net; serviceability-dev >>>>> Subject: Re: RFR(M): 8141529: Fix handling of JAVASRSIGNUM >>>>> >>>>> >>>>> >>>>> Hi Goetz, >>>>> >>>>> >>>>> >>>>> sorry for not looking at this earlier. This is a nice cleanup. Some remarks: >>>>> >>>>> >>>>> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529- >>>> NSIG/webrev.01/src/os/aix/vm/osaix.cpp.udiff.html >>>>> >>>>> >>>>> >>>>> + if (sig > MAX2(SIGSEGV, SIGBUS) && // See 4355769. >>>>> >>>>> + sig < NSIG) { // Must be legal signal and fit_ _>>>>> into sigflags[]. >>>>> >>>>> >>>>> >>>>> I do not like much the MAX2() construct. I would like it better to >>>>> explicitly check whether the SR signal is one of the "forbidden" ones >>>>> the VM uses. >>>>> >>>>> >>>>> >>>>> Maybe keep a mask defined centrally for each platform which contains >>>>> signals the VM needs for itself ? >>>>> >>>>> >>>>> >>>>> +sigsett os::Aix::sigs = { 0 }; >>>>> >>>>> >>>>> >>>>> I would not initialize the signal set this way. sigsett is an opaque >>>>> type; the only way to initialize it is with one of sigemptyset() or >>>>> sigfillset(). >>>>> >>>>> >>>>> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529- >>>> NSIG/webrev.01/src/os/aix/vm/osaix.hpp.udiff.html >>>>> >>>>> >>>>> >>>>> + static struct sigaction sigact[NSIG]; // saved preinstalled sigactions >>>>> >>>>> + static sigsett sigs; // mask of signals that have >>>>> >>>>> >>>>> >>>>> + static int sigflags[NSIG]; >>>>> >>>>> >>>>> >>>>> I know this is not in the scope of your change, but I would like to see >>>>> those removed from os::Aix and put into osaix.cpp at static filescope. >>>>> There is no need at all to export those, and you would get rid of the >>>>> signal.h dependency you know have when including osaix.hpp. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529- >>>> NSIG/webrev.01/src/os/bsd/vm/jsig.c.udiff.html >>>>> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529- >>>> NSIG/webrev.01/src/os/bsd/vm/osbsd.cpp.udiff.html >>>>> >>>>> >>>>> >>>>> On BSD, we have realtime signals. >>>>> >>>>> >>>>> >>>>> http://fxr.watson.org/fxr/source/sys/signal.h >>>>> >>>>> #define SIGRTMAX 126 >>>>> >>>>> and NSIG does not contain them: >>>>> >>>>> #define NSIG 32 >>>>> >>>>> >>>>> >>>>> The max. possible signal number would be 126, which unfortunately >> does >>>>> not even fit into a 64bit mask. >>>>> >>>>> >>>>> >>>>> So the code in jsig.c is broken for the case that someone wants to >>>>> register realtime signals, if the VM were to ever use realtime signals >>>>> itself, which now is not the case. >>>>> >>>>> >>>>> >>>>> The same is true for osbsd.cpp, where signal chaining will not work if >>>>> the application did have handler for real time signals pre-installed >>>>> before jvm is loaded. >>>>> >>>>> >>>>> >>>>> Solaris: >>>>> >>>>> >>>>> >>>>> The only platform where NSIG is missing? >>>>> >>>>> >>>>> >>>>> Here, we calculate the max. signal number dynamically in ossolaris.cpp, >>>>> presumably because SIGRTMAX is not a constant and can be changed >> using >>>>> system configuration. But then, on Linux we have the same situation >>>>> (SIGRTMAX is dynamic) and there we do not go through the trouble of >>>>> calculating the max. signal number dynamically. Instead we just use >>>>> NSIG=64 and rely on the fact that NSIG is larger than the largest >>>>> possible dynamic value for SIGRTMAX. >>>>> >>>>> >>>>> >>>>> Solaris does not seem to have NSIG defined, but I am sure there is also >>>>> a max. possible value for SIGRTMAX (the default seems to be 48). So, >> one >>>>> could probably safely define NSIG for Solaris too, so that we have NSIG >>>>> defined on all Posix platforms. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On Thu, Nov 12, 2015 at 8:24 PM, Lindenmaier, Goetz >>>>> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com>> >>>> wrote: >>>>> >>>>> Hi David, Dmitry, >>>>> >>>>> I've come up with a new webrev: >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529- >> NSIG/webrev.01/ >>>>> >>>>> I hit on some more issues: >>>>> - As proposed, I replaced MAXSIGNUM by NSIG >>>>> - On AIX, NSIG=255. Therefore storing bits in a word does not work. >>>>> I'm now using bitset functionality from signal.h as it's done in >>>>> other places. >>>>> sigsett is >> NSIG on linux, so it's no good idea to use it there. >>>>> >>>>> >>>>> >>>>> Why do we not do this on all platforms, provided sigsett contains all >>>>> signals (incl. realtime signals) ? >>>>> >>>>> >>>>> >>>>> - In the os files I found another bit vector that now is too small: >>>>> sigs. >>>>> I adapted that, too. Removed the dead declaration of this on >>>>> solaris. >>>>> >>>>> Best regards, >>>>> Goetz. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> Kind Regards, Thomas >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> > -----Original Message----- >>>>> > From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com >>>> <mailto:dmitry.samersoff at oracle.com>] >>>>> >>>>> > Sent: Donnerstag, 12. November 2015 10:05 >>>>> > To: Lindenmaier, Goetz; David Holmes; hotspot-runtime- >>>>> > dev at openjdk.java.net <mailto:dev at openjdk.java.net>; >> serviceability- >>>> dev >>>>> > Subject: Re: RFR(M): 8141529: Fix handling of JAVASRSIGNUM >>>>> > >>>>> > Goetz, >>>>> > >>>>> > *BSD including OS X also defines NSIG (just checked) and if my >>>>> memory is >>>>> > not bogus, AIX defines it too. >>>>> > >>>>> > So you may consider to use NSIG on all platform. >>>>> > >>>>> > -Dmitry >>>>> > >>>>> > On 2015-11-12 11:36, Lindenmaier, Goetz wrote: >>>>> > > OK I'll change it to NSIG. That's used in other places in >>>>> oslinux, too. >>>>> > > So it's really more consistent. >>>>> > > >>>>> > > Best regards, >>>>> > > Goetz >>>>> > > >>>>> > >> -----Original Message----- >>>>> > >> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com >>>>> <mailto:dmitry.samersoff at oracle.com>] >>>>> > >> Sent: Donnerstag, 12. November 2015 09:22 >>>>> > >> To: David Holmes; Lindenmaier, Goetz; hotspot-runtime- >>>>> > >> dev at openjdk.java.net <mailto:dev at openjdk.java.net>; >>>>> serviceability-dev >>>>> > >> Subject: Re: RFR(M): 8141529: Fix handling of JAVASRSIGNUM >>>>> > >> >>>>> > >> David, >>>>> > >> >>>>> > >> I think it's better to use NSIG (without underscore) defined in >>>>> signal.h >>>>> > >> >>>>> > >> -Dmitry >>>>> > >> >>>>> > >> >>>>> > >> On 2015-11-12 10:35, David Holmes wrote: >>>>> > >>> Hi Goetz, >>>>> > >>> >>>>> > >>> Adding in serviceability-dev >>>>> > >>> >>>>> > >>> On 9/11/2015 6:22 PM, Lindenmaier, Goetz wrote: >>>>> > >>>> Hi, >>>>> > >>>> >>>>> > >>>> The environment variable JAVASRSIGNUM can be set to a >>>> signal >>>>> > >> number >>>>> > >>>> do be used by the JVM's suspend/resume mechanism. >>>>> > >>>> >>>>> > >>>> If set, a signal handler is installed and the current signal >>>>> handler >>>>> > >>>> is saved to an array. >>>>> > >>>> On linux, this array had size MAXSIGNUM=32, and >>>> JAVASRSIGNUM >>>>> > >> was >>>>> > >>>> allowed >>>>> > >>>> to range up to NSIG=65. This could cause memory corruption. >>>>> > >>>> >>>>> > >>>> Further, in jsig.c, an unsinged int is used to set a bit for >>>>> signals. >>>>> > >>>> This also >>>>> > >>>> is too small, as only 32 signals can be supported. Further, the >>>>> > >>>> signals are mapped >>>>> > >>>> wrong to these bits. '0' is not a valid signal, but '32' >>>>> was. 1<<32_ _>>>>> > >>>> happens to map to >>>>> > >>>> zero, so the signal could be stored, but this probably was not >>>>> > >>>> intended that way. >>>>> > >>>> >>>>> > >>>> This change increases MAXSIGNUM to 65 on linux, and to 64 on >>>>> aix. It >>>>> > >>>> introduces >>>>> > >>>> proper checking of the signal read from the env var, and issues >> a >>>>> > >>>> warning if it >>>>> > >>>> does not use the signal set. It adapts the data types in jisig.c >>>>> > >>>> properly. >>>>> > >>>> >>>>> > >>>> Please review this change. I please need a sponsor. >>>>> > >>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529- >>>> NSIG/webrev.00 >>>>> > >>> >>>>> > >>> This all sounds very good to me. (I must find out why Solaris >>>>> is not >>>>> > >>> involved here :) ). >>>>> > >>> >>>>> > >>> On Linux you didn't add the bounds check to >>>>> os::Linux::setoursigflags. >>>>> > >>> >>>>> > >>> I'm also wondering about documenting where we are >> determining >>>> the >>>>> > >>> maximum from? Is it simply NSIG on some/all distributions? >>>>> And I see >>>>> > >>> NSIG is supposed to be the biggest signal number + one. Also >>>>> linux >>>>> > >>> defines NSIG = NSIG so which should we be using? >>>>> > >>> >>>>> > >>> Thanks, >>>>> > >>> David >>>>> > >>> >>>>> > >>>> Best regards, >>>>> > >>>> Goetz. >>>>> > >>>> >>>>> > >> >>>>> > >> >>>>> > >> -- >>>>> > >> Dmitry Samersoff >>>>> > >> Oracle Java development team, Saint Petersburg, Russia >>>>> > >> * I would love to change the world, but they won't give me the >>>>> sources. >>>>> > >>>>> > >>>>> > -- >>>>> > Dmitry Samersoff >>>>> > Oracle Java development team, Saint Petersburg, Russia >>>>> > * I would love to change the world, but they won't give me the >>>>> sources. >>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Dmitry Samersoff >>>> Oracle Java development team, Saint Petersburg, Russia >>>> * I would love to change the world, but they won't give me the sources. >> >> >> -- >> Dmitry Samersoff >> Oracle Java development team, Saint Petersburg, Russia >> * I would love to change the world, but they won't give me the sources.

-- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.

More information about the hotspot-runtime-dev mailing list