8351309: test/hotspot/jtreg/runtime/posixSig/TestPosixSig.java fails on static-jdk

[jianglizhou]

Please review this PR that excludes libjsig from being statically linked with static-jdk java launcher by default. Please see details in https://bugs.openjdk.org/browse/JDK-8351309 description and comments. Thanks

---

Progress

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

Issue

• JDK-8351309: test/hotspot/jtreg/runtime/posixSig/TestPosixSig.java fails on static-jdk (Bug - P4)

Reviewers

• Man Cao (@caoman - Committer)
• Thomas Stuefe (@tstuefe - Reviewer) Review applies to 9e1aac2e
• Magnus Ihse Bursie (@magicus - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23924

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jiangli! 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]

@jianglizhou 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:

8351309: test/hotspot/jtreg/runtime/posixSig/TestPosixSig.java fails on static-jdk

Reviewed-by: manc, ihse, stuefe

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 71 new commits pushed to the master branch:

• 4be502e: 8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms
• 1fe4526: 8350194: Last 2 parameters of ReturnNode::ReturnNode are swapped in the declaration
• 1d147cc: 8351484: Race condition in max stats in MonitorList::add
• 4412c07: 8351639: Improve debuggability of test/langtools/jdk/jshell/JdiHangingListenExecutionControlTest.java test
• 1dd9cf1: 8349099: java/awt/Headless/HeadlessMalfunctionTest.java fails on CI with Compilation error
• 64464ea: 8351673: Clean up a case of if (LockingMode == LM_LIGHTWEIGHT) in a legacy-only locking mode function
• 9a49418: 8345940: Migrate security-related resources from Java classes to properties files
• e71f327: 8351045: ClassValue::remove cannot ensure computation observes up-to-date state
• cef3693: 8351656: Problemlist gc/TestAllocHumongousFragment#generational
• da2b4f0: 8351606: Use build_platform for graphviz dependency
• ... and 61 more: https://git.openjdk.org/jdk/compare/11a37c829c12d064874416a7b242596cf23972e5...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.

[openjdk]

@jianglizhou The following label will be automatically applied to this pull request:

• build

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

[mlbridge]

Webrevs

• 01: Full - Incremental (9419b7f1)
• 00: Full (9e1aac2e)

[caoman]

This LGTM. Perhaps wait for another approval from a reviewer?

libjsig is not supposed to be enabled by default. Per Java 23 doc, it is user's choice whether to use libjsig.so. If they use it, they should either link the application with linker flag -ljsig, or use LD_PRELOAD.

[dholmes-ora]

I am a bit confused here. libjsig is an application/end-user library. It sounds like something is statically linking libjsig and causing signal chaining to break. ???

[magicus]

This is not the correct way to solve this. You should flag libjsig as ONLY_EXPORTED instead, which means that the library will not be included in a static build.

[jianglizhou]

libjsig is not supposed to be enabled by default. Per Java 23 doc, it is user's choice whether to use libjsig.so. If they use it, they should either link the application with linker flag -ljsig, or use LD_PRELOAD.

For static JDK usages, the documentation would need to be updated when things are mature. There are cases where dynamic linking is not preferred and static linking should be used, and those are common in real world usages. We want to provide libjsig.a as part of the static bundle, users can link the launcher executable with libjsig.a for hermetic image if application wants to use libjsig.

I think in general, using libjsig.so shared library is not an attractive solution on static & hermetic Java image, if application uses libjsig. In order to use libjsig.so for a hermetic Java image, there would be a separate regular JDK with the shared libraries installed into the target machine. Then it loses part of the benefits of using hermetic Java image.

[jianglizhou]

I am a bit confused here. libjsig is an application/end-user library.

libjsig is provided by JDK.

It sounds like something is statically linking libjsig and causing signal chaining to break. ???

AFAICT, signal chaining works when libjsig is statically linked (I stepped through the code in gdb and checked).

TestPosixSig.java failure is a test problem. The test looks for the print output from os::run_periodic_checks. The periodic signal checker is disabled, when libjsig is used. See

jdk/src/hotspot/os/posix/signals_posix.cpp

Line 1359 in 0892913

// We don't activate signal checker if libjsig is in place, we trust ourselves

:

  // We don't activate signal checker if libjsig is in place, we trust ourselves
  // and if UserSignalHandler is installed all bets are off.

[jianglizhou]

This is not the correct way to solve this. You should flag libjsig as ONLY_EXPORTED instead, which means that the library will not be included in a static build.

I think we do want to include libjsig.a as part of the JDK static libs. When users build application hermetic image, they can choose to whether statically link with or without libjsig.a. See my other comments for more details, #23924 (comment).

[magicus]

