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

[coleen.phillimore at oracle.com]

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
>>>
>