RFR (XS) 8202014: Possible to receive signal before signal semaphore created

[Thomas Stüfe]

Beautiful, thank you :)

..Thomas

On Fri, May 18, 2018 at 2:59 PM,  <coleen.phillimore at oracle.com> wrote:
>
>
> On 5/18/18 8:38 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>>
>> On 5/18/18 2:07 AM, Thomas Stüfe wrote:
>>>
>>> Hi Coleen,
>>>
>>> looks good, thanks for taking my suggestion.
>>>
>>> Thoughts for further improvements:
>>>
>>> - os::signal_init could be renamed to something more to the point,
>>> e.g. os::initialize_signal_dispatcher_thread() or
>>> os::initialize_jdk_signal_support() or similar
>>
>>
>> Okay os::signal_init => os::initialize_jdk_signal_support()
>>>
>>>
>>> - os::signal_init_pd could be removed from os.hpp altogether and made
>>> file scope static in the os_xxx.cpp file. It could also have a better
>>> name.
>>
>>
>> os::signal_init_pd => jdk_misc_signal_init().
>>>
>>> I leave it up to you if you do this in a follow up item, or even at
>>> all. The fix is fine to me in its current form.
>>
>>
>> I'll make these changes with this minor change.  Hopefully this is
>> agreeable.
>
>
> http://cr.openjdk.java.net/~coleenp/8202014.03/webrev
>
> Rerunning oracle platform tier1,2 to make sure I didn't break anything.
>
> thanks,
> Coleen
>
>
>>
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks, Thomas
>>>
>>>
>>>
>>>
>>> On Thu, May 17, 2018 at 9:25 PM, <coleen.phillimore at oracle.com> wrote:
>>>>
>>>>
>>>> On 5/16/18 2:04 AM, Thomas Stüfe wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I agree with David. Signal handlers are installed in os::init_2(), but
>>>>> the semaphore sometime later in the (imho misleadingly named) void
>>>>> os::signal_init(). This looks like an initialization error.
>>>>>
>>>>> As a side note, can we make the "void signalHandler(int sig,
>>>>> siginfo_t* info, void* uc)" functions in os_xxx.cpp static, since they
>>>>> are only file local marshalling points?
>>>>
>>>>
>>>> Okay, I've moved the initialization of the signal semaphore to
>>>> os::init_2().
>>>> The signal handler for this signal_notify() case is set up by
>>>> JVM_RegisterSignal early in JDK initialization (initPhase1) which is
>>>> before
>>>> os::signal_init() was run.  Now the semaphore is initialized before
>>>> that.
>>>>
>>>> I also made signalHandler static in this change although not strictly
>>>> related.
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/8202014.02/webrev/index.html
>>>>
>>>> Reran kill -INT script and mach5 hs_tiers 1 and 2.
>>>> test/hotspot/jtreg/runtime/signal tests and
>>>> test/jdk/sun/misc/SunMiscSignalTest.java.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>
>>>>> Kind Regards, Thomas
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Wed, May 16, 2018 at 4:40 AM, David Holmes <david.holmes at oracle.com>
>>>>> wrote:
>>>>>>
>>>>>> Hi Coleen,
>>>>>>
>>>>>> On 16/05/2018 11:23 AM, coleen.phillimore at oracle.com wrote:
>>>>>>>
>>>>>>> Summary: Don't assert for null semaphore because it can be null
>>>>>>> before
>>>>>>> initialization is complete.
>>>>>>
>>>>>>
>>>>>> Sorry I don't agree with this. Surely we should not install our signal
>>>>>> handler until such time as everything is properly initialized? Or the
>>>>>> handler itself should have some kind of initialization check to guide
>>>>>> how
>>>>>> it
>>>>>> responds.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>
>>>>>>> Tested this with a shell script, thanks Robbin, and mach5 tier1,2 all
>>>>>>> Oracle platforms.  AIX doesn't use the semaphore class so doesn't
>>>>>>> have
>>>>>>> this
>>>>>>> bug.
>>>>>>>
>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8202014.01/webrev
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8202014
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>
>>>>
>>
>