8252324: Signal related code should be shared among POSIX platforms

[gerard-ziemski]

hi all,

Please review this change that refactors common POSIX code into a separate
file.

Currently there appears to be quite a bit of duplicated code among POSIX
platforms, which makes it difficult to apply single fix to the signal code.
With this fix, we will only need to touch single file for common POSIX
code fixes from now on.

---

The APIs which moved from os/bsd/os_bsd.cpp to to os/posix/PosixSignals.cpp:

////////////////////////////////////////////////////////////////////////////////
// signal support
void os::Bsd::signal_sets_init()
sigset_t* os::Bsd::unblocked_signals()
sigset_t* os::Bsd::vm_signals()
void os::Bsd::hotspot_sigmask(Thread* thread)
////////////////////////////////////////////////////////////////////////////////
// sun.misc.Signal support
static void UserHandler(int sig, void siginfo, void context)
void os::user_handler()
void os::signal(int signal_number, void* handler)
void os::signal_raise(int signal_number)
int os::sigexitnum_pd()
static void jdk_misc_signal_init()
void os::signal_notify(int sig)
static int check_pending_signals()
int os::signal_wait()
////////////////////////////////////////////////////////////////////////////////
// suspend/resume support
static void resume_clear_context(OSThread osthread)
static void suspend_save_context(OSThread osthread, siginfo_t siginfo, ucontext_t context)
static void SR_handler(int sig, siginfo_t* siginfo, ucontext_t* context)
static int SR_initialize()
static int sr_notify(OSThread* osthread)
static bool do_suspend(OSThread* osthread)
static void do_resume(OSThread* osthread)
///////////////////////////////////////////////////////////////////////////////////
// signal handling (except suspend/resume)
static void signalHandler(int sig, siginfo_t* info, void* uc)
struct sigaction* os::Bsd::get_chained_signal_action(int sig)
static bool call_chained_handler(struct sigaction actp, int sig,
siginfo_t siginfo, void context)
bool os::Bsd::chained_handler(int sig, siginfo_t siginfo, void context)
int os::Bsd::get_our_sigflags(int sig)
void os::Bsd::set_our_sigflags(int sig, int flags)
void os::Bsd::set_signal_handler(int sig, bool set_installed)
void os::Bsd::install_signal_handlers()
static const char get_signal_handler_name(address handler,
char* buf, int buflen)
static void print_signal_handler(outputStream* st, int sig,
char* buf, size_t buflen)
void os::run_periodic_checks()
void os::Bsd::check_signal_handler(int sig)

---

The APIs which moved from os/posix/os_posix.cpp to os/posix/PosixSignals.cpp:

const char* os::Posix::get_signal_name(int sig, char* out, size_t outlen)
int os::Posix::get_signal_number(const char* signal_name)
int os::get_signal_number(const char* signal_name)
bool os::Posix::is_valid_signal(int sig)
bool os::Posix::is_sig_ignored(int sig)
const char* os::exception_name(int sig, char* buf, size_t size)
const char* os::Posix::describe_signal_set_short(const sigset_t* set, char* buffer, size_t buf_size)
void os::Posix::print_signal_set_short(outputStream* st, const sigset_t* set)
const char* os::Posix::describe_sa_flags(int flags, char* buffer, size_t size)
oid os::Posix::print_sa_flags(outputStream* st, int flags)
static bool get_signal_code_description(const siginfo_t* si, enum_sigcode_desc_t* out)
void os::print_siginfo(outputStream* os, const void* si0)
bool os::signal_thread(Thread* thread, int sig, const char* reason)
int os::Posix::unblock_thread_signal_mask(const sigset_t set)
address os::Posix::ucontext_get_pc(const ucontext_t ctx)
void os::Posix::ucontext_set_pc(ucontext_t* ctx, address pc)
struct sigaction* os::Posix::get_preinstalled_handler(int sig)
void os::Posix::save_preinstalled_handler(int sig, struct sigaction& oldAct)

---

---

DETAILS:

---

Public APIs which are now internal static PosixSignals::

