A few cleanup opportunities came up during review for JDK-8252324:

#1 src/hotspot/os/posix/signals_posix.cpp
// signal handling (except suspend/resume)
// suspend/resume

 // glibc on Bsd platform uses non-documented flag
 
 @dholmes-ora dholmes-ora 15 hours ago  Member
This was added for Linux and then copied to the BSD port, so the comment is inaccurate. This seems to be referring to the SA_RESTORER flag which seems to be a linux kernel flag. But I'm unsure why we would need to even be aware of this. I think future cleanup could be done here.


#2 src/hotspot/os/posix/signals_posix.cpp

Are the following useful on other the other POSIX platforms? Currently they are only used on AIX:

set_thread_signal_mask()
unblock_program_error_signals()


#3 src/hotspot/os/posix/signals_posix.cpp

dholmes-ora 15 hours ago  Member
There should be a better way to dispatch here. If the handler has a platform-independent name, and is declared in the platform specific files, then it will link to that one definition.

#if defined(BSD)
  JVM_handle_bsd_signal(sig, info, uc, true);
#elif defined(AIX)
  JVM_handle_aix_signal(sig, info, uc, true);
#else
  JVM_handle_linux_signal(sig, info, uc, true);
#endif

Same with:

address PosixSignals::ucontext_get_pc(const ucontext_t* ctx) {
#if defined(AIX)
   return os::Aix::ucontext_get_pc(ctx);
#elif defined(BSD)
   return os::Bsd::ucontext_get_pc(ctx);
#elif defined(LINUX)
   return os::Linux::ucontext_get_pc(ctx);
#else
   VMError::report_and_die("unimplemented ucontext_get_pc");
#endif
}

void PosixSignals::ucontext_set_pc(ucontext_t* ctx, address pc) {
#if defined(AIX)
   os::Aix::ucontext_set_pc(ctx, pc);
#elif defined(BSD)
   os::Bsd::ucontext_set_pc(ctx, pc);
#elif defined(LINUX)
   os::Linux::ucontext_set_pc(ctx, pc);
#else
   VMError::report_and_die("unimplemented ucontext_set_pc");
#endif
}

#4 In src/hotspot/os/posix/signals_posix.hpp

#ifndef OS_POSIX_SIGNALS_POSIX_HPP
#define OS_POSIX_SIGNALS_POSIX_HPP

 #include "memory/allocation.hpp"
 
 @coleenp coleenp 20 hours ago  Member
There's a new memory/allStatic.hpp header file you can include instead.


#5 src/hotspot/os/posix/signals_posix.hpp

 // Signal number used to suspend/resume a thread
// do not use any signal number less than SIGSEGV, see 4355769
static int SR_signum = SIGUSR2;
 
 @coleenp coleenp 20 hours ago  Member
Can you hide this in the .cpp file so that you can avoid including <signal.h> in the header file?

And use forward declarations for outputStream, ucontext_t, and siginfo_t.
 
 @coleenp coleenp 15 hours ago  Member
Note that these changes are not important enough to rerun testing for this cleanup, so you can make it a new RFE. Thank you for doing consolidation!
 
 @dholmes-ora dholmes-ora 13 hours ago  ��� 
edited 
 Member
In reply to Coleen's query - SR_signum is referenced from the os_*.cpp files so needs to be in the header file now.
 
 @tstuefe tstuefe 9 hours ago  Member
I wondered why this is not const then I saw that it can be overwritten with an environment variable "_JAVA_SR_SIGNUM". Is this still needed? Seems to be an odd way of specifying a VM parameter. And it seems to be untested (I did not find any tests for that in the open codebase).

 @dholmes-ora dholmes-ora 14 hours ago  ��� 
In reply to Coleen's query - SR_signum is referenced from the os_*.cpp files so needs to be in the header file now.

tstuefe 9 hours ago  Member
I wondered why this is not const then I saw that it can be overwritten with an environment variable "_JAVA_SR_SIGNUM". Is this still needed? Seems to be an odd way of specifying a VM parameter. And it seems to be untested (I did not find any tests for that in the open codebase).


#6 src/hotspot/os/aix/os_aix.cpp

  // We also want to know if someone else adds a SIGDANGER handler because
  // that will interfere with OOM killling.
  print_signal_handler(st, SIGDANGER, buf, buflen);
  PosixSignals::print_signal_handler(st, SIGDANGER, buf, buflen);
}
 
 @coleenp coleenp 20 hours ago  Member
I wonder with some #ifdefs, this could also be moved to posix_signals.cpp. Not for this change but maybe a follow-up RFE.
 
 @tstuefe tstuefe 9 hours ago  Member
Yes, this os::print_signal_handlers() and os::print_signal_handler() could be unified across Posix platforms. At least the latter.

(Personally I also would like to print all signal handlers from 1.., regardless of whether its one of "us". E.g. if process overrides SIGCHILD that is interesting too.)


#7 src/hotspot/os/bsd/os_bsd.cpp
@@ -3207,10 +2282,10 @@ bool os::bind_to_processor(uint processor_id) {
}

 void os::SuspendedThreadTask::internal_do_task() {
  if (do_suspend(_thread->osthread())) {
  if (PosixSignals::do_suspend(_thread->osthread())) {
 
 @tstuefe tstuefe 9 hours ago  Member
Future RFE: os::SuspendedThreadTask::internal_do_task() can be unified across posix platforms.


#8 src/hotspot/os/bsd/os_bsd.cpp

  // initialize suspend/resume support - must do this before signal_sets_init()
  if (SR_initialize() != 0) {
  if (PosixSignals::SR_initialize() != 0) {
    perror("SR_initialize failed");
    return JNI_ERR;
  }

   Bsd::signal_sets_init();
  Bsd::install_signal_handlers();
  PosixSignals::signal_sets_init();
  PosixSignals::install_signal_handlers();
  // Initialize data for jdk.internal.misc.Signal
  if (!ReduceSignalUsage) {
    jdk_misc_signal_init();
    PosixSignals::jdk_misc_signal_init();
  }
Comment on lines 2141 to 2152
 
 @tstuefe tstuefe 9 hours ago  Member
This whole section could be unified too into an own PosixSignals::initial_signal_stuff() function. Potentially for a future RFE.


#9 src/hotspot/os/posix/signals_posix.cpp
extern "C" JNIEXPORT int JVM_handle_linux_signal(int signo, siginfo_t* siginfo,
                                               void* ucontext,
                                               int abort_if_unrecognized);
#endif
 
 @tstuefe tstuefe 9 hours ago  Member
Future cleanup: I think it will be difficult to unify the platform specific signal handlers (but worthwhile). But the naming could be at least the same, no reason to have the platform in the name.
 
 @YaSuenag YaSuenag 6 hours ago  Member
I agree with @tstuefe . Can we rename them to JVM_handle_posix_signal ?


#10 src/hotspot/os/posix/signals_posix.cpp
    tty->cr();
    tty->print("  found:");
    print_sa_flags(tty, act.sa_flags);
    tty->cr();
 
 @YaSuenag YaSuenag 6 hours ago  Member
Can we use unified logging at here? If do so, we should out them with single line.


#11 src/hotspot/os/posix/signals_posix.cpp

 void PosixSignals::jdk_misc_signal_init() {
  // Initialize signal structures
  ::memset((void*)pending_signals, 0, sizeof(pending_signals));
 
 @YaSuenag YaSuenag 6 hours ago  Member
Is this code needed? pending_signals is initialized with { 0 } and also jdk_misc_signal_init() would be called from os::init_2() .