Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8253742

POSIX signal code cleanup

    XMLWordPrintable

    Details

    • Type: Enhancement
    • Status: Resolved
    • Priority: P4
    • Resolution: Fixed
    • Affects Version/s: 16
    • Fix Version/s: 16
    • Component/s: hotspot
    • Subcomponent:
    • Resolved In Build:
      b26

      Description

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

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              gziemski Gerard Ziemski
              Reporter:
              gziemski Gerard Ziemski
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: