RFR: JDK-8260485: Simplify and unify handler vectors in Posix signal code [v2]

[Thomas Stuefe]

On Tue, 2 Feb 2021 10:04:04 GMT, David Holmes <dholmes at openjdk.org> wrote:

> Hi Thomas,
> 
> I've taken a first pass at this. I like parts of it, perhaps most of it, but am having trouble seeing the before/after picture clearly.
> 
> A few comments below.
> 
> Thanks,
> David

Thanks David.

I reduced the complexity of the patch somewhat by removing case (3) completely. I found that it adds very little useful functionality. So all that is left now is (1) the simplified handling of chained handlers and (2) the new logic for implementing the signal handler jni check.

I also changed/simplified `SavedSignalHandlers` somewhat. NSIG can be largeish (I believe 1024 on AIX) and that would have made the array-of-sigaction structures large too. Not that it really matters much. But I changed the class into a pointer array, with the sigaction structures living in C-Heap. Since this array is sparsely populated, that saves space. Also removes the need for the membership sigset, since a NULL slot would indicate its not set.

Further answers inline. Feel free to ask questions if something is unclear.

Thanks, Thomas

> src/hotspot/os/posix/signals_posix.cpp line 99:
> 
>> 97:   bool check_signal_number(int sig) const {
>> 98:     assert(sig > 0 || sig < NSIG, "invalid signal number %d", sig);
>> 99:     return sig > 0 || sig < NSIG;
> 
> Surely && not ||

Good catch, this was an oversight.

> src/hotspot/os/posix/signals_posix.cpp line 94:
> 
>> 92: //  structures and membership information.
>> 93: class SavedSignalHandlers {
>> 94:   struct sigaction _v[NSIG];
> 
> Why _v ??

v as in vector... I changed it to _sa.

> src/hotspot/os/posix/signals_posix.cpp line 105:
> 
>> 103:   void mark_as_set(int sig)   { sigaddset(&_set, sig); }
>> 104:   void mark_as_clear(int sig) { sigdelset(&_set, sig); }
>> 105: 
> 
> These don't really pay for themselves IMO. They are private and only used once each, and are a single line of code. The extra abstraction layer really doesn't add anything in terms of clarity.

True. I removed the whole sigset. See my comment below.

> src/hotspot/os/posix/signals_posix.cpp line 844:
> 
>> 842:     if (sig == SHUTDOWN2_SIGNAL && !isatty(fileno(stdin))) {    // Flags?
>> 843:       tty->print_cr("Running in non-interactive shell, %s handler is replaced by shell",
>> 844:                     os::exception_name(sig, buf, O_BUFLEN));    // When comparing, ignore the SA_RESTORER flag on Linux
> 
> I don't understand the flag comments in this block.

Just a typo.

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

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