But if we do not link it with the launcher, it will not get tested. I don't think it is a good idea to provide a binary that have not even had a shred of testing. I'm rather leaning towards saying that signal chaining is not possible for a static JDK.

[jianglizhou]

But if we do not link it with the launcher, it will not get tested. I don't think it is a good idea to provide a binary that have not even had a shred of testing. I'm rather leaning towards saying that signal chaining is not possible for a static JDK.

Signal chaining works on static. So I don't think we should say it's not possible. :-)

I had some thoughts on testing earlier. For testing we want to create a test that builds a hermetic image (when supported in mainline) that statically links with the libjsig.a, to exercise the signal chaining in that combination. We should create a bug for that so we don't forget.

The main point is that we don't want to test with signal chaining enabled by default on static JDK.

[magicus]

I meant more like "not supported". You are correct that it is technically possible.

How useful is signal chaining even these days?

While we could do something like this, I see more trouble ahead. How should we do when we put the static libs in a jmod? Should all static libraries have an "optional/required" flag attached to them? How do the user select if libjsig should be included or not?

I would argue that for the time being, it is better to set libjsig as ONLY_EXPORTED, and if/when we get back to revisit this, we can start compiling it always, and then we will also have a story on how to test it, how to handle it in jmods, etc, including perhaps a better idea on how important it is to have signal chaining for static builds.

[slowhog]

IIUC, signal chaining is a link time question for native executable using hotspot(launchers), the java launcher in regular JDK build is disabled by default unless user preload the libjsig.

So, for the future jmod to support static linking, it could include libjsig.a or leave it out and have user to explicit download and link their native application.

As for the java launcher in hermetic java, we need to decide whether we want to support signal chaining by default, and update the test accordingly.

[jianglizhou]

I meant more like "not supported". You are correct that it is technically possible.

How useful is signal chaining even these days?

While we could do something like this, I see more trouble ahead. How should we do when we put the static libs in a jmod? Should all static libraries have an "optional/required" flag attached to them? How do the user select if libjsig should be included or not?

I would argue that for the time being, it is better to set libjsig as ONLY_EXPORTED, and if/when we get back to revisit this, we can start compiling it always, and then we will also have a story on how to test it, how to handle it in jmods, etc, including perhaps a better idea on how important it is to have signal chaining for static builds.

I think I could be convinced, but it probably not a decision could be made by just us. :-) I think it would require broader discussion and decision making on if the JDK signal chaining provided by libjsig should be supported on static JDK. That decision probably should be made as part of a JEP process when we move to that step.

[jianglizhou]

IIUC, signal chaining is a link time question for native executable using hotspot(launchers), the java launcher in regular JDK build is disabled by default unless user preload the libjsig.

So, for the future jmod to support static linking, it could include libjsig.a or leave it out and have user to explicit download and link their native application.

As for the java launcher in hermetic java, we need to decide whether we want to support signal chaining by default, and update the test accordingly.

Right. And, I think we should not enable that by default to keep the same behavior as on regular JDK.

[magicus]

It seems we agree that we need, at some point, to have a high-level discussion on if libjsig should be supported on static builds, and if so, how it should be implemented. We also agree that having signal chaining enabled by default on our static JDK builds are incorrect.

However, I suggest we chose a simple path, were we utilize the framework for not building a static library that we do not use (this is already done for several libraries that are not included in the static JDK image), while you suggest we keep compiling it, even if we do not include it and test it.

I don't get the point of this. If we don't include it, and don't test it, then surely it would be better to not even build it now?

[jianglizhou]

It seems we agree that we need, at some point, to have a high-level discussion on if libjsig should be supported on static builds, and if so, how it should be implemented. We also agree that having signal chaining enabled by default on our static JDK builds are incorrect.

Thumbs up.

However, I suggest we chose a simple path, were we utilize the framework for not building a static library that we do not use (this is already done for several libraries that are not included in the static JDK image), while you suggest we keep compiling it, even if we do not include it and test it.

I don't get the point of this. If we don't include it, and don't test it, then surely it would be better to not even build it now?

We have no current usages of libjsig in our current hermetic Java testing and prototype development work. So I don't have related concerns if don't produce a libjsig.a in the static-libs for the short term. Let's check with @dougxc and others from GraalVM side to make sure that they are okay with removing libjsig.a from the static libs bundle, before making any changes.

[jianglizhou]

I filed https://bugs.openjdk.org/browse/JDK-8351367 for deciding the overall libjsig signal chaining support on static JDK.

[dholmes-ora]

libjsig is provided by JDK.

Yes for an application to chose to use so that its own signal usage can be better integrated with that of the JDK. Statically linking libjsig with a JDK makes no sense to me at all.

