8294594: Fix cast-function-type warnings in signal handling code

[shipilev]

After JDK-8294314, we would have signals_posix.cpp excluded with cast-function-type warning:

/home/shade/trunks/jdk/src/hotspot/os/posix/signals_posix.cpp: In function 'int SR_initialize()':
/home/shade/trunks/jdk/src/hotspot/os/posix/signals_posix.cpp:1727:20: error: cast between incompatible function types from 'void (*)(int, siginfo_t*, ucontext_t*)' to 'void (*)(int)' [-Werror=cast-function-type]
 1727 | act.sa_handler = (void (*)(int)) SR_handler;
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~

A closer look would reveal that we are using the wrong slots for SR_handler, sa_handler vs sig_handler, which manifests in type cast errors. man sigaction says:

       If SA_SIGINFO is specified in sa_flags, then sa_sigaction (instead of sa_handler) specifies the signal-handling
       function for signum. This function receives three arguments, as described below.

This PR waits for JDK-8294314 to land before merging the build parts, but the product code can be reviewed and tested already.

Additional testing:

• [x] Linux x86_64 fastdebug tier1
• [x] Cross-platform builds

---

Progress

• [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8294594: Fix cast-function-type warnings in signal handling code

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10494/head:pull/10494
$ git checkout pull/10494

Update a local copy of the PR:
$ git checkout pull/10494
$ git pull https://git.openjdk.org/jdk pull/10494/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10494

View PR using the GUI difftool:
$ git pr show -t 10494

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10494.diff

[bridgekeeper[bot]]

:wave: Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@shipilev The following labels will be automatically applied to this pull request:

• build
• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge[bot]]

Webrevs

• 02: Full (5d351532)
• 01: Full - Incremental (c469e840)
• 00: Full (2ca21e2a)

[openjdk[bot]]

@shipilev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8294594: Fix cast-function-type warnings in signal handling code

Reviewed-by: dholmes, kbarrett

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 11 new commits pushed to the master branch:

• 529cc48f355523fd162470b416a5081869adcf0e: 8295396: RISC-V: Cleanup useless CompressibleRegions
• 692cdab2be7dfc6e12b127f8e2c97bc41536cb84: 8295016: Make the arraycopy_epilogue signature consistent with its usage
• 21a825e059170e3a069b9f0982737c5839e6dae2: 8288387: GetLocalXXX/SetLocalXXX spec should require suspending target thread
• 8d751de3198675b22704cdccafaff2fc0fdd3f59: 8295231: Move all linking of native libraries to make
• f300ec8631b781938e6e96165ba23cda14a20f24: 8294546: document where javac differs when invoked via launcher and ToolProvider
• b269c51d10c353d9b7143b2239beb23c01352182: 8295395: Linux Alpha Zero builds fail after JDK-8292591
• ae60599e2ba75d80c3b4279903137b2c549f8066: 8295023: Interpreter(AArch64): Implement -XX:+PrintBytecodeHistogram and -XX:+PrintBytecodePairHistogram options
• 4d37ef2d545c016e6c3ad52171ea961d4406726f: 8295262: Build binutils out of source tree
• 0919a3a0c198a5234b5ed9a3bb999564d2382a56: 8294186: AArch64: VectorMaskToLong failed on SVE2 machine with -XX:UseSVE=1
• ec2981b83bc3ef6977b5f16d5222eb49b0ea49ad: 8293711: Factor out size parsing functions from arguments.cpp
• ... and 1 more: https://git.openjdk.org/jdk/compare/172006c0e9433046252bd79e8864890ab7c0ce56...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[kimbarrett]

Fix looks good as far as it goes (can't believe I didn't see what was going on here!) - but os::signal is still broken as it uses sa_handler instead of sa_sigaction.