sigset_t* os::Bsd::vm_signals()
struct sigaction* os::Bsd::get_chained_signal_action(int sig)
int os::Bsd::get_our_sigflags(int sig)
void os::Bsd::set_our_sigflags(int sig, int flags)
void os::Bsd::set_signal_handler(int sig, bool set_installed)
void os::Bsd::check_signal_handler(int sig)
const char* os::Posix::get_signal_name(int sig, char* out, size_t outlen)
bool os::Posix::is_valid_signal(int sig)
const char* os::Posix::describe_signal_set_short(const sigset_t* set, char* buffer, size_t buf_size)
void os::Posix::print_signal_set_short(outputStream* st, const sigset_t* set)
const char* os::Posix::describe_sa_flags(int flags, char* buffer, size_t size)
oid os::Posix::print_sa_flags(outputStream* st, int flags)
static bool get_signal_code_description(const siginfo_t* si, enum_sigcode_desc_t* out)
void os::Posix::save_preinstalled_handler(int sig, struct sigaction& oldAct)

---

Public APIs which moved to public PosixSignals::

void os::Bsd::signal_sets_init()
void os::Bsd::hotspot_sigmask(Thread* thread)
bool os::Bsd::chained_handler(int sig, siginfo_t* siginfo, void* context)
void os::Bsd::install_signal_handlers()
bool os::Posix::is_sig_ignored(int sig)
int os::Posix::unblock_thread_signal_mask(const sigset_t set)
address os::Posix::ucontext_get_pc(const ucontext_t ctx)
void os::Posix::ucontext_set_pc(ucontext_t* ctx, address pc)

---

Internal APIs which are now public in PosixSignals::

static void jdk_misc_signal_init()
static int SR_initialize()
static bool do_suspend(OSThread* osthread)
static void do_resume(OSThread* osthread)
static void print_signal_handler(outputStream* st, int sig, char* buf, size_t buflen)

---

New APIs in PosixSignals::

static bool are_signal_handlers_installed();

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8252324: Signal related code should be shared among POSIX platforms

Reviewers

• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 1ce8353591177dcc8685409096e5027dd93b2458
• Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to c1c5addf85e96ef62d9394073c3d6e0d415053d8

Download

$ git fetch https://git.openjdk.java.net/jdk pull/157/head:pull/157
$ git checkout pull/157

[bridgekeeper]

👋 Welcome back gziemski! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@gerard-ziemski The following label will be automatically applied to this pull request: hotspot-runtime.

When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label (add|remove) "label" command.

[openjdk]

@gerard-ziemski the test group 8252324 does not exist

[openjdk]

@gerard-ziemski you need to get approval to run the tests in tier1 for commits up until 4bc34bc1

[mlbridge]

Webrevs

• 07: Full - Incremental (06b104d)
• 06: Full - Incremental (b50d7229dcda7db91cea8440f903a68b1b74fce2)
• 05: Full (4f5d4cd49436b64dc3770e4ba1f811a76b5a09e7)
• 04: Full (c1c5addf85e96ef62d9394073c3d6e0d415053d8)
• 03: Full - Incremental (1ce8353591177dcc8685409096e5027dd93b2458)
• 02: Full - Incremental (cc13700d7d3f15927e22d92d9f5ec9a0739ef9a1)
• 01: Full - Incremental (f3e3dd85e718970002bf2252a2f2ff123020853a)
• 00: Full (4bc34bc194bf2c0ad222db8ad1bc378b65c4e0ec)

[mlbridge]

Mailing list message from Doerr, Martin on hotspot-runtime-dev:

Hi Gerard,

sorry for the long delay. It took time to get our nightly tests working again on AIX.
I have seen an issue, but it may be unrelated to your change. We'll retest it.

Note that there's still unused code left in os_aix.cpp (see below).
Thanks again for taking care of AIX. I appreciate having more shared POSIX code.

Best regards,
Martin

diff --git a/src/hotspot/os/aix/os_aix.cpp b/src/hotspot/os/aix/os_aix.cpp
index 02568cf..eb84217 100644
--- a/src/hotspot/os/aix/os_aix.cpp
+++ b/src/hotspot/os/aix/os_aix.cpp
@@ -1550,134 +1550,6 @@ void os::print_jni_name_suffix_on(outputStream* st, int args_size) {
// no suffix required
}

