8349178: runtime/jni/atExit/TestAtExit.java should be supported on static JDK

[jianglizhou]

Please review runtime/jni/atExit/TestAtExit.java test change:

• Remove BUILD_HOTSPOT_JTREG_LIBRARIES_JDK_LIBS_libatExit := java.base:libjvm. Don't explicitly link with libjvm, because it adds libjvm.so as a recorded dependency to libatExit.so (on Linux for example). That requires libjvm.so must be resolved and loaded successfully when libatExit.so is loaded. The static JDK (e.g. static-jdk image) does not provide a libjvm.so for runtime usage.
• Instead of calling the following functions directly in libatExit.c, change to look up the functions and calls them using the retrieved function addresses:
  • JNI_GetDefaultJavaVMInitArgs
  • JNI_GetCreatedJavaVMs
  • JNI_CreateJavaVM

On Linux (and similar) platform, there is no need to call dlopen for libjvm in libatExit.c explicitly, because the VM must already be loaded by the time when the libatExit native code is executed. Using RTLD_DEFAULT can "find symbols in the executable and its dependencies, as well as symbols in shared objects that were dynamically loaded with the RTLD_GLOBAL flag" (see https://man7.org/linux/man-pages/man3/dlsym.3.html).

jdk/src/java.base/unix/native/libjli/java_md.c

Line 533 in 9b49597

libjvm = dlopen(jvmpath, RTLD_NOW + RTLD_GLOBAL);

is where we dlopen libjvm with RTLD_GLOBAL for unix platform.

For Windows platform, I added Windows specific code to get the loaded jvm.dll first. If it can't find loaded jvm.dll, it then get the handle of the executable running the current process. The returned handle is used for finding the function addresses.

TestAtExit passes with https://github.com/jianglizhou/jdk/actions/runs/13124407248/job/36619759000. TestAtExit also pass on static-jdk with my local jtreg run.

---

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-8349178: runtime/jni/atExit/TestAtExit.java should be supported on static JDK (Enhancement - P4)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23431

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23431.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:

8349178: runtime/jni/atExit/TestAtExit.java should be supported on static JDK

Reviewed-by: dholmes

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

• 8f6ccde: 8349554: [UBSAN] os::attempt_reserve_memory_between reported applying non-zero offset to non-null pointer produced null pointer
• 7d52f1e: 8349525: RBTree: provide leftmost, rightmost, and a simple way to print trees
• e9278de: 8348411: C2: Remove the control input of LoadKlassNode and LoadNKlassNode
• 5395ffa: 8327378: XMLStreamReader throws EOFException instead of XMLStreamException
• 1ed9ef1: 8349559: Compiler interface doesn't need to store protection domain
• f0ea38b: 8349509: [macos] Clean up macOS dead code in jpackage
• 7f6c687: 8349374: [JVMCI] concurrent use of HotSpotSpeculationLog can crash
• bd9b24c: 8349512: Duplicate PermittedSubclasses entries with doclint enabled
• b40f8ee: 8337251: C1: Improve Class.isInstance intrinsic
• 88a8483: 8349121: SSLParameters.setApplicationProtocols() ALPN example could be clarified
• ... and 68 more: https://git.openjdk.org/jdk/compare/651ac3cc0f2a8b3edf5cddb42df1d38d4aa0e1a6...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 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]

Webrevs

• 00: Full (f22936b4)

[dholmes-ora]

@jianglizhou I will be brutally honest and say that I do not like this at all. Does this mean all JNI using tests/applications have to becoded differently to be used with a static JDK? I find it somewhat ironic that to work with a static JDK we have to rewrite the test to perform a dynamic lookup!

[jianglizhou]

@jianglizhou I will be brutally honest and say that I do not like this at all. Does this mean all JNI using tests/applications have to becoded differently to be used with a static JDK? I find it somewhat ironic that to work with a static JDK we have to rewrite the test to perform a dynamic lookup!

@dholmes-ora Thanks for the forthright response. JNI functions are already dynamically called. E.g. in the same test here:

    res = (*jvm)->AttachCurrentThread(jvm, (void **)&env, NULL);

We don't need to change those at all to work on static-jdk. The existing design of JNI functions can work on both dynamic JDK and static JDK.

The calls fixed by this PR are the very few exceptions that do not follow the design principle. I do think we should fix them by following the right patterns/conventions. Note LoadJavaVM in JDK code (https://github.com/openjdk/jdk/blob/b985347c2383a7a637ffa9a4a8687f7f7cde1369/src/java.base/unix/native/libjli/java_md.c#L524C1-L524C11) calls these functions 'properly', see

jdk/src/java.base/unix/native/libjli/java_md.c

Line 548 in b985347

ifn->GetDefaultJavaVMInitArgs = (GetDefaultJavaVMInitArgs_t)

.

So far I've only found this test in hotspot tier1 and libExplicitAttach (AttachTest_id0) in JDK tier1 with the issue.

@dholmes-ora If we don't go this change, do you have any other suggestion? One other possible alternative that I can think is using weak symbols, however I think that's no better.

[jianglizhou]

Also to point it out if it's not clear already, libjvm.so is implementation detail. One cannot safely that exists at runtime. The static JDK case is a good example.

[AlanBateman]

@jianglizhou I will be brutally honest and say that I do not like this at all. Does this mean all JNI using tests/applications have to becoded differently to be used with a static JDK? I find it somewhat ironic that to work with a static JDK we have to rewrite the test to perform a dynamic lookup!

A custom launcher will typically link to libjvm or use dlopen/dlym to get to JNI_CreateJavaVM. A static build isn't really suitable for custom launchers (or anything that embeds a VM).

So I think the changes to the test are okay if we really want to run this test with a static build. An alternative is to have the tests that use JNI_CreateJavaVM to not be selected. At some point I suspect we'll need to add a property to jtreg-ext/requires/VMProps.java for these builds anyway.

[jianglizhou]

A static build isn't really suitable for custom launchers (or anything that embeds a VM).

Just want to provide some relevant data point. We build custom launcher and statically link with the libjvm (for hermetic case). If libjvm is statically linked with the launcher, then there's actually no need to change the direct calls to those JNI_ functions. The direct calls are not problematic on static build.

An alternative is to have the tests that use JNI_CreateJavaVM to not be selected.

The alternative also sounds reasonable to me. We could skip running on static JDK if a test explicitly requires libjvm.so, libjava.so, etc at runtime, at least initially.

At some point I suspect we'll need to add a property to jtreg-ext/requires/VMProps.java for these builds anyway.

That's a good point. We can add a vm.static.jdk to help test selection.

[dholmes-ora]

Thanks for clarifying @jianglizhou . Okay so the issue is only with the top-level invocation API functions:

• JNI_GetDefaultJavaVMInitArgs
• JNI_GetCreatedJavaVMs
• JNI_CreateJavaVM
  and as noted launchers have the choice of either linking with libjvm.so or else using dynamic lookup. The former doesn't work with a static JDK (can we link with libjvm.a?). So in the context of fixing a couple of tests this is okay.

[dholmes-ora]

Also to point it out if it's not clear already, libjvm.so is implementation detail. One cannot safely that exists at runtime. The static JDK case is a good example.

Is there any other example? Presuming the existence of dynamic linked libraries is pretty standard.

[jianglizhou]

I want to answer the question carefully to avoid possible confusions, so the comment below may provide extra information (also verbose) than just addressing the question. Please let me know if anything is unclear.

and as noted launchers have the choice of either linking with libjvm.so or else using dynamic lookup. The former doesn't work with a static JDK (can we link with libjvm.a?).

Yes. A launcher executable can explicitly link with a native library, either using .so (shared library) or .a (static library) if the launcher code has any direct references to symbols defined in the native library. The launcher can also choose to do dynamic symbol lookup and avoids the need for explicitly linking with the native library.

If a launcher executable is linked with libjvm.so at build time, it requires libjvm.so to be resolved (normally from the RPATH) and loaded successfully when the launcher executable is loaded. Otherwise the executable fails to load and start due to the missing libjvm.so dependency. If a launcher executable is statically linked with libjvm.a, runtime does not try to find the libjvm.a since that is already built into the executable.

Same for a shared library (e.g. the libatExit.so used by the test) linked with libjvm.so, the shared library fails to be loaded if libjvm.so do not present at runtime.

If launcher code choose to use dynamic symbol lookup and avoids linking with libjvm.so, the launcher code can support both dynamic case and static case. The java launcher does that.

So in the context of fixing a couple of tests this is okay.

Yeah, since there are just few cases in the tests, I feel fixing these tests seem to be a good choice.

[jianglizhou]

Also to point it out if it's not clear already, libjvm.so is implementation detail. One cannot safely that exists at runtime. The static JDK case is a good example.

Is there any other example? Presuming the existence of dynamic linked libraries is pretty standard.

Other possible examples could be running the test on different JVM implementation (e.g. non-hotspot) that does not have libjvm.so.

[jianglizhou]

@dholmes-ora and @AlanBateman Could you please review and approve the change if there's no additional concerns? I also sent out #23500, which applies the same principle to fix libExplicitAttach issue.

I'm also in the process of adding the VMProps for static JDK as @AlanBateman suggested. That's needed to skip the tests require accessing tools in bin/, until we have a clear story on the tools support for static JDK/hermetic Java (that topic has been brought up in the hermetic Java meetings).

[jianglizhou]

@AlanBateman I created #23528 for the VMProps change. Thanks.

[dholmes-ora]

Okay - approved.

[jianglizhou]

Thanks, @dholmes-ora!

[jianglizhou]

/integrate

[openjdk]

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

• ab66c82: 8349639: jfr/event/gc/detailed/TestShenandoahEvacuationInformationEvent.java fails to compile after JDK-8348610
• f74c4df: 8349580: Do not use address in MemTracker top level functions
• c9cadbd: 8346567: Make Class.getModifiers() non-native
• 5589892: 8343074: test/jdk/com/sun/net/httpserver/docs/test1/largefile.txt could be generated
• d104deb: 8349556: RISC-V: improve the performance when -COH and -AvoidUnalignedAccesses for UL and LU string comparison
• 4a83ca1: 8349666: RISC-V: enable superwords tests for vector reductions
• 8f6ccde: 8349554: [UBSAN] os::attempt_reserve_memory_between reported applying non-zero offset to non-null pointer produced null pointer
• 7d52f1e: 8349525: RBTree: provide leftmost, rightmost, and a simple way to print trees
• e9278de: 8348411: C2: Remove the control input of LoadKlassNode and LoadNKlassNode
• 5395ffa: 8327378: XMLStreamReader throws EOFException instead of XMLStreamException
• ... and 74 more: https://git.openjdk.org/jdk/compare/651ac3cc0f2a8b3edf5cddb42df1d38d4aa0e1a6...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jianglizhou Pushed as commit 84b32cb.

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