RFR: 8253742: POSIX signal code cleanup

[Thomas Stuefe]

On Tue, 13 Oct 2020 14:19:02 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

> hi all,
> 
> Please review this followup to [JDK-8252324 Signal related code should be shared among POSIX platforms](https://bugs.openjdk.java.net/browse/JDK-8252324), where several issues were identified for a cleanup. This change addresses them all:
> 
> #1 David's feedback - removed non POSIX SIGNIFICANT_SIGNAL_MASK code
> 
> #2 David's feedback - used unblock_program_error_signals() on all platforms (this change is superseeded by JDK-8252533, will need to merge)
> 
> #3 David's feedback - used single JVM_handle_posix_signal API for all POSIX platforms
> 
> #4 Coleen's feedback - cleanup header files in src/hotspot/os/posix/signals_posix.hpp
> 
> #5 Coleen's feedback - hid SR_signum assignment in src/hotspot/os/posix/signals_posix.hpp to avoid having to include <signal.h>
> 
> #6 Coleen's feedback - factored out print_signal_handlers()
> 
> #7 Thomas' feedback - factored out common POSIX os::SuspendedThreadTask::internal_do_task()
> 
> #8 Thomas's feedback - factored out common POSIX signal initialization code
> 
> #9 YaSuenag's feedback - used JVM_handle_posix_signal for the common API
> 
> #10 YaSuenag's feedback - unified logging out of the scope for this fix
> 
> #11 YaSuenag's feedback - memset usage in PosixSignals::jdk_misc_signal_init() correct?

Hi Gerard,

good job!

But we really should synchronize  :)

I am currently working on: https://bugs.openjdk.java.net/browse/JDK-8255711
(see Draft PR: https://github.com/openjdk/jdk/pull/982)

and https://bugs.openjdk.java.net/browse/JDK-8252533 is also still open (see https://github.com/openjdk/jdk/pull/839) - still waiting Davids final OK.

So unfortunately there are a number of clashes with your change:

- "JVM_handle_xxx_signal()": See my mail to jdk-dev from this morning: https://mail.openjdk.java.net/pipermail/jdk-dev/2020-November/004887.html - we either can get completely rid of this function, which is what my current Draft for JDK-8255711 does. Or we need to retain it for backward compatibility, but if so, we need to retain it with the current interface.

Either way, could you please withhold changes to the hotspot signal handlers for the moment (so, both javaSignalHandler() and the various JVM_handle_xxx_signal() functions)?

- I removed some functions you changed - all that signal blocking mask stuff. See https://github.com/openjdk/jdk/pull/839. Could you hold any changes to those functions until JDK-8252533 is out of the door? Hope to do this tomorrow, with your and Davids approval.

The unification of the signal handler printing stuff and the SR initialization make sense and are a good simplification.
 
Please fine further remarks remarks inline.

Cheers, Thomas

make/hotspot/symbols/symbols-linux line 24:

> 22: #
> 23: 
> 24: JVM_handle_posix_signal

Please don't change these (see comment above).

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

> 1259:   PosixSignals::print_signal_handler(st, SHUTDOWN3_SIGNAL , buf, buflen);
> 1260:   PosixSignals::print_signal_handler(st, BREAK_SIGNAL, buf, buflen);
> 1261: #if defined(AIX)

Can you change both #ifdefs to: `#ifdef SIGDANGER` resp. `#ifdef SIGTRAP` please? Some other Unices have this too.

(Side note, I always wanted to change this coding to a loop to print all signal handlers unconditionally, regardless of whether this is a "hotspot signal" or not. Since when analyzing customer problems, sometimes its interesting to know if other handlers are installed too(eg SIGCHILD). At least that's how we do things in our propietary VM.)

src/hotspot/os/aix/os_aix.cpp line 1442:

> 1440: }
> 1441: 
> 1442: void os::print_signal_handlers(outputStream* st, char* buf, size_t buflen) {

This makes sense. But then, it would make sense to move this to os_posix.cpp completely. Or to even completely replace calls to os::print_signal_handlers with PosixSignals::print_signal_handlers() and remove the former.

src/hotspot/os/aix/os_aix.cpp line 3301:

> 3299: }
> 3300: 
> 3301: address os::ucontext_get_pc(const ucontext_t* ctx) {

+1

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

> 450: }
> 451: 
> 452: // Renamed from 'signalHandler' to avoid collision with other shared libs.

Please don't change those, nor javaSignalHandler().

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

> 457:   // on all our platforms they would bring down the process immediately when
> 458:   // getting raised while being blocked.
> 459:   unblock_program_error_signals();

As remarked above, this will conflict with JDK-8252533, since I remove this function too. Can we leave this out please?

src/hotspot/share/runtime/os.hpp line 970:

> 968: 
> 969:   static address ucontext_get_pc(const ucontext_t* ctx);
> 970:   static void    ucontext_set_pc(ucontext_t* ctx, address pc);

This feels misplaced here (and probably won't compile on windows) since ucontext_t is POSIX. At the very least needs ucontext.h. But I would consider moving this to os_posix.

-------------

PR: https://git.openjdk.java.net/jdk/pull/636