Attention AIX developers - factoring out POSIX signal code (JDK-8252324)

Doerr, Martin martin.doerr at sap.com
Tue Sep 1 20:11:51 UTC 2020


Hi Gerard,

it compiles without errors and warnings with the following patch. I don’t think we still need this old stuff, but I hope Thomas can find some time to double-check.
I haven’t run it through our testing, yet.

Best regards,
Martin


diff -r ec42084221a6 src/hotspot/os/aix/os_aix.cpp
--- a/src/hotspot/os/aix/os_aix.cpp     Tue Sep 01 21:54:57 2020 +0200
+++ b/src/hotspot/os/aix/os_aix.cpp     Tue Sep 01 22:05:45 2020 +0200
@@ -1563,61 +1563,16 @@
   os::signal_notify(sig);
}

-void* os::user_handler() {
-  return CAST_FROM_FN_PTR(void*, UserHandler);
-}
-
extern "C" {
   typedef void (*sa_handler_t)(int);
   typedef void (*sa_sigaction_t)(int, siginfo_t *, void *);
}

-void* os::signal(int signal_number, void* handler) {
-  struct sigaction sigAct, oldSigAct;
-
-  sigfillset(&(sigAct.sa_mask));
-
-  // Do not block out synchronous signals in the signal handler.
-  // Blocking synchronous signals only makes sense if you can really
-  // be sure that those signals won't happen during signal handling,
-  // when the blocking applies. Normal signal handlers are lean and
-  // do not cause signals. But our signal handlers tend to be "risky"
-  // - secondary SIGSEGV, SIGILL, SIGBUS' may and do happen.
-  // On AIX, PASE there was a case where a SIGSEGV happened, followed
-  // by a SIGILL, which was blocked due to the signal mask. The process
-  // just hung forever. Better to crash from a secondary signal than to hang.
-  sigdelset(&(sigAct.sa_mask), SIGSEGV);
-  sigdelset(&(sigAct.sa_mask), SIGBUS);
-  sigdelset(&(sigAct.sa_mask), SIGILL);
-  sigdelset(&(sigAct.sa_mask), SIGFPE);
-  sigdelset(&(sigAct.sa_mask), SIGTRAP);
-
-  sigAct.sa_flags   = SA_RESTART|SA_SIGINFO;
-
-  sigAct.sa_handler = CAST_TO_FN_PTR(sa_handler_t, handler);
-
-  if (sigaction(signal_number, &sigAct, &oldSigAct)) {
-    // -1 means registration failed
-    return (void *)-1;
-  }
-
-  return CAST_FROM_FN_PTR(void*, oldSigAct.sa_handler);
-}
-
-void os::signal_raise(int signal_number) {
-  ::raise(signal_number);
-}
-
//
// The following code is moved from os.cpp for making this
// code platform specific, which it is by its very nature.
//

-// Will be modified when max signal is changed to be dynamic
-int os::sigexitnum_pd() {
-  return NSIG;
-}
-
// a counter for each possible signal value
static volatile jint pending_signals[NSIG+1] = { 0 };

@@ -1687,11 +1642,6 @@
   local_sem_init();
}

