RFR: 8295125: os::signal should be os specific

[David Holmes]

On Thu, 20 Oct 2022 06:29:08 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> This was intended as a significant cleanup RFE but it turned out things were a lot messier than initially envisaged, so the resultant cleanup is quite limited.
>> 
>> The OS API/namespace was simplified by removing three signal related functions that are not needed in shared code:
>>  - signal
>>  - user_handler
>>  - raise
>> The first two were reworked and placed into PosixSignals and os::win32.
>> There was no need for a specific XX::raise API as os::raise simply called ::raise.
>> 
>> On the Posix side we had the basic signal handler installation code in two places:
>> 
>> 1. Inline in set_signal_handler and used by nearly all the VM signals
>> 2. In os::signal which was used by three different "clients"
>>   - VM error handler for updating crash handler
>>   - VM for installing SIG_BREAK handler
>>   - JDK via JVM_RegisterSignal
>> 
>> This was refactored so that we have:
>> 
>> 1. PosixSignals::install_sigaction_signal_handler
>>   - used by the VM only
>>     - set_signal_handler
>>     - VM error handler for updating crash handler
>>     - VM for installing SIG_BREAK handler
>> 
>> 2. PosixSignals::install_generic_signal_handler
>>   - used by JDK via JVM_RegisterSignal
>> 
>> The sigaction handlers were all consistently defined as per the posix definition:
>>     void func(int signo, siginfo_t *info, void *context);
>>   
>> 
>> On the Windows side we just fixed the incorrect definition of UserHandler and made some consistency changes to use the right signal handler type where possible.
>> 
>> Thanks.
>
> src/hotspot/os/windows/jvm_windows.cpp line 48:
> 
>> 46: JVM_ENTRY_NO_ENV(void*, JVM_RegisterSignal(jint sig, void* handler))
>> 47:   signal_handler_t newHandler = handler == (void *)2 ?
>> 48:                                 CAST_TO_FN_PTR(signal_handler_t, os::win32::user_handler()) :
> 
> If `user_handler` returned `signal_handler_t` instead of `void*` then this cast wouldn't be needed.

See previous response.

> src/hotspot/os/windows/os_windows.hpp line 128:
> 
>> 126:   // signal support
>> 127:   static void* install_signal_handler(int sig, signal_handler_t handler);
>> 128:   static void* user_handler();
> 
> Why does `user_handler` return `void*` instead of `signal_handler_t`?  I'd prefer the handler type be used everywhere except where it really can't be, which I _think_ is only the parameter and return type of JVM_RegisterSignal.  I don't know if that makes some places overly messy though.

If `user_handler` returned the actual type you would have to cast it to `void*` in the later comparison, just moving the cast in `JVM_RegisterSignal` but saving the cast in `os::win32::user_handler()`. This seemed a small gain compared to maintaining consistency with the Posix version - but I can change it if you want.

-------------

PR: https://git.openjdk.org/jdk/pull/10780