RFR: 8294594: Fix cast-function-type warnings in signal handling code [v2]

[Aleksey Shipilev]

On Tue, 11 Oct 2022 08:37:59 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>>> > > Fix looks good as far as it goes (can't believe I didn't see what was going on here!) - but `os::signal` is still broken as it uses `sa_handler` instead of `sa_sigaction`.
>>> > 
>>> > 
>>> > Yikes! I think this change should proceed as is, and `os::signal` should be looked at as a new issue. That looks messy :(
>>> 
>>> There is no rush, because we are waiting for another build system change to drop.
>>> 
>>> Why can't we do the same thing we did for `SR_handler` in `SR_initialize`?
>>> 
>>> ```
>>> @@ -864,7 +864,7 @@ void* os::signal(int signal_number, void* handler) {
>>>    remove_error_signals_from_set(&(sigAct.sa_mask));
>>>  
>>>    sigAct.sa_flags   = SA_RESTART|SA_SIGINFO;
>>> -  sigAct.sa_handler = CAST_TO_FN_PTR(sa_handler_t, handler);
>>> +  sigAct.sa_sigaction = CAST_TO_FN_PTR(sa_sigaction_t, handler);
>>>  
>>>    if (sigaction(signal_number, &sigAct, &oldSigAct)) {
>>>      // -1 means registration failed
>>> ```
>>> 
>>> It matches what we should do for `SIG_INFO` flag, and as Kim said, it is still likely yields the same code as `sa_handler` and `sa_sigaction` are probably the same on currently supported systems.
>> 
>> For POSIX we expect the handler argument for os::signal to be a sigaction handler (taking 3 arguments).  For Windows we expect os::signal to take a 1 argument handler, since Windows seemingly doesn't support the sigaction stuff.  How is that a portability layer?
>> 
>> So all of the calls are going to need their handler functions examined.  In at least one use (in os_windows), the UserHandler function takes 3 arguments though only uses one, and is called with only one.
>> 
>> Looking at where os::signal is called, it's not clear it needs to be in os, and given the inconsistent expectations it probably shouldn't be.  All calls are in posix-specific or windows-specific files.  I'd suggest removing os::signal entirely, and having windows code use ::signal and posix code use ::sigaction.  Maybe with some convenience wrappers around each, but those wrappers are windows-specific or posix-specific and never the twain shall meet.
>> 
>> That all seems to me like it would be better to separate from this change, keeping this one nice and simple.
>
>> For POSIX we expect the handler argument for os::signal to be a sigaction handler (taking 3 arguments). For Windows we expect os::signal to take a 1 argument handler, since Windows seemingly doesn't support the sigaction stuff. How is that a portability layer?
> 
> D'oh.
> 
>> That all seems to me like it would be better to separate from this change, keeping this one nice and simple.
> 
> Agreed.

> ...unless @shipilev really wants it? :)

No, I do NOT want it :) I'll be happy to test it, though.

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

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