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

Dmitry Samersoff dmitry.samersoff at oracle.com
Tue Nov 17 08:33:40 UTC 2015

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

More information about the hotspot-runtime-dev mailing list