Attention AIX developers - factoring out POSIX signal code (JDK-8252324)
Doerr, Martin
martin.doerr at sap.com
Mon Aug 31 17:20:10 UTC 2020
Hi Thomas,
I have found several checks for
if (os::Aix::on_aix())
in the new signals_posix.cpp.
That is code which checks if it’s running on regular AIX or on as400 PASE.
@Gerard:
Some background:
We had originally built the original AIX code in a way that we can run in on as400 PASE, too (which is an incomplete POSIX layer for a non-POSIX OS, Thomas knows it better).
We have avoided to link against certain libraries statically and only use limited features. The VM can check if it’s running on regular AIX instead of as400 PASE and it can link against additional libraries dynamically and use more features.
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?
Best regards,
Martin
From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Samstag, 29. August 2020 11:17
To: Doerr, Martin <martin.doerr at sap.com>
Cc: gerard ziemski <gerard.ziemski at oracle.com>; 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)
On Fri, Aug 28, 2020 at 11:23 PM Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi Gerard,
I just gave it a quick spin.
Fix for os_aix_ppc.cpp is trivial (see below).
signals_posix.cpp takes more effort. I can see that a lot of AIX specific code is for as400 PASE support which we never contributed to OpenJDK.
Maybe we can remove it and use the regular posix code instead.
@Thomas: What do you think?
I see that we have a patch in AIX for the synchronous-error-signals-during-signal-handling-issue which would make sense for all platforms. I opened issue https://bugs.openjdk.java.net/browse/JDK-8252533 to track that. We already deal with this (see https://bugs.openjdk.java.net/browse/JDK-8065895, one of my first contributions btw :) but that first fix leaves some windows open which should be closed. I see Gerard already reviewed the original patch :)
I'd propose to ignore this AIX-specific patch, just remove it for the time being and treat AIX like every other posix platform. Then, re-evaluate https://bugs.openjdk.java.net/browse/JDK-825253<https://bugs.openjdk.java.net/browse/JDK-8252533> after this cleanup as a separate issue.
@Gerard: If you want to take over https://bugs.openjdk.java.net/browse/JDK-825253<https://bugs.openjdk.java.net/browse/JDK-8252533> this would be fine by me.
Other than that I cannot find much AIX/pase specifics in the original code. @Martin: Have I missed something?
Cheers, Thomas
Link which works correctly (version below links to pre1):
http://cr.openjdk.java.net/~gziemski/8252324_pre2/
Best regards,
Martin
diff -r 39c430bf9bb3 src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp
--- a/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp Fri Aug 28 22:59:25 2020 +0200
+++ b/src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp Fri Aug 28 23:13:25 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 (os::signal_handlers_are_installed) {
if (t != NULL) {
if(t->is_Java_thread()) {
thread = (JavaThread*)t;
From: gerard ziemski <gerard.ziemski at oracle.com<mailto:gerard.ziemski at oracle.com>>
Sent: Freitag, 28. August 2020 22:28
To: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>; Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>
Cc: hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>; Schmidt, Lutz <lutz.schmidt at sap.com<mailto:lutz.schmidt at sap.com>>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>; Stuefe, Thomas <thomas.stuefe at sap.com<mailto:thomas.stuefe at sap.com>>
Subject: Re: Attention AIX developers - factoring out POSIX signal code (JDK-8252324)
hi Thomas,
Thank you for your offer to test the changes. I know that you are currently busy with your metaspace changes, but when you do you have some time, then I have made a preliminary changes for AIX here to test: http://cr.openjdk.java.net/~gziemski/8252324_pre2<http://cr.openjdk.java.net/~gziemski/8252324_pre1>
cheers
On 8/27/20 4:48 PM, Thomas Stüfe wrote:
Hi Gerard, Martin,
I like the direction this patch is going, this is definitely a good cleanup! Also thanks from me for not wanting to break AIX :)
Wrt to signals AIX should not be that different from other Posix platforms. Any differences probably stem from the fact that the AIX port had a long time brewing in our proprietary code base before we contributed it, with not much sync points with other ports after the initial fork. (Memory management in AIX is different for a number of reasons, but signal stuff should be pretty standard).
There is this small issue that we use SIGTRAP for implicit null checks (@Martin: is this still a thing?) but strictly speaking this is a ppc issue, not an AIX issue, so it hits Linux too (@Martin: there seem to be small diffs between how sigtrap is handled in AIX and Linux, see e.g. the use of UseSigTrap, is that by design?). But all that should do is to add another signal to handle/unblock.
I would say, give it your best shot and dry-code the changes. When in doubt, just ask. We will test your patch and correct if necessary.
---
Some more general remarks to your preliminary changes:
I would really like it if we could start documenting APIs. I am always astonished that this is so seldom done. Ironically, almost all existing API comments in os_posix.hpp were contributed by SAP :-)
Since you now move all APIs to a central location, and it's all fresh in your head, maybe this would be a good point to add comments to them, describing behavior and input/output params.
---
I quite liked the os::Posix namespace and think the signal functions are well placed there. But I see what you want, you want to separate the signal functions from the os namespace since that one requires including os.hpp? Unfortunately this also makes the diff more complicated since a lot of code just moved.
Side note, I wish we would start using real C++ namespaces, we could make os a namespace and hence split it over as many header files as we want. Maybe something for future cleanups.
---
ucontext_(s|g)et_pc() have not much to do with signals, even though they are only used for signal continuation currently. They seem misplaced in PosixSignals.
---
Thanks, Thomas
On Thu, Aug 27, 2020 at 10:33 PM Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi Gerard,
> I don’t want to straight break AIX port unexpectedly.
Thanks for that! We highly appreciate that you take care of it.
It’s not surprising that adapting the AIX code is difficult.
I’ve added Thomas to Cc. He’s surely the most experienced person for these AIX specific things.
Unfortunately, he’s currently very busy because he’s contributing his new metaspace implementation.
We have to discuss how we can help.
Best regards,
Martin
From: gerard ziemski <gerard.ziemski at oracle.com<mailto:gerard.ziemski at oracle.com>>
Sent: Donnerstag, 27. August 2020 19:19
To: hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>
Cc: Schmidt, Lutz <lutz.schmidt at sap.com<mailto:lutz.schmidt at sap.com>>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>>; Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>
Subject: Attention AIX developers - factoring out POSIX signal code (JDK-8252324)
hi all,
I’m hoping I have included the right people in this email, but everyone is welcome to provide feedback.
I have a code for JDK-8252324, which factors out the common POSIX signal related code out into common file, so that all platforms can benefit from fixes that apply to all POSIX platforms (like JDK-82514380)
The preliminary webrev can be found here: http://cr.openjdk.java.net/~gziemski/8252324_pre1
I tried to port the changes to AIX myself, but got bogged down by more "#ifdef AIX” than I was hoping for (possibly too many differences to factor out the code for AIX, but frankly I don't think so), so I stopped (though I probably was on the right track) - to me it looks like AIX has some extra fixes and differences in the signal code than BSD/Linux.
My question is: given my inability to test on AIX, what can I do to make it easier for AIX developers to adopt this fix? I will try porting my changes again, but in the meantime if anyone has any pointers or offer of other help, I’d appreciate it. I don’t want to straight break AIX port unexpectedly.
Reference: https://bugs.openjdk.java.net/browse/JDK-8252324
Some notes about the affected APIs:
--------------------------------------------------------------------------------------------------
The APIs which moved from os/bsd/os_bsd.cpp (similarly for linux) 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();
cheers
More information about the hotspot-runtime-dev
mailing list