-////////////////////////////////////////////////////////////////////////////////
-// sun.misc.Signal support
-
-static void
-UserHandler(int sig, void *siginfo, void *context) {
- // Ctrl-C is pressed during error reporting, likely because the error
- // handler fails to abort. Let VM die immediately.
- if (sig == SIGINT && VMError::is_error_reported()) {
- os::die();
- }
-
- os::signal_notify(sig);
-}
-
-extern "C" {
- typedef void (*sa_handler_t)(int);
- typedef void (*sa_sigaction_t)(int, siginfo_t *, void *);
-}
-
-//
-// The following code is moved from os.cpp for making this
-// code platform specific, which it is by its very nature.
-//
-
-// a counter for each possible signal value
-static volatile jint pending_signals[NSIG+1] = { 0 };
-
-// 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 sem_t sig_sem;
-static msemaphore* p_sig_msem = 0;
-
-static void local_sem_init() {
- if (os::Aix::on_aix()) {
- int rc = ::sem_init(&sig_sem, 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");
- }
-}
-
-static void local_sem_post() {
- static bool warn_only_once = false;
- if (os::Aix::on_aix()) {
- int rc = ::sem_post(&sig_sem);
- 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;
- }
- }
-}
-
-static void local_sem_wait() {
- static bool warn_only_once = false;
- if (os::Aix::on_aix()) {
- int rc = ::sem_wait(&sig_sem);
- 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;
- }
- }
-}
-
-static void jdk_misc_signal_init() {
- // Initialize signal structures
- ::memset((void*)pending_signals, 0, sizeof(pending_signals));
-
- // Initialize signal semaphore
- local_sem_init();
-}
-
-static int check_pending_signals() {
- for (;;) {
- for (int i = 0; i < NSIG + 1; i++) {
- jint n = pending_signals[i];
- if (n > 0 && n == Atomic::cmpxchg(&pending_signals[i], n, n - 1)) {
- return i;
- }
- }
- JavaThread *thread = JavaThread::current();
- ThreadBlockInVM tbivm(thread);
-
- bool threadIsSuspended;
- do {
- thread->set_suspend_equivalent();
- // cleared by handle_special_suspend_equivalent_condition() or java_suspend_self()
-
- local_sem_wait();
-
- // were we externally suspended while we were waiting?
- threadIsSuspended = thread->handle_special_suspend_equivalent_condition();
- if (threadIsSuspended) {
- //
- // The semaphore has been incremented, but while we were waiting
- // another thread suspended us. We don't want to continue running
- // while suspended because that would surprise the thread that
- // suspended us.
- //
-
- local_sem_post();
-
- thread->java_suspend_self();
- }
- } while (threadIsSuspended);
- }
-}

////////////////////////////////////////////////////////////////////////////////
// Virtual Memory

[mlbridge]

Mailing list message from Doerr, Martin on hotspot-runtime-dev:

Sorry, Gerard's email address was wrong.

-----Original Message-----
From: Doerr, Martin
Sent: Mittwoch, 23. September 2020 11:40
To: Gerard Ziemski <gziemski at openjdk.java.net>; hotspot-runtime-
dev at openjdk.java.net
Cc: Stuefe, Thomas <thomas.stuefe at sap.com>
Subject: RE: RFR: 8252324: Signal related code should be shared among POSIX
platforms

Hi Gerard,

sorry for the long delay. It took time to get our nightly tests working again on
AIX.
I have seen an issue, but it may be unrelated to your change. We'll retest it.

Note that there's still unused code left in os_aix.cpp (see below).
Thanks again for taking care of AIX. I appreciate having more shared POSIX
code.

Best regards,
Martin

diff --git a/src/hotspot/os/aix/os_aix.cpp b/src/hotspot/os/aix/os_aix.cpp
index 02568cf..eb84217 100644
--- a/src/hotspot/os/aix/os_aix.cpp
+++ b/src/hotspot/os/aix/os_aix.cpp
@@ -1550,134 +1550,6 @@ void
os::print_jni_name_suffix_on(outputStream* st, int args_size) {
// no suffix required
}

