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

Thomas Stuefe stuefe at openjdk.java.net
Fri Feb 5 11:00:44 UTC 2021 

On Fri, 5 Feb 2021 06:38:58 GMT, Thomas Stuefe <stuefe at openjdk.org> 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

More information about the hotspot-dev mailing list