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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Tue Nov 17 08:35:21 UTC 2015

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.

More information about the hotspot-runtime-dev mailing list