-void os::signal_notify(int sig) {
-  Atomic::inc(&pending_signals[sig]);
-  local_sem_post();
-}
-
static int check_pending_signals() {
   for (;;) {
     for (int i = 0; i < NSIG + 1; i++) {
@@ -1728,9 +1678,6 @@
   }
}

-int os::signal_wait() {
-  return check_pending_signals();
-}

////////////////////////////////////////////////////////////////////////////////
// Virtual Memory
diff -r ec42084221a6 src/hotspot/os/posix/signals_posix.cpp
--- a/src/hotspot/os/posix/signals_posix.cpp    Tue Sep 01 21:54:57 2020 +0200
+++ b/src/hotspot/os/posix/signals_posix.cpp    Tue Sep 01 22:05:45 2020 +0200
@@ -95,12 +95,7 @@
#endif

// sun.misc.Signal support
-#if !defined(AIX)
-  static Semaphore* sig_semaphore = NULL;
-#else
-  static sem_t sig_semaphore;
-  static msemaphore* p_sig_msem = 0;
-#endif
+static Semaphore* sig_semaphore = NULL;
// a counter for each possible signal value
static volatile jint pending_signals[NSIG+1] = { 0 };

@@ -272,71 +267,17 @@
// sun.misc.Signal support

static void local_sem_init() {
-#if !defined(AIX)
   sig_semaphore = new Semaphore();
-#else
-  if (os::Aix::on_aix()) {
-    int rc = ::sem_init(&sig_semaphore, 0, 0);
-    guarantee(rc != -1, "sem_init failed");
-  } else {
-    // Memory semaphores must live in shared mem.
-    guarantee0(p_sig_msem == NULL);
-    p_sig_msem = (msemaphore*)os::reserve_memory(sizeof(msemaphore), NULL);
-    guarantee(p_sig_msem, "Cannot allocate memory for memory semaphore");
-    guarantee(::msem_init(p_sig_msem, 0) == p_sig_msem, "msem_init failed");
-  }
-#endif
}

// Wrapper functions for: sem_init(), sem_post(), sem_wait()
-// On AIX, we use sem_init(), sem_post(), sem_wait()
-// On Pase, we need to use msem_lock() and msem_unlock(), because Posix Semaphores
-// do not seem to work at all on PASE (unimplemented, will cause SIGILL).
-// Note that just using msem_.. APIs for both PASE and AIX is not an option either, as
-// on AIX, msem_..() calls are suspected of causing problems.

static void local_sem_post() {
-#if !defined(AIX)
   sig_semaphore->signal();
-#else
-  static bool warn_only_once = false;
-  if (os::Aix::on_aix()) {
-    int rc = ::sem_post(&sig_semaphore);
-    if (rc == -1 && !warn_only_once) {
-      trcVerbose("sem_post failed (errno = %d, %s)", errno, os::errno_name(errno));
-      warn_only_once = true;
-    }
-  } else {
-    guarantee0(p_sig_msem != NULL);
-    int rc = ::msem_unlock(p_sig_msem, 0);
-    if (rc == -1 && !warn_only_once) {
-      trcVerbose("msem_unlock failed (errno = %d, %s)", errno, os::errno_name(errno));
-      warn_only_once = true;
-    }
-  }
-  #endif
}

static void local_sem_wait() {
-#if !defined(AIX)
   sig_semaphore->wait();
-#else
-  static bool warn_only_once = false;
-  if (os::Aix::on_aix()) {
-    int rc = ::sem_wait(&sig_semaphore);
-    if (rc == -1 && !warn_only_once) {
-      trcVerbose("sem_wait failed (errno = %d, %s)", errno, os::errno_name(errno));
-      warn_only_once = true;
-    }
-  } else {
-    guarantee0(p_sig_msem != NULL); // must init before use
-    int rc = ::msem_lock(p_sig_msem, 0);
-    if (rc == -1 && !warn_only_once) {
-      trcVerbose("msem_lock failed (errno = %d, %s)", errno, os::errno_name(errno));
-      warn_only_once = true;
-    }
-  }
-  #endif
}

void PosixSignals::jdk_misc_signal_init() {
@@ -1573,7 +1514,6 @@
   os::SuspendResume::State current = osthread->sr.state();

// TODO: reconcile the differences betrween Linux/BSD vs AIX here!
-#if !defined(AIX)
   if (current == os::SuspendResume::SR_SUSPEND_REQUEST) {
     suspend_save_context(osthread, siginfo, context);

@@ -1617,45 +1557,6 @@
   } else {
     // ignore
   }
-#else
-  if (current == os::SuspendResume::SR_SUSPEND_REQUEST) {
-    suspend_save_context(osthread, siginfo, context);
-
-    // attempt to switch the state, we assume we had a SUSPEND_REQUEST
-    os::SuspendResume::State state = osthread->sr.suspended();
-    if (state == os::SuspendResume::SR_SUSPENDED) {
-      sigset_t suspend_set;  // signals for sigsuspend()
-      sigemptyset(&suspend_set);
-
-      // get current set of blocked signals and unblock resume signal
-      pthread_sigmask(SIG_BLOCK, NULL, &suspend_set);
-      sigdelset(&suspend_set, SR_signum);
-
-      // wait here until we are resumed
-      while (1) {
-        sigsuspend(&suspend_set);
-
-        os::SuspendResume::State result = osthread->sr.running();
-        if (result == os::SuspendResume::SR_RUNNING) {
-          break;
-        }
-      }
-
-    } else if (state == os::SuspendResume::SR_RUNNING) {
-      // request was cancelled, continue
-    } else {
-      ShouldNotReachHere();
-    }
-
-    resume_clear_context(osthread);
-  } else if (current == os::SuspendResume::SR_RUNNING) {
-    // request was cancelled, continue
-  } else if (current == os::SuspendResume::SR_WAKEUP_REQUEST) {
-    // ignore
-  } else {
-    ShouldNotReachHere();
-  }
-#endif

   errno = old_errno;
}
@@ -1721,7 +1622,6 @@
   }

// TODO: reconcile the differences betrween Linux/BSD vs AIX here!
-#if !defined(AIX)
   if (sr_notify(osthread) != 0) {
     ShouldNotReachHere();
   }
@@ -1745,44 +1645,6 @@
       }
     }
   }
