RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code

David Holmes dholmes at openjdk.java.net
Tue Feb 2 10:06:43 UTC 2021 

On Wed, 27 Jan 2021 09:18:19 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> In signal handling code, we have code sections which save signal handler state into vectors of sigaction structures, or of integers (if only flags are saved). All these code sections can be unified, disentangled and the using code simplified.
> 
> There are three places where we do this:
> 
> 1) When installing hotspot signal handlers, should we find a handler in place and signal chaining is enabled, we save the original handler inside a sigaction array and a corresponding sigset:
> https://github.com/openjdk/jdk/blob/e0d748d56f817b4fb5158aae86aee12d90b36356/src/hotspot/os/posix/signals_posix.cpp#L85
> https://github.com/openjdk/jdk/blob/e0d748d56f817b4fb5158aae86aee12d90b36356/src/hotspot/os/posix/signals_posix.cpp#L338
> 
> 2) if diagnostics are enabled with -Xcheck:jni, we periodically check if our hotspot signal handlers had been replaced (`static void check_signal_handler(int sig)`):
> https://github.com/openjdk/jdk/blob/e0d748d56f817b4fb5158aae86aee12d90b36356/src/hotspot/os/posix/signals_posix.cpp#L766
> To do that, we store information about the handlers we installed and we expect to be intact; in this case we only store the sigaction flags (`int sigflags[NSIG];`) and deduce the handler address from context.
> 
> 3) There is a complicated dance between VMError and the posix signal handler code: If a fatal error happens, we enter error reporting and install the secondary handler (`VMError::install_secondary_signal_handler()`). Before doing that, we store the handler we replace in yet another array, in this case one array for the handler address, one for the flag:
> https://github.com/openjdk/jdk/blob/e0d748d56f817b4fb5158aae86aee12d90b36356/src/hotspot/os/posix/vmError_posix.cpp#L77
> I believe the purpose of this is to - when printing signal handlers as part of error reporting - print the original signal handler instead of the secondary crash handler (see `PosixSignals::print_signal_handler()`):
> https://github.com/openjdk/jdk/blob/e0d748d56f817b4fb5158aae86aee12d90b36356/src/hotspot/os/posix/signals_posix.cpp#L1372
> and additionally to not trip this warning here:
> https://github.com/openjdk/jdk/blob/e0d748d56f817b4fb5158aae86aee12d90b36356/src/hotspot/os/posix/signals_posix.cpp#L1391
> 
> ------
> 
> Changes in this patch:
> 
> - I added some convenience macros to check if a handler matches a given function (HANDLER_IS), check if a handler is set to ignore or default or both (HANDLER_IS_IGN, HANDLER_IS_DFL, HANDLER_IS_IGN_OR_DFL). Makes code more readable.
> - I added convenience class `SavedSignalHandlers` to keep a vector of handler information by signal number.
> - I used that class to cover cases (1)..(3):
> 	- `chained_handlers` contains all information of chained handlers
> 	- `expected_handlers` contains a copy of the handlers the hotspot installed
> 	- `replaced_handlers` contains information about replaced handlers
> 
> - about (1): I store the chained signal handler information in `chained_handlers` when installing a hotspot handler, UseSignalChaining is 1, and a non-default handler was encountered.
> 
> - about (2): I simplified the signal checking mechanism quite a bit: it compares the handler (address and flags) it finds present with expectations. Before this patch, the expected handler address was deduced in a hard-wired way, now, we just compare the active sigaction structure with the one we installed on VM start.
> 
> - about (3): when installing any handler (hotspot as well as user defined via java), I store the handler it replaced in `replaced_handlers`. I use that to print which handler had been replaced in `PosixSignals::print_signal_handler`. I simplified `PosixSignals::print_signal_handler` such that it does not retain any knowledge about hotspot signal handlers. Now, it just prints out the currently established handlers. In addition to that, it prints out chaining information and which handlers had been replaced. I removed the associated coding from VMError.
> 
> Output Before:
> 663 Signal Handlers:
> 664    SIGSEGV: javaSignalHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 665     SIGBUS: javaSignalHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 666     SIGFPE: javaSignalHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 667    SIGPIPE: javaSignalHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 668    SIGXFSZ: javaSignalHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 669     SIGILL: javaSignalHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 670    SIGUSR2: SR_handler in libjvm.so, sa_mask[0]=00000000000000000000000000000000, sa_flags=SA_RESTART|SA_SIGINFO
> 671     SIGHUP: UserHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 672     SIGINT: UserHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 673    SIGTERM: UserHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 674    SIGQUIT: UserHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 675    SIGTRAP: javaSignalHandler in libjvm.so, sa_mask[0]=11100100010111111101111111111110, sa_flags=SA_RESTART|SA_SIGINFO
> 
> Now:
>  Signal Handlers:
>     SIGSEGV: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>    replaced:    SIGSEGV: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>      SIGBUS: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>    replaced:     SIGBUS: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>      SIGFPE: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>    replaced:     SIGFPE: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>     SIGPIPE: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>     SIGXFSZ: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>      SIGILL: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>    replaced:     SIGILL: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>     SIGUSR2: SR_handler in libjvm.so, mask=00000000000000000000000000000000, flags=SA_RESTART|SA_SIGINFO
>      SIGHUP: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>      SIGINT: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>     SIGTERM: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>     SIGQUIT: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
>     SIGTRAP: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
> 
> -----
> Tests: GA, and the patch has been tested in our nighlies for over a month now. I manually executed the runtime/jni/checked tests too.

Hi Thomas,

I've taken a first pass at this. I like parts of it, perhaps most of it, but am having trouble seeing the before/after picture clearly.

A few comments below.

Thanks,
David

src/hotspot/os/posix/signals_posix.cpp line 99:

> 97:   bool check_signal_number(int sig) const {
> 98:     assert(sig > 0 || sig < NSIG, "invalid signal number %d", sig);
> 99:     return sig > 0 || sig < NSIG;

Surely && not ||

src/hotspot/os/posix/signals_posix.cpp line 94:

> 92: //  structures and membership information.
> 93: class SavedSignalHandlers {
> 94:   struct sigaction _v[NSIG];

Why _v ??

src/hotspot/os/posix/signals_posix.cpp line 105:

> 103:   void mark_as_set(int sig)   { sigaddset(&_set, sig); }
> 104:   void mark_as_clear(int sig) { sigdelset(&_set, sig); }
> 105: 

These don't really pay for themselves IMO. They are private and only used once each, and are a single line of code. The extra abstraction layer really doesn't add anything in terms of clarity.

src/hotspot/os/posix/signals_posix.cpp line 142:

> 140: //  Our own hotspot signal handlers should never ever get replaced by a third
> 141: //  party one. To check that, store a copy of the handler setup and compare it
> 142: //  periodically against reality (see see os::run_periodic_checks()).

typo: see see

src/hotspot/os/posix/signals_posix.cpp line 844:

> 842:     if (sig == SHUTDOWN2_SIGNAL && !isatty(fileno(stdin))) {    // Flags?
> 843:       tty->print_cr("Running in non-interactive shell, %s handler is replaced by shell",
> 844:                     os::exception_name(sig, buf, O_BUFLEN));    // When comparing, ignore the SA_RESTORER flag on Linux

I don't understand the flag comments in this block.

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

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2251

More information about the hotspot-dev mailing list