RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
Dmitry Samersoff
dmitry.samersoff at oracle.com
Tue Nov 17 13:41:54 UTC 2015
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.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.
--
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