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

classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stuefe
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
Reply | Threaded
Open this post in threaded view
|

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

David Holmes-2
On Wed, 27 Jan 2021 09:18:19 GMT, Thomas Stuefe <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]

Thomas Stuefe
In reply to this post by Thomas Stuefe
> 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

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2251/files
  - new: https://git.openjdk.java.net/jdk/pull/2251/files/016d35c2..f8deb8a7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=00-01

  Stats: 23284 lines in 487 files changed: 8309 ins; 5639 del; 9336 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]

Thomas Stuefe
In reply to this post by David Holmes-2
On Tue, 2 Feb 2021 10:04:04 GMT, David Holmes <[hidden email]> wrote:

> 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

Thanks David.

I reduced the complexity of the patch somewhat by removing case (3) completely. I found that it adds very little useful functionality. So all that is left now is (1) the simplified handling of chained handlers and (2) the new logic for implementing the signal handler jni check.

I also changed/simplified `SavedSignalHandlers` somewhat. NSIG can be largeish (I believe 1024 on AIX) and that would have made the array-of-sigaction structures large too. Not that it really matters much. But I changed the class into a pointer array, with the sigaction structures living in C-Heap. Since this array is sparsely populated, that saves space. Also removes the need for the membership sigset, since a NULL slot would indicate its not set.

Further answers inline. Feel free to ask questions if something is unclear.

Thanks, Thomas

> 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 ||

Good catch, this was an oversight.

> src/hotspot/os/posix/signals_posix.cpp line 94:
>
>> 92: //  structures and membership information.
>> 93: class SavedSignalHandlers {
>> 94:   struct sigaction _v[NSIG];
>
> Why _v ??

v as in vector... I changed it to _sa.

> 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.

True. I removed the whole sigset. See my comment below.

> 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.

Just a typo.

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]

David Holmes-2
In reply to this post by Thomas Stuefe
On Tue, 2 Feb 2021 17:31:03 GMT, Thomas Stuefe <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]

Thomas Stuefe
On Wed, 3 Feb 2021 00:42:39 GMT, David Holmes <[hidden email]> wrote:

>> 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
>
> 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 ???

Nothing. This, like the "flags" comment before, must have been accidental paste errors. I went through the patch again, but I hope this was the last one.

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]

Thomas Stuefe
In reply to this post by David Holmes-2
On Wed, 3 Feb 2021 00:54:23 GMT, David Holmes <[hidden email]> wrote:

>> 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
>
> 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.

I removed this check since I did not think it very useful. We have the checking code with Xcheck:jni, which does the same check.

But I am now reconsidering. If we run without Xcheck, it still is useful. I'll re-add this test.

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]

Thomas Stuefe
In reply to this post by David Holmes-2
On Wed, 3 Feb 2021 00:57:07 GMT, David Holmes <[hidden email]> wrote:

>> 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
>
> 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. :( )

I think they were used for path (3). When printing signal handlers, we do this little test to check - similar to what we do in Xcheck:jni - to print out if someone changed the handler under us. See:
https://github.com/openjdk/jdk/blob/cb127a4bb5d95d19eb1f5e625b600311a2490135/src/hotspot/os/posix/signals_posix.cpp#L1372
and
https://github.com/openjdk/jdk/blob/cb127a4bb5d95d19eb1f5e625b600311a2490135/src/hotspot/os/posix/signals_posix.cpp#L1389
.
But when printing signal handlers as part of a hs-err file, we replaced the hotspot signal handlers with the secondary crash handler and would trigger this test. I think this whole logic is needed just to prevent this.

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v3]

Thomas Stuefe
In reply to this post by Thomas Stuefe
> 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 incrementally with one additional commit since the last revision:

  David Feedback

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2251/files
  - new: https://git.openjdk.java.net/jdk/pull/2251/files/f8deb8a7..44fa2199

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=01-02

  Stats: 27 lines in 3 files changed: 23 ins; 0 del; 4 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]

