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