RFR: JDK-8252533: Signal handlers should run with synchronous error signals unblocked

[David Holmes]

Hi Thomas,

On 29/10/2020 4:48 pm, Thomas Stuefe wrote:
> On Thu, 29 Oct 2020 02:43:40 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>> Hi Thomas,
>>
>> I still feel that we should never explicitly block these error signals given it leads to undefined behaviour. So I'd like to see a change to os::signal at least in a future RFE.
> 
> Not a problem, that is reasonable. No need to make a new RFE. I will prepare a new version which excludes those signals from the sigaction sa_mask too, but leaves the unblocking in place. And I'll add the SR_handler. Since this will probably take some days to get tested again, I'll be happy if you review it after your vacation :)

Okay.

> Cheers, Thomas
> 
>>
>> For now I think all the signal handlers should explicitly unblock the error signals, which means the SR_handler also needs updating.
>>
>> Some other minor changes requested below.
>>
>> Thanks,
>> David
> 
>> src/hotspot/os/posix/vmError_posix.cpp line 105:
>>
>>> 103: static void crash_handler(int sig, siginfo_t* info, void* ucVoid) {
>>> 104:
>>> 105:   PosixSignals::unblock_error_signals();
>>
>> Just to be clear this crash_handler is only installed for the error signals, so then original code was redundantly processing "sig" twice?
> 
> I'm unsure what you mean. Do you mean, it had been added twice to newset?
> 
>>   sigaddset(&newset, sig);  <<
>>   // also unmask other synchronous signals
>>   for (int i = 0; i < NUM_SIGNALS; i++) {
>>     sigaddset(&newset, SIGNALS[i]);  <<
>>   }
>>   PosixSignals::unblock_thread_signal_mask(&newset);
> 
> That is true. I believe the original coding was from us too, so I feel responsible :)

Right - if the only expected values for sig were in fact one of those 
defined in SIGNALS then it gets added twice.

Cheers,
David
-----

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/839
>