-
//////////////////////////////////////////////////////////////////////////////
//
-// sun.misc.Signal support
-
-static void
-UserHandler(int sig, void *siginfo, void *context) {
- // Ctrl-C is pressed during error reporting, likely because the error
- // handler fails to abort. Let VM die immediately.
- if (sig == SIGINT && VMError::is_error_reported()) {
- os::die();
- }
-
- os::signal_notify(sig);
-}
-
-extern "C" {
- typedef void (*sa_handler_t)(int);
- typedef void (*sa_sigaction_t)(int, siginfo_t *, void *);
-}
-
-//
-// The following code is moved from os.cpp for making this
-// code platform specific, which it is by its very nature.
-//
-
-// a counter for each possible signal value
-static volatile jint pending_signals[NSIG+1] = { 0 };
-
-// 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 sem_t sig_sem;
-static msemaphore* p_sig_msem = 0;
-
-static void local_sem_init() {
- if (os::Aix::on_aix()) {
- int rc = ::sem_init(&sig_sem, 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");
- }
-}
-
-static void local_sem_post() {
- static bool warn_only_once = false;
- if (os::Aix::on_aix()) {
- int rc = ::sem_post(&sig_sem);
- 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;
- }
- }
-}
-
-static void local_sem_wait() {
- static bool warn_only_once = false;
- if (os::Aix::on_aix()) {
- int rc = ::sem_wait(&sig_sem);
- 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;
- }
- }
-}
-
-static void jdk_misc_signal_init() {
- // Initialize signal structures
- ::memset((void*)pending_signals, 0, sizeof(pending_signals));
-
- // Initialize signal semaphore
- local_sem_init();
-}
-
-static int check_pending_signals() {
- for (;;) {
- for (int i = 0; i < NSIG + 1; i++) {
- jint n = pending_signals[i];
- if (n > 0 && n == Atomic::cmpxchg(&pending_signals[i], n, n - 1)) {
- return i;
- }
- }
- JavaThread *thread = JavaThread::current();
- ThreadBlockInVM tbivm(thread);
-
- bool threadIsSuspended;
- do {
- thread->set_suspend_equivalent();
- // cleared by handle_special_suspend_equivalent_condition() or
java_suspend_self()
-
- local_sem_wait();
-
- // were we externally suspended while we were waiting?
- threadIsSuspended = thread-

handle_special_suspend_equivalent_condition();
- if (threadIsSuspended) {
- //
- // The semaphore has been incremented, but while we were waiting
- // another thread suspended us. We don't want to continue running
- // while suspended because that would surprise the thread that
- // suspended us.
- //
-
- local_sem_post();
-
- thread->java_suspend_self();
- }
- } while (threadIsSuspended);
- }
-}

//////////////////////////////////////////////////////////////////////////////
//
// Virtual Memory

-----Original Message-----
From: hotspot-runtime-dev <hotspot-runtime-dev-
retn at openjdk.java.net>
On Behalf Of Gerard Ziemski
Sent: Mittwoch, 16. September 2020 18:00
To: hotspot-runtime-dev at openjdk.java.net
Subject: RFR: 8252324: Signal related code should be shared among POSIX
platforms

hi all,

Please review this change that refactors common POSIX code into a
separate
file.

Currently there appears to be quite a bit of duplicated code among POSIX
platforms, which makes it difficult to apply single fix to the signal code.
With this fix, we will only need to touch single file for common POSIX
code fixes from now on.

----------------------------------------------------------------------------
The APIs which moved from os/bsd/os_bsd.cpp to to
os/posix/PosixSignals.cpp:

//////////////////////////////////////////////////////////////////////////////

//
// signal support
void os::Bsd::signal_sets_init()
sigset_t* os::Bsd::unblocked_signals()
sigset_t* os::Bsd::vm_signals()
void os::Bsd::hotspot_sigmask(Thread* thread)

//////////////////////////////////////////////////////////////////////////////

