RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code
Thomas Stuefe
stuefe at openjdk.java.net
Wed Jan 27 10:43:55 UTC 2021
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.
-------------
Commit messages:
- JDK-8260485-signal-handler-improvements
Changes: https://git.openjdk.java.net/jdk/pull/2251/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8260485
Stats: 328 lines in 4 files changed: 126 ins; 132 del; 70 mod
Patch: https://git.openjdk.java.net/jdk/pull/2251.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2251/head:pull/2251
PR: https://git.openjdk.java.net/jdk/pull/2251
More information about the hotspot-dev
mailing list