-#else
-  if (sr_notify(osthread) != 0) {
-    // try to cancel, switch to running
-
-    os::SuspendResume::State result = osthread->sr.cancel_suspend();
-    if (result == os::SuspendResume::SR_RUNNING) {
-      // cancelled
-      return false;
-    } else if (result == os::SuspendResume::SR_SUSPENDED) {
-      // somehow managed to suspend
-      return true;
-    } else {
-      ShouldNotReachHere();
-      return false;
-    }
-  }
-
-  // managed to send the signal and switch to SUSPEND_REQUEST, now wait for SUSPENDED
-
-  for (int n = 0; !osthread->sr.is_suspended(); n++) {
-    for (int i = 0; i < RANDOMLY_LARGE_INTEGER2 && !osthread->sr.is_suspended(); i++) {
-      os::naked_yield();
-    }
-
-    // timeout, try to cancel the request
-    if (n >= RANDOMLY_LARGE_INTEGER) {
-      os::SuspendResume::State cancelled = osthread->sr.cancel_suspend();
-      if (cancelled == os::SuspendResume::SR_RUNNING) {
-        return false;
-      } else if (cancelled == os::SuspendResume::SR_SUSPENDED) {
-        return true;
-      } else {
-        ShouldNotReachHere();
-        return false;
-      }
-    }
-  }
-#endif

   guarantee(osthread->sr.is_suspended(), "Must be suspended");
   return true;
@@ -1799,7 +1661,6 @@
   }

// TODO: reconcile the differences betrween Linux/BSD vs AIX here!
-#if !defined(AIX)
   while (true) {
     if (sr_notify(osthread) == 0) {
       if (sr_semaphore.timedwait(2)) {
@@ -1811,19 +1672,6 @@
       ShouldNotReachHere();
     }
   }
-#else
-  while (!osthread->sr.is_running()) {
-    if (sr_notify(osthread) == 0) {
-      for (int n = 0; n < RANDOMLY_LARGE_INTEGER && !osthread->sr.is_running(); n++) {
-        for (int i = 0; i < 100 && !osthread->sr.is_running(); i++) {
-          os::naked_yield();
-        }
-      }
-    } else {
-      ShouldNotReachHere();
-    }
-  }
-#endif
-
+
   guarantee(osthread->sr.is_running(), "Must be running!");
}
diff -r ec42084221a6 src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
--- a/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp Tue Sep 01 21:54:57 2020 +0200
+++ b/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp Tue Sep 01 22:05:45 2020 +0200
@@ -51,6 +51,7 @@
#include "runtime/stubRoutines.hpp"
#include "runtime/thread.inline.hpp"
#include "runtime/timer.hpp"
+#include "signals_posix.hpp"
#include "utilities/events.hpp"
#include "utilities/vmError.hpp"
#ifdef COMPILER1
@@ -225,7 +226,7 @@

   JavaThread* thread = NULL;
   VMThread* vmthread = NULL;
-  if (os::Aix::signal_handlers_are_installed) {
+  if (PosixSignals::are_signal_handlers_installed()) {
     if (t != NULL) {
       if(t->is_Java_thread()) {
         thread = (JavaThread*)t;


From: gerard ziemski <gerard.ziemski at oracle.com>
Sent: Dienstag, 1. September 2020 18:36
To: Doerr, Martin <martin.doerr at sap.com>; Thomas Stüfe <thomas.stuefe at gmail.com>
Cc: hotspot-runtime-dev at openjdk.java.net; Schmidt, Lutz <lutz.schmidt at sap.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Stuefe, Thomas <thomas.stuefe at sap.com>
Subject: Re: Attention AIX developers - factoring out POSIX signal code (JDK-8252324)

Thank you Martin, Thomas for your feedback.
On 8/31/20 12:20 PM, Doerr, Martin wrote:
However, jdk/jdk doesn’t support as400 PASE, so I’d be fine with using Gerard’s new POSIX code and removing all AIX specific stuff which was built to support as400 PASE.
I believe the semaphore stuff works on AIX, so we don’t need “#if !defined(AIX)”, right?
Just to make sure I understand it correctly: we don't need AIX specific semaphore code (i.e. local_sem_init(), local_sem_post() and local_sem_wait()), so we can remove it and go back to using runtime Semaphore for BSD/Linux/AIX?

What other PASE specific code is currently in singlas_posix.cpp that should be removed? I see some extra signal unblocking code in os::signal() that is for AIX platform (with a comment that it applies to both AIX and PASE). That should stay in though, right?

Is this something you want me to do, or will you clean that up yourself as appropriate?

@Thomas, I assigned https://bugs.openjdk.java.net/browse/JDK-825253 to myself as per your suggestion.


cheers


More information about the hotspot-runtime-dev mailing list