Great, looks good. Thanks a lot! Best regards, Goetz.
> -----Original Message----- > From: Dmitry Samersoff [mailto:dmitry.samers...@oracle.com] > Sent: Dienstag, 17. November 2015 14:42 > 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, > > 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.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. > > > -- > Dmitry Samersoff > Oracle Java development team, Saint Petersburg, Russia > * I would love to change the world, but they won't give me the sources.