Thomas Stuefe
In reply to this post by David Holmes-2
On Wed, 3 Feb 2021 00:58:11 GMT, David Holmes <[hidden email]> wrote:

>> 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

I added a printout to print_signal_handler to print out a message if the expected handler changed. This is the same logic, somewhat abridged, applied at Xcheck:jni.

Only here, it fires whenever we print signal handlers independently from Xcheck:jni. So it gets also displayed in hs-err files or as part of a VM.info jcmd.

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v4]

Thomas Stuefe
In reply to this post by Thomas Stuefe
> 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 incrementally with one additional commit since the last revision:

  Fix build error on zlinux

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2251/files
  - new: https://git.openjdk.java.net/jdk/pull/2251/files/44fa2199..40201e1d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=02-03

  Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v4]

David Holmes-2
On Thu, 4 Feb 2021 10:08:07 GMT, Thomas Stuefe <[hidden email]> 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 incrementally with one additional commit since the last revision:
>
>   Fix build error on zlinux

Looks good. Thanks for the updates.

One query for further improvement below. :)

Thanks,
David

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

> 119:       assert(_sa[sig] == NULL, "Overwriting signal handler?");
> 120:       _sa[sig] = NEW_C_HEAP_OBJ(struct sigaction, mtInternal);
> 121:       *_sa[sig] = *act;

Sorry I missed the fact we weren't actually copying the incoming sigaction struct!

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

> 1424:       LINUX_ONLY(this_flag &= SA_RESTORER_FLAG_MASK;)
> 1425:       if (this_flag != expected_act->sa_flags) {
> 1426:         st->print_cr("  *** Flags changed! Consider using jsig library.");

Can't we easily print the difference in flags?

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

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v4]

Thomas Stuefe
On Fri, 5 Feb 2021 04:19:02 GMT, David Holmes <[hidden email]> wrote:

> Looks good. Thanks for the updates.
>
> One query for further improvement below. :)
>
> Thanks,
> David

Thanks for approval, David. Unfortunately I still have a strange build problem on one of our s390 boxes, so this is not done yet.

> src/hotspot/os/posix/signals_posix.cpp line 121:
>
>> 119:       assert(_sa[sig] == NULL, "Overwriting signal handler?");
>> 120:       _sa[sig] = NEW_C_HEAP_OBJ(struct sigaction, mtInternal);
>> 121:       *_sa[sig] = *act;
>
> Sorry I missed the fact we weren't actually copying the incoming sigaction struct!

Yes me too, and I'm annoyed by the fact that no tier1 test did catch that. The full jtreg suite would have catched it but it did not run yet since the last commit.

> src/hotspot/os/posix/signals_posix.cpp line 1426:
>
>> 1424:       LINUX_ONLY(this_flag &= SA_RESTORER_FLAG_MASK;)
>> 1425:       if (this_flag != expected_act->sa_flags) {
>> 1426:         st->print_cr("  *** Flags changed! Consider using jsig library.");
>
> Can't we easily print the difference in flags?

Sure. I'll change it.

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v5]

Thomas Stuefe
In reply to this post by Thomas Stuefe
> 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 incrementally with one additional commit since the last revision:

  Further fixes

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2251/files
  - new: https://git.openjdk.java.net/jdk/pull/2251/files/40201e1d..d2434ad2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=03-04

  Stats: 111 lines in 1 file changed: 34 ins; 45 del; 32 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v4]

Thomas Stuefe
In reply to this post by Thomas Stuefe
On Fri, 5 Feb 2021 06:38:58 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Looks good. Thanks for the updates.
>>
>> One query for further improvement below. :)
>>
>> Thanks,
>> David
>
>> Looks good. Thanks for the updates.
>>
>> One query for further improvement below. :)
>>
>> Thanks,
>> David
>
> Thanks for approval, David. Unfortunately I still have a strange build problem on one of our s390 boxes, so this is not done yet.

