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

[Thomas Stuefe]

On Tue, 16 Feb 2021 18:58:15 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Use universal zero initializer for do_check_signal_periodically
>
> src/hotspot/os/posix/signals_posix.cpp line 1222:
> 
>> 1220:   // Query the current signal handler. Needs to be a separate operation
>> 1221:   // from installing a new handler since we need to honor AllowUserSignalHandlers.
>> 1222:   void* oldhand = get_signal_handler(&oldAct);
> 
> It's a pre-existing issue, but I dislike the **"old"** in **oldhand** and **oldAct**. It makes it sound like it is a cached or previous value, which is especially confusing when in the comment we refer to it as the **current** one. Any chance we can clean this up in this fix?

I think it stems from the Posix interface calling the parameters `act` and `oact` and ppl are used to that naming:
int sigaction(int sig, const struct sigaction *restrict act,
       struct sigaction *restrict oact);
That said, we use a bunch of different names, like oact, oldAct or oldSigAct, which I disliked. I wanted to unify those namings but refrained since I wanted to keep the patch small. Its difficult enough as it is.

Since this would be an easy change in itself, can we keep it for a different RFE? As for name, lets say currentAct?

> src/hotspot/os/posix/signals_posix.cpp line 153:
> 
>> 151: //  and compare it periodically against reality (see os::run_periodic_checks()).
>> 152: static bool check_signals = true;
>> 153: static SavedSignalHandlers expected_handlers;
> 
> I personally would prefer `vm_handlers` or `java_handlers` or `our_handlers` here instead of `expected_handlers`.

Okay, vm_handlers it is.

> src/hotspot/os/posix/signals_posix.cpp line 817:
> 
>> 815:   const int expected_flags = get_sanitized_sa_flags(expected_sa);
>> 816:   return this_handler == expected_handler &&
>> 817:          this_flags == expected_flags;
> 
> Could we use brackets here like:
> 
>   return ((this_handler == expected_handler) &&
>            (this_flags == expected_flags));

I can add the inside brackets, but don't see the need for the outside ones, there is nothing to bracket off against.

> src/hotspot/os/posix/signals_posix.cpp line 845:
> 
>> 843: 
>> 844:   // Compare both sigaction structures (intelligently; only the members we care about).
>> 845:   if (!compare_handler_info(&act, expected_act)) {
> 
> The name `compare_handler_info()` makes it sound like a function that tells whether we should compare the values, not whether the values' comparison evaluates to true.
> 
> How about `are_handlers_equal()`?

Ok

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

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