RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
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 _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.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 _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.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 _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.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.
> >>> 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.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 _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.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 _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.
More information about the hotspot-runtime-dev
mailing list