RFR: 8345782: Refining the cases that libjsig deprecation warning is issued

[David Holmes]

On Wed, 18 Dec 2024 09:12:55 GMT, Joakim Nordström <jnordstrom at openjdk.org> wrote:

> Could I get a review of this fix to refine the warnings printed by `libjsig` when using the deprecated `signal()`/`sigset()` functions?
> 
> Currently the libjsig library supports chaining `signal()` and `sigset()`. With these functions being deprecated, `libjsig` warns when a program calls either function when the it's LD_PRELOADed. The warning is supposed to warn that when the support for `signal()` and `sigset()` is removed, the application might stop working as expected.
> 
>> VM warning: the use of signal() and sigset() for signal chaining was deprecated in version 16.0 and will be removed in a future release. Use sigaction() instead.
> 
> The warning is however printed in a few cases when its unnecessary, and also fails to catch some usages where it should be printed. With this fix the warning is printed (in order as they appear in jsig.c):
> 1. when `signal()` or `sigset()` is called for a JVM-used signal, after the JVM has installed its signals
> 2. when the JVM installs its signals (using `sigaction()`), and libjsig sees that `signal()` or `sigset()` has been used for a JVM-used signal
> 
> ### Changes
> * The printing is removed from `call_os_signal()`. This warning was only correct if `signal()` or `sigset()` was called for a JVM-used signal before the JVM was installed. This is now instead covered by step 1 above.
> * If the JVM was installed first, the `signal()`/`sigset()` calls were effectively translated to `sigaction()`, and the warning was _not_ printed. When the `signal()`/`sigset()` support is dropped, this behaviour will change and the warning should be printed. This is now covered by step 2.
> 
> ## Testing
> - [x] Test `open/test/hotspot/jtreg/runtime/signal` has added checks for the deprecation warning, and passes with the fix
> - [x] Tier1-3 in progress

Thanks for taking this on @jaokim and apologies I did not get to this before the Xmas/NY break.

The changes look good and I agree with the updated conditions as to when the deprecation warning is, and isn't, printed.

A couple of minor suggestions and a query about the test. Copyright years will need updating to 2025.

test/hotspot/jtreg/runtime/signal/SigTestDriver.java line 131:

> 129:                             if (deprecatedSigFunctionUsed && jvmInvolved && sigUsedByJVM) {
> 130:                                 if (!warningPrinted) {
> 131:                                     failureMessage = "FAILED Missing deprecation warning for mode " + mode +

Suggestion:

                                    failureMessage = "FAILED: Missing deprecation warning for mode " + mode +

test/hotspot/jtreg/runtime/signal/SigTestDriver.java line 136:

> 134:                                 }
> 135:                             } else if (warningPrinted) {
> 136:                                 failureMessage = "FAILED Deprecation warning shouldn't be printed for mode " + mode +

Suggestion:

                                failureMessage = "FAILED: Deprecation warning shouldn't be printed for mode " + mode +

test/hotspot/jtreg/runtime/signal/SigTestDriver.java line 176:

> 174:     /**
> 175:      * Return true for all signals used by the JVM. This only covers the
> 176:      * case where the JVM is started normally, and not with -Xrs.

The VM also uses SIGUSR2 (`SR_signum`), SIGINT, and SIGQUIT. Though the latter two only if no `-Xrs` is given.

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

PR Review: https://git.openjdk.org/jdk/pull/22806#pullrequestreview-2531291539
PR Review Comment: https://git.openjdk.org/jdk/pull/22806#discussion_r1903630765
PR Review Comment: https://git.openjdk.org/jdk/pull/22806#discussion_r1903630936
PR Review Comment: https://git.openjdk.org/jdk/pull/22806#discussion_r1903636877