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

[Gerard Ziemski]

On Mon, 28 Sep 2020 03:40:46 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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.
>
> 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.

Fixed.

> 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.

This is just existing AIX code, nothing new. I assume it's fine since that's how it was before. In our POSIX impl it's
guarded by #if def(AIX)

> 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.

That's how it was in the original AIX code, I will just add #if def(AIX) around set_thread_signal_mask() and
unblock_program_error_signals() for now. I was hoping to figure out later, whether the other POSIX platforms can use
that code as well...

Added to the followup JDK issue

> 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.

Yes, same with set_thread_signal_mask(), fixed.

> 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.

Right, will leave for future cleanup - here I am just putting the code together, no other fixes.

Added to the followup JDK issue

> 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.

Fixed.

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

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