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.