//
// sun.misc.Signal support
static void UserHandler(int sig, void *siginfo, void *context)
void* os::user_handler()
void* os::signal(int signal_number, void* handler)
void os::signal_raise(int signal_number)
int os::sigexitnum_pd()
static void jdk_misc_signal_init()
void os::signal_notify(int sig)
static int check_pending_signals()
int os::signal_wait()

//////////////////////////////////////////////////////////////////////////////

//
// suspend/resume support
static void resume_clear_context(OSThread *osthread)
static void suspend_save_context(OSThread *osthread, siginfo_t* siginfo,
ucontext_t* context)
static void SR_handler(int sig, siginfo_t* siginfo, ucontext_t* context)
static int SR_initialize()
static int sr_notify(OSThread* osthread)
static bool do_suspend(OSThread* osthread)
static void do_resume(OSThread* osthread)

//////////////////////////////////////////////////////////////////////////////

/////
// signal handling (except suspend/resume)
static void signalHandler(int sig, siginfo_t* info, void* uc)
struct sigaction* os::Bsd::get_chained_signal_action(int sig)
static bool call_chained_handler(struct sigaction *actp, int sig,
siginfo_t *siginfo, void *context)
bool os::Bsd::chained_handler(int sig, siginfo_t* siginfo, void* context)
int os::Bsd::get_our_sigflags(int sig)
void os::Bsd::set_our_sigflags(int sig, int flags)
void os::Bsd::set_signal_handler(int sig, bool set_installed)
void os::Bsd::install_signal_handlers()
static const char* get_signal_handler_name(address handler,
char* buf, int buflen)
static void print_signal_handler(outputStream* st, int sig,
char* buf, size_t buflen)
void os::run_periodic_checks()
void os::Bsd::check_signal_handler(int sig)

-----------------------------------------------------------------------------
The APIs which moved from os/posix/os_posix.cpp to
os/posix/PosixSignals.cpp:

const char* os::Posix::get_signal_name(int sig, char* out, size_t outlen)
int os::Posix::get_signal_number(const char* signal_name)
int os::get_signal_number(const char* signal_name)
bool os::Posix::is_valid_signal(int sig)
bool os::Posix::is_sig_ignored(int sig)
const char* os::exception_name(int sig, char* buf, size_t size)
const char* os::Posix::describe_signal_set_short(const sigset_t* set, char*
buffer, size_t buf_size)
void os::Posix::print_signal_set_short(outputStream* st, const sigset_t*
set)
const char* os::Posix::describe_sa_flags(int flags, char* buffer, size_t size)
oid os::Posix::print_sa_flags(outputStream* st, int flags)
static bool get_signal_code_description(const siginfo_t* si,
enum_sigcode_desc_t* out)
void os::print_siginfo(outputStream* os, const void* si0)
bool os::signal_thread(Thread* thread, int sig, const char* reason)
int os::Posix::unblock_thread_signal_mask(const sigset_t *set)
address os::Posix::ucontext_get_pc(const ucontext_t* ctx)
void os::Posix::ucontext_set_pc(ucontext_t* ctx, address pc)
struct sigaction* os::Posix::get_preinstalled_handler(int sig)
void os::Posix::save_preinstalled_handler(int sig, struct sigaction& oldAct)

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

DETAILS:

--------------------------------------------------------
Public APIs which are now internal static PosixSignals::

sigset_t* os::Bsd::vm_signals()
struct sigaction* os::Bsd::get_chained_signal_action(int sig)
int os::Bsd::get_our_sigflags(int sig)
void os::Bsd::set_our_sigflags(int sig, int flags)
void os::Bsd::set_signal_handler(int sig, bool set_installed)
void os::Bsd::check_signal_handler(int sig)
const char* os::Posix::get_signal_name(int sig, char* out, size_t outlen)
bool os::Posix::is_valid_signal(int sig)
const char* os::Posix::describe_signal_set_short(const sigset_t* set, char*
buffer, size_t buf_size)
void os::Posix::print_signal_set_short(outputStream* st, const sigset_t*
set)

[gerard-ziemski]

Mailing list message from Doerr, Martin on hotspot-runtime-dev:

