On Wed, 18 Dec 2024 09:12:55 GMT, Joakim Nordström <[email protected]>
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
