Attention AIX developers - factoring out POSIX signal code (JDK-8252324)
Doerr, Martin
martin.doerr at sap.com
Fri Aug 28 21:23:07 UTC 2020
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?
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>
Sent: Freitag, 28. August 2020 22:28
To: Thomas Stüfe <thomas.stuefe at gmail.com>; Doerr, Martin <martin.doerr at sap.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)
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