Attention AIX developers - factoring out POSIX signal code (JDK-8252324)
gerard ziemski
gerard.ziemski at oracle.com
Fri Aug 28 20:27:38 UTC 2020
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