On Wed, 8 Jan 2025 11:03:06 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
>
> Joakim Nordström has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Merge
> - Copyright year. Doc updated. Comments revised.
> - Merge
src/java.base/unix/native/libjsig/jsig.c line 265:
> 263: if (deprecated_usage[sig] == true) {
> 264: print_deprecation_warning();
> 265: }
Yes, took me a minute to realise this is needed because back when/if signal was
called, we set deprecated_usage[sig] but only called
print_deprecation_warning() if jvm_signal_installed && sigused.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22806#discussion_r1908516725
