RFR: 8295125: os::signal should be os specific
Kim Barrett
kbarrett at openjdk.org
Thu Oct 20 16:47:51 UTC 2022
On Thu, 20 Oct 2022 05:59:33 GMT, David Holmes <dholmes 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.
Changes requested by kbarrett (Reviewer).
src/hotspot/os/posix/signals_posix.cpp line 877:
> 875: // - sigAct: the new struct sigaction to be filled in and used
> 876: // for this signal. The caller must provide this as it
> 877: // may need to be stored/accessed by that caller.
Maybe I missed something, but I didn't find any places where the caller needs sigAct.
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.
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.
-------------
PR: https://git.openjdk.org/jdk/pull/10780
More information about the hotspot-runtime-dev
mailing list