RFR: 8253742: POSIX signal code cleanup [v3]

[David Holmes]

Typo:

On 4/11/2020 2:29 pm, David Holmes wrote:
> On Tue, 3 Nov 2020 21:35:08 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 (reverted for JDK-8252533)
>>>
>>> #3 David's feedback - used single JVM_handle_posix_signal API for all POSIX platforms (reverted for JDK-8255711)
>>>
>>> #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?
>>
>> Gerard Ziemski has updated the pull request incrementally with two additional commits since the last revision:
>>
>>   - use ifdef(SIGDANGER) and ifdef(SIGTRAP)
>>   - revert unblock_program_error_signals change
> 
> Hi Gerard,
> 
> Overall looking good. Some changes still to be finalized e.g ucontext_t related functions in os.hpp.
> 
> I flagged some os functions that are implemented in os_foo.cpp but which just call the Posix helper, which can be deleted from os_foo.cpp and simply added to os_posix.cpp. That can't be a further cleanup RFE if you want to limit changes in this PR.

s/can't be/can be/ :)

David
-----

> A few minor nits below.
> 
> Thanks,
> David
> 
> src/hotspot/os/aix/os_aix.cpp line 2578:
> 
>> 2576: }
>> 2577:
>> 2578: void os::SuspendedThreadTask::internal_do_task() {
> 
> We should be able to have a single definition of this function in os_posix.cpp too.
> 
> src/hotspot/os/posix/signals_posix.cpp line 1286:
> 
>> 1284: void PosixSignals::print_signal_handlers(outputStream* st, char* buf, size_t buflen) {
>> 1285:   st->print_cr("Signal Handlers:");
>> 1286:   PosixSignals::print_signal_handler(st, SIGSEGV, buf, buflen);
> 
> You shouldn't need the PosixSignals:: prefix in this method.
> 
> src/hotspot/os/posix/signals_posix.cpp line 1349:
> 
>> 1347:     sigaddset(&unblocked_sigs, SIGTRAP);
>> 1348:   #endif
>> 1349:   sigaddset(&unblocked_sigs, PosixSignals::SR_signum);
> 
> Shouldn't need the PosixSignals::prefix in this method
> 
> src/hotspot/os/posix/signals_posix.cpp line 1642:
> 
>> 1640:
>> 1641: void PosixSignals::do_task(Thread* thread, os::SuspendedThreadTask* task) {
>> 1642:   if (PosixSignals::do_suspend(thread->osthread())) {
> 
> Shouldn't need PosixSignals:: prefix in this method.
> 
> src/hotspot/os/posix/signals_posix.hpp line 33:
> 
>> 31:
>> 32: typedef siginfo_t siginfo_t;
>> 33: typedef sigset_t sigset_t;
> 
> I don't see why this is needed/wanted. We can include signal.h without a problem.
> 
> I'm not even sure what these typedefs means ??
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/636
>