RFR: 8252324: Signal related code should be shared among POSIX platforms [v4]

Mon Sep 28 04:42:29 UTC 2020

On Thu, 24 Sep 2020 15:04:01 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> hi all,
>> 
>> Please review this change that refactors common POSIX code into a separate
>> file.
>> 
>> Currently there appears to be quite a bit of duplicated code among POSIX
>> platforms, which makes it difficult to apply single fix to the signal code.
>> With this fix, we will only need to touch single file for common POSIX
>> code fixes from now on.
>> 
>> ----------------------------------------------------------------------------
>> The APIs which moved from os/bsd/os_bsd.cpp 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();
>
> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert "Add AIX specific SA code"
>   
>   This reverts commit cc13700d7d3f15927e22d92d9f5ec9a0739ef9a1.

Hi Gerard,
Thank you for tackling this long overdue cleanup and conciliation of the the signal management code! Great work on a
tedious task.

Overall this looks okay but I confess it is very hard to compare each previous platform version with the new shared
version. I'm glad the AIX folk have take an indepth look there.

I have a few minor comments in specific files below.

I would also suggest naming the file posixSignal.cpp/hpp as that is the more common naming pattern.

Future cleanup: do we really need JVM_handle_<os>_signal to be cpu specific? I can imagine one copy of this with a few
ifdefs or newly introduced os::pd_* functions.

Thanks,
David

src/hotspot/os/linux/os_linux.cpp line 944:

> 942:     // is no gap between the last two virtual memory regions.
> 943:
> 944:     JavaThread *jt = (JavaThread *)thread;

thread is already a JavaThread* - see line 903

src/hotspot/os/linux/os_linux.cpp line 1685:

> 1683:                 filename);
> 1684:
> 1685:         assert(Thread::current()->is_Java_thread(), "must be Java thread");

This assertion is already inside JavaThread::current().

src/hotspot/os/posix/os_posix.cpp line 1456:

> 1454:   Thread* thread = Thread::current();
> 1455:   assert(thread->is_Java_thread(), "Must be JavaThread");
> 1456:   JavaThread *jt = (JavaThread *)thread;

This change is unnecessary. Please restore the original line JavaThread::current() call.

src/hotspot/os/posix/os_posix.cpp line 1501:

> 1499:   }
> 1500:
> 1501:   OSThreadWaitState osts(thread->osthread(), false /* not Object.wait() */);

Unnecessary change - just use jt

src/hotspot/os/posix/signals_posix.cpp line 46:

> 44: // suspend/resume
> 45:
> 46: // glibc on Bsd platform uses non-documented flag

This was added for Linux and then copied to the BSD port, so the comment is inaccurate. This seems to be referring to
the SA_RESTORER flag which seems to be a linux kernel flag. But I'm unsure why we would need to even be aware of this.
I think future cleanup could be done here.

src/hotspot/os/posix/signals_posix.cpp line 442:

> 440: //
> 441:
> 442: #if defined(__APPLE__)

This should be checking for BSD, which would include macOS.

src/hotspot/os/posix/signals_posix.cpp line 456:

> 454: #endif
> 455:
> 456: // Set thread signal mask (for some reason on AIX sigthreadmask() seems

This comment block seems inappropriate for shared code. Are you suggesting this code might be wrong for AIX? If so it
shouldn't be pushed in this form.

src/hotspot/os/posix/signals_posix.cpp line 462:

> 460:   const int rc = ::pthread_sigmask(how, set, oset);
> 461:   // return value semantics differ slightly for error case:
> 462:   // pthread_sigmask returns error number, sigthreadmask -1 and sets global errno

There is no sigthreadmask in use.

src/hotspot/os/posix/signals_posix.cpp line 471:

> 469: // to POSIX, typical program error signals. If they happen while being blocked,
> 470: // they typically will bring down the process immediately.
> 471: bool unblock_program_error_signals() {

This is AIX specific and should just be a file local function for AIX only.

src/hotspot/os/posix/signals_posix.cpp line 494:

> 492:
> 493:   int orig_errno = errno;  // Preserve errno value over signal handler.
> 494: #if defined(__APPLE__)

Again this should be checking for BSD, whcih will include macOS.

There should be a better way to dispatch here. If the handler has a platform-independent name, and is declared in the
platform specific files, then it will link to that one definition.

src/hotspot/os/posix/signals_posix.cpp line 571:

> 569:     { SA_NODEFER,   "SA_NODEFER"   },
> 570: #if defined(AIX)
> 571:     { SA_ONSTACK,   "SA_ONSTACK"   },

Existing issue but we already have an entry for SA_ONSTACK.

src/hotspot/os/posix/signals_posix.cpp line 1310:

> 1308:
> 1309: address PosixSignals::ucontext_get_pc(const ucontext_t* ctx) {
> 1310: #if defined(AIX)

Again there must be a better way to do this dispatch. If the target were os::ucontext_get_pc, defined is os<os>.cpp
then we would link to the current platforms version.

src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp line 232:

> 230:     if (t != NULL) {
> 231:       if(t->is_Java_thread()) {
> 232:         thread = (JavaThread*)t;