Hi Gerard,

sorry for the long delay. It took time to get our nightly tests working again on AIX.
I have seen an issue, but it may be unrelated to your change. We'll retest it.

I will take another look at AIX changes to see if I missed something.

Note that there's still unused code left in os_aix.cpp (see below).
Thanks again for taking care of AIX. I appreciate having more shared POSIX code.

Thank you for the diff, I'll use it and update the webrev.

[gerard-ziemski]

Mailing list message from Doerr, Martin on hotspot-runtime-dev:
Hi Gerard,
sorry for the long delay. It took time to get our nightly tests working again on AIX.
I have seen an issue, but it may be unrelated to your change. We'll retest it.

I will take another look at AIX changes to see if I missed something.

I took another look at the changes and I have concerns over the following methods:

PosixSignals::SR_handler
PosixSignals::do_suspend
PosixSignals::do_resume

They seem to differ substantially in parts from the other POSIX platforms. In my early webrevs I have accounted for those changes using "#if defined(AIX)", but during pre-reviews you asked me to revert to the common POSIX code.

Can you please take a look again and tell me if you are OK with the following SA related changes:

• using POSIX semaphores
• using the common POSIX code

I will upload, what I think (based only on the code diffs between the platforms as I lack AIX platform understanding) the AIX platform needs here.

[TheRealMDoerr]

I believe the reason for not using POSIX semaphores was that it is not supported on as400 PASE which we don't support in OpenJDK. I'm not aware of any problems when using this common POSIX code on AIX 7.2.
Is SA supported on AIX? That would be new to me.
But I'm not an expert for these topics. I hope that Thomas can find some time to take a look.

[mlbridge]

Mailing list message from Thomas St��fe on hotspot-runtime-dev:

Hi Gerard,

first off, have said it already, but great work! This is a really good
cleanup.

About your last commit (
https://github.com//pull/157/commits/cc13700d7d3f15927e22d92d9f5ec9a0739ef9a1
Add AIX specific SA code): I think you can drop that again. The version
beforehand looks good to me. I did some archeology and all this coding was
forked from Linux and introduced by Sun for JEP167:

https://bugs.openjdk.java.net/browse/JDK-8005849

It introduces also those "RANDOMLY_.." variables and uses them in Linux and
BSD coding. We just forked that for AIX. Later that coding was rewritten in
Linux, but the changes were not ported to AIX, hence the diff.

--

Weirdly enough, that exact version for JDK-8005849 I cannot even find in
the current jdk git or mercurial depots. I see jep167 introduced in
mercurial with:

changeset: 18025:b7bcf7497f93
user: sla
date: Mon Jun 10 11:30:51 2013 +0200
summary: 8005849: JEP 167: Event-Based JVM Tracing

But that one seems to be already the later version, not the initial one I
see at https://bugs.openjdk.java.net/browse/JDK-8005849. Who knows, the
history may be lost in the course of the hg forest consolidation. Maybe
Steffan Larson may know.

--

Anyway, I think all that coding is not needed for AIX. The fact that it
exists is only an effect of AIX missing code after the initial fork.

--https://bugs.openjdk.java.net/browse/JDK-8005849

Also, if you feel like it, you also can remove the remaining unused
definitions of "RANDOMLY_..." in os_linux.cpp and os_bsd.cpp.

--

I wish you had done the move to a new file (os_posix -> signals_posix) in a
separate change, that would have made the review easier. But alas, it is
what it is. I eyed the changes and they seem to be okay. A couple of places
have #ifdef AIX still but we can go through them later. Also, you still
have JDK-8252533 outstanding which will do away with some of the AIX
specific sections.

For all I see this seems fine.

I think roll back the last commit and let us test the final version for a
night or two in our systems, and then this is good to go.

Thanks, Thomas

On Wed, Sep 23, 2020 at 9:17 PM Gerard Ziemski <gziemski at openjdk.java.net>
wrote:

[gerard-ziemski]

Reverted the last commit. Thank you for taking a look and testing - please let me know how it goes.

I have already tested this change here at Oracle a while ago, but I will do so once again...