Hi David,

sorry, its not yet done.

Changes in this version:

1) I found that on zlinux, sigaction.sa_flags is actually a long unsigned int. Which surprised me since this is clearly not posix compatible. Therefore I rewrote the code, adding a new function `get_sanitized_sa_flags`, which hides the correct casting and also contains the stripping away of unwanted flags set by the kernel (SA_RESTORER). As a side effect this cleanly factors out the SA_RESTORER handling which is nice.

2) You requested that `print_single_signal_handler` should print the flag difference when spotting a change. I agree and expanded on this: now  `print_single_signal_handler` will print out the full expected handler information if it spots a difference (see below for examples). This contains more information that just the flags, and is actually less code.

3) Then I found that further code could be folded: We have a detailed printout for Xcheck:jni in `check_signal_handler`, but then we also print all signal handlers as part of that printout, which now (2) contains a verbose description of the found differences. Therefore the detailed printout in `check_signal_handler` was not needed anymore and could be removed. That also allows me to factor out the "compare sigaction structures semantically" logic into one function, called `compare_handler_info`. That again safes some code.

Now the logic is like this:

- If Xcheck:jni is not set, we still store the expected signal handler info. When printing signal handlers, e.g. as part of a hs_err file, in `os::print_signal_handlers`, we print a description of any changed handlers to nudge the supporter into the right direction.

- If Xcheck:jni is active, we will do our periodic checks; upon finding a difference, we do not elaborate but call `os::print_signal_handlers()` which will elaborate for us.

The final adjustment I had to make was to separate the "expected handler" info from the "check when Xcheck:jni is active" logic. That is because when Xcheck:jni is active and we find a mismatch, we want to disable checking for that signal in `check_signal_handler` but still see the difference in future printouts when calling `os::print_signal_handlers`, e.g. if later we crash and write a hs-err file.

I am really sorry for the added review work, but I think its a worthwhile change and complexity gets further reduced.

I thought about adding tests but testing this is quite difficult. So I tested manually by overwriting some of the hotspot signal handlers and check how the VM reacts.

First a hs-err file, good case:

659 Signal Handlers:
660    SIGSEGV: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
661     SIGBUS: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
662     SIGFPE: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
663    SIGPIPE: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
664    SIGXFSZ: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
665     SIGILL: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
666    SIGUSR2: SR_handler in libjvm.so, mask=00000000000000000000000000000000, flags=SA_RESTART|SA_SIGINFO
667     SIGHUP: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
668     SIGINT: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
669    SIGTERM: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
670    SIGQUIT: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
671    SIGTRAP: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO

Bad cases (I changed handlers for SIGFPE, SIGPIPE, SIGXFSZ and SIGUSR2):

hs-err file:

658 Signal Handlers:
659    SIGSEGV: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
660     SIGBUS: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
661     SIGFPE: badwolf in libjvm.so, mask=10000000000000000000000000000000, flags=none
662   *** Handler was modified!
663   *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
664    SIGPIPE: badwolf in libjvm.so, mask=10000000000000000000000000000000, flags=none
665   *** Handler was modified!
666   *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
667    SIGXFSZ: badwolf in libjvm.so, mask=10000000000000000000000000000000, flags=none
668   *** Handler was modified!
669   *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
670     SIGILL: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
671    SIGUSR2: badwolf in libjvm.so, mask=10000000000000000000000000000000, flags=none
672   *** Handler was modified!
673   *** Expected: SR_handler in libjvm.so, mask=00000000000000000000000000000000, flags=SA_RESTART|SA_SIGINFO
674     SIGHUP: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
675     SIGINT: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
676    SIGTERM: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
677    SIGQUIT: UserHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
678    SIGTRAP: crash_handler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO

jcmd VM.info:

