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

[Thomas Stuefe]

On Mon, 15 Feb 2021 22:19:04 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 115:
> 
>> 113:     assert(sig > 0 && sig < NSIG, "invalid signal number %d", sig);
>> 114:     return sig > 0 && sig < NSIG;
>> 115:   }
> 
> We have
> 
>  assert(sig > 0 && sig < NSIG, "invalid signal number %d", sig);
>  return sig > 0 && sig < NSIG;
> 
> here in `SavedSignalHandlers::check_signal_number()` and
> 
> #if defined(__APPLE__)
>   return sig >= 1 && sig < NSIG;
> #else
> 
> in `is_valid_signal()`.
> 
> Can we combine those, something like:
> 
> // Returns true if signal number is valid.
> static bool is_valid_signal(int sig) {
>   assert(sig > 0 && sig < NSIG, "invalid signal number %d", sig);
>   if (sig > 0 && sig < NSIG) {
>     // Use sigaddset to check for signal validity.
>     sigset_t set;
>     sigemptyset(&set);
>     if (sigaddset(&set, sig) == -1 && errno == EINVAL) {
>       return false;
>     } else {
>       return true;
>     }
>   } else {
>     return false;
>   }
> }
> 
> so then we can drop `SavedSignalHandlers::check_signal_number()` altogether?

I would like to keep this. SavedSignalHandlers::check_signal_number() is just a simple bounds check to prevent memory overwriters, with an added assert for goot measure in debug builds. Its proximity to the array it guards makes this clear. SavedSignalHandlers gets only fed with know values in this file, so an assert is sufficient.

is_valid_signal() OTOH does a runtime check for signal validity. It is more expensive but only used in analysis situations. It pulls its signal number from a sigaction structure, which may be corrupt and contain whatever, so the runtime check makes more sense to me.

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

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