[gerard-ziemski]

Mach5 hs-tier1,2,3,4,5,6,7 testing looks good.

[dholmes-ora]

Hi Gerard,
Thank you for tackling this long overdue cleanup and conciliation of the the signal management code! Great work on a tedious task.

Overall this looks okay but I confess it is very hard to compare each previous platform version with the new shared version. I'm glad the AIX folk have take an indepth look there.

I have a few minor comments in specific files below.

I would also suggest naming the file posixSignal.cpp/hpp as that is the more common naming pattern.

Future cleanup: do we really need JVM_handle_<os>_signal to be cpu specific? I can imagine one copy of this with a few ifdefs or newly introduced os::pd_* functions.

Thanks,
David

[openjdk]

@gerard-ziemski this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8252324
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[TheRealMDoerr]

Tests on AIX look good so far. Maybe we should re-check after conflict resolution, but basically, I think it can get pushed when reviews are done.

[coleenp]

Future cleanup: do we really need JVM_handle_signal to be cpu specific? I can imagine one copy of this with a few ifdefs or newly introduced os::pd* functions.
The small bit of code that I looked at last week defied refactoring unfortunately, but yes, we should try to do this.

[coleenp]

Gerard, thank you for doing this! Thank you to Martin and Thomas for looking at the AIX code.

[gerard-ziemski]

Gerard, thank you for doing this! Thank you to Martin and Thomas for looking at the AIX code.

Thank you for the review!

[gerard-ziemski]

Hi Gerard,
Thank you for tackling this long overdue cleanup and conciliation of the the signal management code! Great work on a tedious task.

Overall this looks okay but I confess it is very hard to compare each previous platform version with the new shared version.

Sorry about that, I do understand how hard it was to review it and really appreciate it!

I'm glad the AIX folk have take an indepth look there.

Me too!

I have a few minor comments in specific files below.

I would also suggest naming the file posixSignal.cpp/hpp as that is the more common naming pattern.

I based the name on the other files in that directory, i.e.:

jvm_posix.cpp
os_posix.cpp
semaphore_posix.cpp
threadLocalStorage_posix.cpp
vmError_posix.cpp

so I introduced:

signals_posix.cpp

Future cleanup: do we really need JVM_handle_signal to be cpu specific? I can imagine one copy of this with a few ifdefs or newly introduced os::pd* functions.

Thanks,
David

Thank you David very much for catching the merge issues and review!

[gerard-ziemski]

I will merge again with current, review the changes again looking for weird merge issue, push here, then re-test...

[gerard-ziemski]

Darn, I closed it by mistake (I thought I was closing a comment), reopening.

[openjdk]

@gerard-ziemski This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for more details.

After integration, the commit message for the final commit will be:

8252324: Signal related code should be shared among POSIX platforms

Reviewed-by: coleenp, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 17 new commits pushed to the master branch:

• 7778047: 8253736: Cleanup some of WorkArounds and usage thereof
• 87d77eb: 8253879: Simplify redundant code in IndexBuilder
• dffe9db: 8252537: Updated @exception with @throws
• 406db1c: 8253913: unify gtest test names
• ea5a2b1: 8251464: make Node::dump(int depth) support indent
• fff8c8d: 8253882: remove PropertyResolvingWrapper
• 7dcdc1f: 8253865: Pre-submit testing using GitHub Actions does not detect failures reliably
• 0fd41c3: 8253470: Javadoc clean up in Filter and Headers
• f686a38: 8253926: Use extra_size correctly in anon_mmap_aligned
• b8966e1: 8253928: G1: Remove G1ConcurrentMarkThread::set_in_progress declaration
• ... and 7 more: https://git.openjdk.java.net/jdk/compare/60ec2a53c2e545cc4f151ac02c38180a76a729aa...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[dholmes-ora]

I would also suggest naming the file posixSignal.cpp/hpp as that is the more common naming pattern.

I based the name on the other files in that directory, i.e.:

jvm_posix.cpp

Yes my bad. I was thinking of the class->file naming convention, but in this directory we're following the foo_<os> convention.

[tstuefe]

