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

Sat Aug 29 09:16:44 UTC 2020

On Fri, Aug 28, 2020 at 11:23 PM Doerr, Martin <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>
> *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>
> 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
>
>
>