On Tue, 11 Oct 2022 08:37:59 GMT, Aleksey Shipilev <sh...@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