Gerard, this is a great cleanup. I am fine with the changes.

This will make maintenance of ports easier. As we see right in this patch, platform ports can diverge over time, especially if they are closed ports which only exist downstream (as the AIX port did for a long time at SAP).

.. Thomas

[gerard-ziemski]

Gerard, this is a great cleanup. I am fine with the changes.

This will make maintenance of ports easier. As we see right in this patch, platform ports can diverge over time, especially if they are closed ports which only exist downstream (as the AIX port did for a long time at SAP).

.. Thomas

Thank you Martin for the review!

[gerard-ziemski]

Sorry for taking so long, but this is my 1st github pr and I did not liked the merge issues that David discovered and tried to redo the merge, but I keep getting it wrong.

I just forced pushed update removing the BIG merge and I keep working on it, hopefully without introducing another huge merge. If anyone has tips on how to apply local changes (i.e. rebase/stash?) without having to merge, please let me know.

Once I figure this out, I will re-implement the few, non-merge, issues that David discovered and this will be ready for integration.

[dholmes-ora]

@gerard-ziemski Please do not force-push any commits in an open PR as it breaks the commit history and the chain of review comments. I will not be able to see the merge issues actually fixed as a diff from the previous commit.

[openjdk]

⚠️ @gerard-ziemski This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[gerard-ziemski]

merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Can anyone explain what this means and whether it's OK to go ahead with this pr, or should I close this one (since I made quite a mess of it) and start a new one?

[dholmes-ora]

Hi Gerard,

I'm afraid something is still very wrong here. I see you fixing the original merge issues in:

b33d292e96a559b8a3a998641411f61bf087738f (Address David's review issues )

but then the "Fix Merge" commit

4f5d4cd49436b64dc3770e4ba1f811a76b5a09e7

applied after that seems to have reversed them again!

I think it might be necessary to close this PR and create a new one.

Thanks.

[mlbridge]

Mailing list message from Thomas St��fe on hotspot-runtime-dev:

Can anyone explain what this means and whether it's OK to go ahead with
this pr, or should I close this one (since I
made quite a mess of it) and start a new one?

Hi Gerard,

I'm afraid something is still very wrong here. I see you fixing the
original merge issues in:

b33d292e96a559b8a3a998641411f61bf087738f (Address David's review issues )

but then the "Fix Merge" commit

4f5d4cd49436b64dc3770e4ba1f811a76b5a09e7

applied after that seems to have reversed them again!

I think it might be necessary to close this PR and create a new one.

I agree. Also, then you get the new Activity-based builds and tier0 tests
for free since that submit.yml file is now checked in head :)

Cheers, Thomas

Thanks.

[gerard-ziemski]

I'm considering this pr to be "lost cause", so I just "force pushed" to a known good state, wiping out the bad merge that backed out my fixes based on David's comments, so that I have a snapshot of what I want to become the basis of new pr.

Just thought I would explain what I'm trying to do before anyone points out to me that I should not be using "push --force".

[gerard-ziemski]

Mailing list message from Thomas St��fe on hotspot-runtime-dev:

Can anyone explain what this means and whether it's OK to go ahead with
this pr, or should I close this one (since I
made quite a mess of it) and start a new one?

Hi Gerard,
I'm afraid something is still very wrong here. I see you fixing the
original merge issues in:
b33d292 (Address David's review issues )
but then the "Fix Merge" commit
4f5d4cd
applied after that seems to have reversed them again!
I think it might be necessary to close this PR and create a new one.

I agree. Also, then you get the new Activity-based builds and tier0 tests
for free since that submit.yml file is now checked in head :)

Cheers, Thomas

Thanks.

I think I recovered this pr to the point where we could continue with reviews (all changes are in and it passes Mach5 hs-tier1,2,3,4,5 testing), but I do like the idea of Activity-based builds and tier0 tests with new pr, so I'm going to close this one, and open a new one shortly...

Thank you Thomas and David for assuring me that the huge merges eventually get resolved and disappear automatically -that was the reason I started tweaking this pr and why I got it messed up.

[gerard-ziemski]

The new pr for this issue is #497