...
Signal Handlers:
   SIGSEGV: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
    SIGBUS: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
    SIGFPE: badwolf in libjvm.so, mask=00000111001010000101001100010001, flags=none
  *** Handler was modified!
  *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
   SIGPIPE: badwolf in libjvm.so, mask=00000111001010000101001100010001, flags=none
  *** Handler was modified!
  *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
   SIGXFSZ: badwolf in libjvm.so, mask=00000111001010000101001100010001, flags=none
  *** Handler was modified!
  *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
    SIGILL: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
   SIGUSR2: badwolf in libjvm.so, mask=00000111001010000101001100010001, flags=none
  *** Handler was modified!
  *** Expected: 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: SIG_DFL, mask=00000000000000000000000000000000, flags=none


-Xcheck:jni output:

Warning: SIGFPE handler modified!                                                                                                                                                                                                                                                    
Signal Handlers:
   SIGSEGV: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
    SIGBUS: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
    SIGFPE: badwolf in libjvm.so, mask=11100100010111111101111111111110, flags=none
  *** Handler was modified!
  *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
   SIGPIPE: badwolf in libjvm.so, mask=11100100010111111101111111111110, flags=none
  *** Handler was modified!
  *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
   SIGXFSZ: badwolf in libjvm.so, mask=11100100010111111101111111111110, flags=none
  *** Handler was modified!
  *** Expected: javaSignalHandler in libjvm.so, mask=11100100110111111111111111111110, flags=SA_RESTART|SA_SIGINFO
    SIGILL: javaSignalHandler in libjvm.so, mask=11100100010111111101111111111110, flags=SA_RESTART|SA_SIGINFO
   SIGUSR2: badwolf in libjvm.so, mask=11100100010111111101111111111110, flags=none
  *** Handler was modified!
  *** Expected: 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: SIG_DFL, mask=00000000000000000000000000000000, flags=none
Consider using jsig library.
...
Thanks, Thomas

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v5]

David Holmes-2
In reply to this post by Thomas Stuefe
On Fri, 5 Feb 2021 10:57:00 GMT, Thomas Stuefe <[hidden email]> 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 incrementally with one additional commit since the last revision:
>
>   Further fixes

Still seems okay.

One query below.

Thanks,
David

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

> 152: static bool check_signals = true;
> 153: static SavedSignalHandlers expected_handlers;
> 154: static bool do_check_signal_periodically[NSIG];

Does this need explicit initialization to have false entries?

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

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v6]

Thomas Stuefe
In reply to this post by Thomas Stuefe
> 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 incrementally with one additional commit since the last revision:

  Use universal zero initializer for do_check_signal_periodically

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2251/files
  - new: https://git.openjdk.java.net/jdk/pull/2251/files/d2434ad2..06e1b030

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2251&range=04-05

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v5]

Thomas Stuefe
In reply to this post by David Holmes-2
On Tue, 9 Feb 2021 05:08:40 GMT, David Holmes <[hidden email]> wrote:

> Still seems okay.
>
> One query below.
>
> Thanks,
> David

Thank you David.

> src/hotspot/os/posix/signals_posix.cpp line 154:
>
>> 152: static bool check_signals = true;
>> 153: static SavedSignalHandlers expected_handlers;
>> 154: static bool do_check_signal_periodically[NSIG];
>
> Does this need explicit initialization to have false entries?

I think it does not since global static objects are zero-initialized and 0 = false; however, I made the initialization explicit.

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v6]

David Holmes-2
In reply to this post by Thomas Stuefe
On Tue, 9 Feb 2021 05:35:03 GMT, Thomas Stuefe <[hidden email]> 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 incrementally with one additional commit since the last revision:
>
>   Use universal zero initializer for do_check_signal_periodically

Marked as reviewed by dholmes (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/2251
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v6]

Thomas Stuefe
On Tue, 9 Feb 2021 05:43:05 GMT, David Holmes <[hidden email]> wrote:

>> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Use universal zero initializer for do_check_signal_periodically
>
> Marked as reviewed by dholmes (Reviewer).

Gentle ping..

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

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