RFR: 8253742: POSIX signal code cleanup [v7]

[Thomas Stuefe]

On Mon, 16 Nov 2020 17:18:29 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> hi all,
>> 
>> Please review this followup to [JDK-8252324 Signal related code should be shared among POSIX platforms](https://bugs.openjdk.java.net/browse/JDK-8252324), where several issues were identified for a cleanup. This change addresses them all:
>> 
>> #1 David's feedback - removed non POSIX SIGNIFICANT_SIGNAL_MASK code
>> 
>> #2 David's feedback - used unblock_program_error_signals() on all platforms (reverted for JDK-8252533)
>> 
>> #3 David's feedback - used single JVM_handle_posix_signal API for all POSIX platforms (reverted for JDK-8255711)
>> 
>> #4 Coleen's feedback - cleanup header files in src/hotspot/os/posix/signals_posix.hpp
>> 
>> #5 Coleen's feedback - hid SR_signum assignment in src/hotspot/os/posix/signals_posix.hpp to avoid having to include <signal.h>
>> 
>> #6 Coleen's feedback - factored out print_signal_handlers()
>> 
>> #7 Thomas' feedback - factored out common POSIX os::SuspendedThreadTask::internal_do_task()
>> 
>> #8 Thomas's feedback - factored out common POSIX signal initialization code
>> 
>> #9 YaSuenag's feedback - used JVM_handle_posix_signal for the common API
>> 
>> #10 YaSuenag's feedback - unified logging out of the scope for this fix
>> 
>> #11 YaSuenag's feedback - memset usage in PosixSignals::jdk_misc_signal_init() correct?
>
> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix missing os::print_signal_handlers on Windows, white space on Linux

Hi Gerard,

we are getting closer :)

Cheers, Thomas

src/hotspot/os/posix/signals_posix.hpp line 31:

> 29: #include "utilities/globalDefinitions.hpp"
> 30: 
> 31: // Forward declarations to be independent of the include structure.

I don't think you have to write this, this is clear from the context (we do this in a zillion other places too).

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

> 1387: }
> 1388: 
> 1389: int PosixSignals::unblock_thread_signal_mask(const sigset_t *set) {

I don't think we need this (nor the prototype in the header)

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

> 1722:   // initialize suspend/resume support - must do this before signal_sets_init()
> 1723:   if (SR_initialize() != 0) {
> 1724:     perror("SR_initialize failed");

perror() is old :) does not make much sense here either since errno could be stale. Could you pls switch this to whatever we do at this point when facing an unrecoverable error (probly vm_exit_initialization or similar)

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

> 1391: }
> 1392: 
> 1393: void signal_sets_init() {

If this is not needed outside this compilation unit, make it pls static.

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

Changes requested by stuefe (Reviewer).

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