RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]
David Holmes
dholmes at openjdk.java.net
Wed Feb 3 01:00:55 UTC 2021
On Tue, 2 Feb 2021 17:31:03 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.
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>
> - Make SavedSignalHandlers use C-heap for its items
> - Removed display-replaced-handler-logic
> - Feedback David
> - Merge
> - JDK-8260485-signal-handler-improvements
Hi Thomas,
Still a few minor comments and queries, but overall it is looking good.
Thanks,
David
src/hotspot/os/posix/signals_posix.cpp line 152:
> 150: // For signal-chaining:
> 151: // if chaining is active, chained_handlers contains all handlers which we
> 152: // did replace with our own and to which we must delegate.
Nit: s/which we did replace/we replaced/
src/hotspot/os/posix/signals_posix.cpp line 842:
> 840: if (sig == SHUTDOWN2_SIGNAL && !isatty(fileno(stdin))) {
> 841: tty->print_cr("Running in non-interactive shell, %s handler is replaced by shell",
> 842: os::exception_name(sig, buf, O_BUFLEN)); // When comparing, ignore the SA_RESTORER flag on Linux
What does this comment mean in this context ???
src/hotspot/os/posix/signals_posix.cpp line 1393:
> 1391: st->print(", flags=");
> 1392: int flags = act->sa_flags;
> 1393: // On Linux, hide the SA_RESTORE flag
typo: SA_RESTORE -> SA_RESTORER
src/hotspot/os/posix/signals_posix.cpp line 1401:
> 1399: // Print established signal handler for this signal.
> 1400: // - if this signal handler was installed by us and is chained to a pre-established user handler
> 1401: // it did replace, print that one too.
s/it did replace/it replaced/
src/hotspot/os/posix/signals_posix.cpp line 1392:
> 1390:
> 1391: // Print established signal handler for this signal.
> 1392: // - if this signal handler was installed by us and is chained to a pre-established user handler
It is not clear to me that this check still remains somewhere.
src/hotspot/os/posix/vmError_posix.cpp line 60:
> 58: }
> 59:
> 60: void VMError::interrupt_reporting_thread() {
I'm not clear what happened to these ?? (I'm not clear how they were used in the first place. :( )
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2251
More information about the hotspot-dev
mailing list