TestPosixSig.java failure is a test problem. The test looks for the print output from os::run_periodic_checks. The periodic signal checker is disabled, when libjsig is used.

So the test fails if you have statically linked libjsig into a JDK image. My response is "don't do that".

[dholmes-ora]

How useful is signal chaining even these days?

@magicus as useful as it has ever been if your application uses signals that overlap with JVM usage.

[dougxc]

Let's check with @dougxc and others from GraalVM side

@wirthi or someone from the Native Image team will comment on this. Thanks for the heads up.

[tstuefe]

I meant more like "not supported". You are correct that it is technically possible.

How useful is signal chaining even these days?

It is a niche solution to a complex problem (using signals in third-party native libraries which do their signal setup after hotspot signal setup has happened, preventing the library from messing up the hotspot signal mechanism). It is certainly used and needs to continue existing.

[tstuefe]

To me this feels like a valid solution. Barring objections by others, I am fine with it.

[jianglizhou]

libjsig is provided by JDK.

Yes for an application to chose to use so that its own signal usage can be better integrated with that of the JDK. Statically linking libjsig with a JDK makes no sense to me at all.

@dholmes-ora Please correct if I'm wrong, I interpret the above as not statically linking libjsig with JDK by default. Leave the decision to the users when building application hermetic/static images according to their usage requirements.

[jianglizhou]

@tstuefe Thanks for taking a looking!

[olpaw]

Let's check with @dougxc and others from GraalVM side to make sure that they are okay with removing libjsig.a from the static libs bundle, before making any changes.

Hi @jianglizhou,

Native image has no use for libjsig.a and we are fine if it is not part of static libs bundle.

[magicus]

With that confirmation, I think we should go on with the ONLY_EXPORTED solution instead.

If needed, we can revisit this in the future, to discuss how we should handle an optional library like libjsig. But that requires a complete solution througout the jmod/jlink pipeline to be meaningful.

[jianglizhou]

Let's check with @dougxc and others from GraalVM side to make sure that they are okay with removing libjsig.a from the static libs bundle, before making any changes.

Hi @jianglizhou,

Native image has no use for libjsig.a and we are fine if it is not part of static libs bundle.

Thanks for the feedback!

While making the change suggested by @magicus , I realize it libjsig.a is still included in the static libs bundle (at least not affect by the current change). It only exclude libjsig.a for java.base module-included-libs.

[jianglizhou]

With that confirmation, I think we should go on with the ONLY_EXPORTED solution instead.

Done. I also reverted StaticLibs.gmk change.

@caoman @tstuefe, please see if the updated change still looks okay to you.

If needed, we can revisit this in the future, to discuss how we should handle an optional library like libjsig. But that requires a complete solution througout the jmod/jlink pipeline to be meaningful.

Let's follow up with https://bugs.openjdk.org/browse/JDK-8351367.

[caoman]

Still LGTM. I recommend waiting for approval from @magicus as well.

[jianglizhou]

With that confirmation, I think we should go on with the ONLY_EXPORTED solution instead.

Done. I also reverted StaticLibs.gmk change.

@caoman @tstuefe, please see if the updated change still looks okay to you.

I also tested with running jlink to create a custom JDK (dynamic regular). This change does not affect that. libjsig.so is still properly included in the jlink'ed output JDK.

[jianglizhou]

Still LGTM. I recommend waiting for approval from @magicus as well.

Thanks @caoman! Yes, we should wait for @magicus.

[jianglizhou]

Thanks, @magicus!

[jianglizhou]

/integrate

[openjdk]

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

• 3b189e0: 8351345: [IR Framework] Improve reported disabled IR verification messages
• 95b66d5: 8351700: Remove code conditional on BarrierSetNMethod being null
• 84f87dd: 8351665: Remove unused UseNUMA in os_aix.cpp
• 4be502e: 8350642: Interpreter: Upgrade CountBytecodes to 64 bit on 64 bit platforms
• 1fe4526: 8350194: Last 2 parameters of ReturnNode::ReturnNode are swapped in the declaration
• 1d147cc: 8351484: Race condition in max stats in MonitorList::add
• 4412c07: 8351639: Improve debuggability of test/langtools/jdk/jshell/JdiHangingListenExecutionControlTest.java test
• 1dd9cf1: 8349099: java/awt/Headless/HeadlessMalfunctionTest.java fails on CI with Compilation error
• 64464ea: 8351673: Clean up a case of if (LockingMode == LM_LIGHTWEIGHT) in a legacy-only locking mode function
• 9a49418: 8345940: Migrate security-related resources from Java classes to properties files
• ... and 64 more: https://git.openjdk.org/jdk/compare/11a37c829c12d064874416a7b242596cf23972e5...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jianglizhou Pushed as commit a21fa46.

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