Yikes! I think this change should proceed as is, and os::signal should be looked at as a new issue. That looks messy :(

[shipilev]

Fix looks good as far as it goes (can't believe I didn't see what was going on here!) - but os::signal is still broken as it uses sa_handler instead of sa_sigaction.

Yikes! I think this change should proceed as is, and os::signal should be looked at as a new issue. That looks messy :(

There is no rush, because we are waiting for another build system change to drop.

Why can't we do the same thing we did for SR_handler in SR_initialize?

@@ -864,7 +864,7 @@ void* os::signal(int signal_number, void* handler) {
   remove_error_signals_from_set(&(sigAct.sa_mask));
 
   sigAct.sa_flags   = SA_RESTART|SA_SIGINFO;
-  sigAct.sa_handler = CAST_TO_FN_PTR(sa_handler_t, handler);
+  sigAct.sa_sigaction = CAST_TO_FN_PTR(sa_sigaction_t, handler);
 
   if (sigaction(signal_number, &sigAct, &oldSigAct)) {
     // -1 means registration failed

It matches what we should do for SIG_INFO flag, and as Kim said, it is still likely yields the same code as sa_handler and sa_sigaction are probably the same on currently supported systems.

[kimbarrett]

Fix looks good as far as it goes (can't believe I didn't see what was going on here!) - but os::signal is still broken as it uses sa_handler instead of sa_sigaction.

Yikes! I think this change should proceed as is, and os::signal should be looked at as a new issue. That looks messy :(

There is no rush, because we are waiting for another build system change to drop.

Why can't we do the same thing we did for SR_handler in SR_initialize?

@@ -864,7 +864,7 @@ void* os::signal(int signal_number, void* handler) {
   remove_error_signals_from_set(&(sigAct.sa_mask));
 
   sigAct.sa_flags   = SA_RESTART|SA_SIGINFO;
-  sigAct.sa_handler = CAST_TO_FN_PTR(sa_handler_t, handler);
+  sigAct.sa_sigaction = CAST_TO_FN_PTR(sa_sigaction_t, handler);
 
   if (sigaction(signal_number, &sigAct, &oldSigAct)) {
     // -1 means registration failed

It matches what we should do for SIG_INFO flag, and as Kim said, it is still likely yields the same code as sa_handler and sa_sigaction are probably the same on currently supported systems.

For POSIX we expect the handler argument for os::signal to be a sigaction handler (taking 3 arguments). For Windows we expect os::signal to take a 1 argument handler, since Windows seemingly doesn't support the sigaction stuff. How is that a portability layer?

So all of the calls are going to need their handler functions examined. In at least one use (in os_windows), the UserHandler function takes 3 arguments though only uses one, and is called with only one.

Looking at where os::signal is called, it's not clear it needs to be in os, and given the inconsistent expectations it probably shouldn't be. All calls are in posix-specific or windows-specific files. I'd suggest removing os::signal entirely, and having windows code use ::signal and posix code use ::sigaction. Maybe with some convenience wrappers around each, but those wrappers are windows-specific or posix-specific and never the twain shall meet.

That all seems to me like it would be better to separate from this change, keeping this one nice and simple.

[shipilev]

For POSIX we expect the handler argument for os::signal to be a sigaction handler (taking 3 arguments). For Windows we expect os::signal to take a 1 argument handler, since Windows seemingly doesn't support the sigaction stuff. How is that a portability layer?

D'oh.

That all seems to me like it would be better to separate from this change, keeping this one nice and simple.

Agreed.

[kimbarrett]

For POSIX we expect the handler argument for os::signal to be a sigaction handler (taking 3 arguments). For Windows we expect os::signal to take a 1 argument handler, since Windows seemingly doesn't support the sigaction stuff. How is that a portability layer?

I'll file a bug report for this.

[kimbarrett]

For POSIX we expect the handler argument for os::signal to be a sigaction handler (taking 3 arguments). For Windows we expect os::signal to take a 1 argument handler, since Windows seemingly doesn't support the sigaction stuff. How is that a portability layer?

I'll file a bug report for this.

https://bugs.openjdk.org/browse/JDK-8295125

[dholmes-ora]

Okay I agree it makes no sense to have os::signal as shared code because you can't use it in a platform independent manner.

I can take that new bug (thanks @kimbarrett ) unless @shipilev really wants it? :)

[shipilev]

...unless @shipilev really wants it? :)

No, I do NOT want it :) I'll be happy to test it, though.

[shipilev]

Build system changes now merged with upstream (look how nice they are). Ready to integrate.

[shipilev]

/integrate

[openjdk[bot]]

Going to push as commit b06f1b149c8f8a49d4e42c4b782a4b3a22aa79f6. Since your change was applied there have been 16 commits pushed to the master branch:

• 71aa8210910dbafe30eccc772eaa7747f46be0cd: 8295176: some langtools test pollutes source tree
• bca7ab3c1109e6cff9b50ecdd3045cb0ae8f6953: 8295412: support latest VS2022 MSC_VER in abstract_vm_version.cpp
• c33ca0c5edd60454c58916cb588e5b2cfcc7b36a: 6229853: BasicTextAreaUI:create incompletely documents the possible returned View types
• 358ac07255cc640cbcb9b0df5302d97891a34087: 8294254: [macOS] javax/swing/plaf/aqua/CustomComboBoxFocusTest.java failure
• 490fcd0c2547cb4e564363f0cd121c777c3acc02: 8293833: Error mixing types with -XX:+UseCMoveUnconditionally -XX:+UseVectorCmov
• 529cc48f355523fd162470b416a5081869adcf0e: 8295396: RISC-V: Cleanup useless CompressibleRegions
• 692cdab2be7dfc6e12b127f8e2c97bc41536cb84: 8295016: Make the arraycopy_epilogue signature consistent with its usage
• 21a825e059170e3a069b9f0982737c5839e6dae2: 8288387: GetLocalXXX/SetLocalXXX spec should require suspending target thread
• 8d751de3198675b22704cdccafaff2fc0fdd3f59: 8295231: Move all linking of native libraries to make
• f300ec8631b781938e6e96165ba23cda14a20f24: 8294546: document where javac differs when invoked via launcher and ToolProvider
• ... and 6 more: https://git.openjdk.org/jdk/compare/172006c0e9433046252bd79e8864890ab7c0ce56...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@shipilev Pushed as commit b06f1b149c8f8a49d4e42c4b782a4b3a22aa79f6.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.