That's great, thanks a lot! Best regards, Goetz.
> -----Original Message----- > From: Dmitry Samersoff [mailto:dmitry.samers...@oracle.com] > Sent: Dienstag, 17. November 2015 09:34 > To: Lindenmaier, Goetz; Thomas Stüfe > Cc: David Holmes; hotspot-runtime-...@openjdk.java.net; serviceability- > dev > Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM > > 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.samers...@oracle.com] > >> Sent: Montag, 16. November 2015 11:53 > >> To: Lindenmaier, Goetz; Thomas Stüfe > >> Cc: David Holmes; hotspot-runtime-...@openjdk.java.net; serviceability- > >> dev > >> Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM > >> > >> 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 os_aix.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.stu...@gmail.com] > >>> *Sent:* Freitag, 13. November 2015 10:54 > >>> *To:* Lindenmaier, Goetz > >>> *Cc:* Dmitry Samersoff; David Holmes; > >>> hotspot-runtime-...@openjdk.java.net; serviceability-dev > >>> *Subject:* Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM > >>> > >>> > >>> > >>> 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/os_aix.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 ? > >>> > >>> > >>> > >>> +sigset_t os::Aix::sigs = { 0 }; > >>> > >>> > >>> > >>> I would not initialize the signal set this way. sigset_t 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/os_aix.hpp.udiff.html > >>> > >>> > >>> > >>> + static struct sigaction sigact[NSIG]; // saved preinstalled sigactions > >>> > >>> + static sigset_t 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 os_aix.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 os_aix.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/os_bsd.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 os_bsd.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 os_solaris.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.lindenma...@sap.com <mailto:goetz.lindenma...@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. > >>> sigset_t is >> NSIG on linux, so it's no good idea to use it there. > >>> > >>> > >>> > >>> Why do we not do this on all platforms, provided sigset_t 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.samers...@oracle.com > >> <mailto:dmitry.samers...@oracle.com>] > >>> > >>> > Sent: Donnerstag, 12. November 2015 10:05 > >>> > To: Lindenmaier, Goetz; David Holmes; hotspot-runtime- > >>> > d...@openjdk.java.net <mailto:d...@openjdk.java.net>; > serviceability- > >> dev > >>> > Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM > >>> > > >>> > 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 > >>> os_linux, too. > >>> > > So it's really more consistent. > >>> > > > >>> > > Best regards, > >>> > > Goetz > >>> > > > >>> > >> -----Original Message----- > >>> > >> From: Dmitry Samersoff [mailto:dmitry.samers...@oracle.com > >>> <mailto:dmitry.samers...@oracle.com>] > >>> > >> Sent: Donnerstag, 12. November 2015 09:22 > >>> > >> To: David Holmes; Lindenmaier, Goetz; hotspot-runtime- > >>> > >> d...@openjdk.java.net <mailto:d...@openjdk.java.net>; > >>> serviceability-dev > >>> > >> Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM > >>> > >> > >>> > >> 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 _JAVA_SR_SIGNUM 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 > >> _JAVA_SR_SIGNUM > >>> > >> 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::set_our_sigflags. > >>> > >>> > >>> > >>> 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.