Attention AIX developers - factoring out POSIX signal code (JDK-8252324)
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Aug 27 21:48:59 UTC 2020
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> 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>
> Sent: Donnerstag, 27. August 2020 19:19
> To: hotspot-runtime-dev at openjdk.java.net
> Cc: Schmidt, Lutz <lutz.schmidt at sap.com>; Lindenmaier, Goetz <
> goetz.lindenmaier at sap.com>; Doerr, Martin <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