8355080: java.base/jdk.internal.foreign.SystemLookup.find() doesn't work on static JDK

[jianglizhou]

Please review this PR that changes to use NativeLibraries.loadLibrary() for loading the libsyslookup in jdk.internal.foreign.SystemLookup class.

NativeLibraries.loadLibrary() handles both the shared library and (static) built-in library loading properly. On static-jdk, calling NativeLibraries.loadLibrary() for systlookup library can find the built-in library by looking up using JNI_OnLoad_syslookup. The current change adds DEF_STATIC_JNI_OnLoad in syslookup.c (in shared, windows and aix versions) for that purpose.

In addition to GHA testing, I tested the change on static-jdk with jdk tier1 tests on linux-x64 locally. All java/foreign/* jdk tier1 tests pass with the change. Without the change, there are about 60 java/foreign/* jdk tier1 tests fail on static-jdk.

---

Progress

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

Issue

• JDK-8355080: java.base/jdk.internal.foreign.SystemLookup.find() doesn't work on static JDK (Bug - P4)

Reviewers

• Maurizio Cimadamore (@mcimadamore - Reviewer)
• Jorn Vernee (@JornVernee - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24801

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

Using diff file

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

8355080: java.base/jdk.internal.foreign.SystemLookup.find() doesn't work on static JDK

Reviewed-by: mcimadamore, jvernee

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

• b1e778d: 8354686: [AIX] now ubsan is possible
• 66358fa: 8355616: Incorrect ifdef in compilationMemoryStatistic.cpp
• e7a4162: 8355717: Problem list tests until JDK-8355708 is fixed
• ... and 141 more: https://git.openjdk.org/jdk/compare/684d3b336e9cb31707d35e75f9b785e04e1fdbee...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
• core-libs

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

• 02: Full - Incremental (6661b815)
• 01: Full - Incremental (11f674c6)
• 00: Full (7cc1b268)

[liach]

sysLookup does look much cleaner compared to jdkLibraryPath.

[jianglizhou]

sysLookup does look much cleaner compared to jdkLibraryPath.

@liach Thanks for the quick review!

[slowhog]

Do we need this include? DEF_STATIC_JNI_OnLoad is defined by jni_util.h, which should include jni.h?

[jianglizhou]

Do we need this include? DEF_STATIC_JNI_OnLoad is defined by jni_util.h, which should include jni.h?

Removed, thanks!

[openjdk]

⚠️ @jianglizhou This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[jianglizhou]

@liach @slowhog Can you approve the change? Thanks!

[slowhog]

I would consult @mcimadamore or @JornVernee look at this for the native function call permission requirements.

[jianglizhou]

@mcimadamore or @JornVernee Can you help take a look of this? Thanks!

[liach]

Unfortunately, I believe both of them are on vacation, yet they have the required expertise to review this PR. You might need to wait a bit, sorry!

[jaikiran]

Hello Jiangli, if I understand this change correctly, then this is forcing a non-JNI library to be a JNI library for it to work on static JDK. Is that a requirement for static JDK builds? Or is it just a convenient way to use existing library loading mechanism in the jdk.internal.loader.NativeLibraries?

Are there other similar libraries shipped in JAVA_HOME/lib that would require a similar change/treatment?

[jianglizhou]

Hello Jiangli, if I understand this change correctly, then this is forcing a non-JNI library to be a JNI library for it to work on static JDK. Is that a requirement for static JDK builds? Or is it just a convenient way to use existing library loading mechanism in the jdk.internal.loader.NativeLibraries?

Hi @jaikiran Good questions. I kind of expected questions like those might be raised with the addition of DEF_STATIC_JNI_OnLoad to libsyslookup, when I made the change.

The original static built-in JNI library support in JDK was added by JEP 178 (https://openjdk.org/jeps/178) "Statically-Linked JNI Libraries" work. It's on top of the JNI library support. Both spec & implementation expected JNI_OnLoad_L (L as the library name) in a statically linked JNI library. From JNI spec (https://docs.oracle.com/en/java/javase/24/docs/specs/jni/invocation.html#jni_onload_l):

JNI_OnLoad_L
jint JNI_Onload_<L>(JavaVM *vm, void *reserved);

Mandatory function that must be defined by statically linked libraries .

If a library, named 'L', is statically linked, then upon the first invocation of System.loadLibrary("L") or equivalent API, a JNI_OnLoad_L function will be invoked with the same arguments and expected return value as specified for the JNI_OnLoad function. JNI_OnLoad_L must return the JNI version needed by the native library. This version must be JNI_VERSION_1_8 or later. If the VM does not recognize the version number returned by JNI_OnLoad_L, the VM will act as if the library was never loaded.

LINKAGE:
Exported from statically linked native libraries that contain native method implementations.

PARAMETERS:
vm: a pointer to the current VM structure.

reserved: unused pointer.

RETURNS:
Return the required JNI_VERSION constant (see also GetVersion). The minimum version returned being at least JNI_VERSION_1_8.

SINCE:
JDK/JRE 1.8

JNI_OnUnload_L
void JNI_OnUnload_<L>(JavaVM *vm, void *reserved);

Optional function defined by statically linked libraries. When the class loader containing a statically linked native library 'L' is garbage collected, the VM will invoke the JNI_OnUnload_L function of the library if such a function is exported.

This function can be used to perform cleanup operations. Because this function is called in an unknown context (such as from a finalizer), the programmer should be conservative on using Java VM services, and refrain from arbitrary Java call-backs.

LINKAGE:
Exported from statically linked native libraries that contain native method implementations.

PARAMETERS:
vm: a pointer to the current VM structure.

reserved: unused pointer.

SINCE:
JDK/JRE 1.8

Informational Note:
The act of loading a native library is the complete process of making the library and its native entry points known and registered to the Java VM and runtime. Note that simply performing operating system level operations to load a native library, such as dlopen on a UNIX(R) system, does not fully accomplish this goal. A native function is normally called from the Java class loader to perform a call to the host operating system that will load the library into memory and return a handle to the native library. This handle will be stored and used in subsequent searches for native library entry points. The Java native class loader will complete the load process once the handle is successfully returned to register the library.

You are right that this PR change turns the libsyslookup from a "non-JNI" native library to JNI native library. The DEF_STATIC_JNI_OnLoad macro adds JNI_OnLoad_syslookup in libsyslookup. The JNI_OnLoad_syslookup symbol can be used by Java_jdk_internal_loader_NativeLibraries_findBuiltinLib (

jdk/src/java.base/share/native/libjava/NativeLibraries.c

Line 235 in 4b88029

Java_jdk_internal_loader_NativeLibraries_findBuiltinLib

) to find the statically linked library.

Are there other similar libraries shipped in JAVA_HOME/lib that would require a similar change/treatment?

Yes. Our current static/hermetic work has found any missing DEF_STATIC_JNI_OnLoad in the JDK JNI libraries and added them.

[mcimadamore]

The changes here look good. Essentially, if syslookup is statically linked, trying to do a dlopen on it will fail, as the shared library for it doesn't exist. For this reason, this PR seems to "upgrade" syslookup to a JNI library. The presence of the JNI_OnLoad_syslookup is then used, as per JEP 178 to determine whether syslookup is a "builtin" library -- that is, a library for which no dynamic linking should occur.

The loaded library is associated with the boot classloader. This could be problematic, in principle. However, since the requests to loadLibrary also come from the boot loader (they are from the SystemLookup class) it all works out ok.

Having to upgrade to JNI is a bit sad -- although I get that it is required as a workaround for now. For the longer term I'd prefer a better way to integrate static lookups in the FFM API. For instance, all JNI::loadLibrary does when it comes to static libraries is to return the so called "process handle" -- e.g. a handle to the running process (the JVM). If there was a way to retrieve such a handle (e.g. via a dedicated SymbolLookup factory) it would be possible to avoid the JNI dance: just get the process symbol lookup, and look for statically linked symbols in there.

[mcimadamore]

/reviewers 2

[openjdk]

@mcimadamore
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[mcimadamore]

Looks good to me -- but also risky. I'd prefer if @JornVernee could also take a look.

[JornVernee]

Having to upgrade to JNI is a bit sad -- although I get that it is required as a workaround for now. For the longer term I'd prefer a better way to integrate static lookups in the FFM API. For instance, all JNI::loadLibrary does when it comes to static libraries is to return the so called "process handle" -- e.g. a handle to the running process (the JVM). If there was a way to retrieve such a handle (e.g. via a dedicated SymbolLookup factory) it would be possible to avoid the JNI dance: just get the process symbol lookup, and look for statically linked symbols in there.

I remember this wasn't quite as elegant on Windows, but I suppose there is already some solution in place to look up symbols that are statically linked into the same executable?

[JornVernee]

I had a look at the implementation, and see how it works now. The JNI_OnLoad_XXX function is essentially being used as a marker to indicate that this library was statically linked into the VM binary, so we can create a NativeLibrary instance that is marked as 'builtin', which changes the way symbol lookups are handled.

I think the current solution is probably fine. I think alternatively we could keep track of a table with statically linked libraries to determine where/how symbols should be looked up for a particular library. Or, the symbol doesn't necessarily have to be JNI_OnLoad_XXX, but could be some other, non-JNI-specific, marker symbol.

[jianglizhou]

Having to upgrade to JNI is a bit sad -- although I get that it is required as a workaround for now. For the longer term I'd prefer a better way to integrate static lookups in the FFM API. For instance, all JNI::loadLibrary does when it comes to static libraries is to return the so called "process handle" -- e.g. a handle to the running process (the JVM). If there was a way to retrieve such a handle (e.g. via a dedicated SymbolLookup factory) it would be possible to avoid the JNI dance: just get the process symbol lookup, and look for statically linked symbols in there.

I remember this wasn't quite as elegant on Windows, but I suppose there is already some solution in place to look up symbols that are statically linked into the same executable?

With the current effort for enhancing/completing static JDK support, the system is able to detect if it's running on static using checks like is_vm_statically_linked/JVM_IsStaticallyLinked/JLI_IsStaticallyLinked. For example, os::native_java_library (

jdk/src/hotspot/share/runtime/os.cpp

Line 516 in 2f84480

void* os::native_java_library() {

) and load_zip_library retrieve the os::get_default_process_handle without doing dlopen (e.g. on Linux) to load the native libraries. For longer term, we can make the solution more coherent/general for handling the various cases in the system.

[jianglizhou]

@mcimadamore @JornVernee Thanks for the in-depth thoughts and reviews!

[jianglizhou]

/integrate

[openjdk]

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

• 2f84480: 8354053: Remove unused JavaIOFilePermissionAccess
• a090aa3: 8354902: Change to Visual Studio 17.13.2 for building on Windows at Oracle
• c0dc314: 8355558: SJIS.java test is always ignored
• ... and 144 more: https://git.openjdk.org/jdk/compare/684d3b336e9cb31707d35e75f9b785e04e1fdbee...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jianglizhou Pushed as commit acd93df.

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

[magicus]

@mcimadamore

Having to upgrade to JNI is a bit sad

I'm not quite sure what you mean that "upgrades" the library to a "JNI library"? Nor why this is sad?

What we do is that we add a marker to identify the library as a built-in library. The name of the marker contains the letters JNI, but that is the only JNI thing about it. As Jiangli says, we have added this marker for all other internal JDK libraries. I agree that the name could have been better, but it seems like a minor detail.

[JornVernee]

@mcimadamore

Having to upgrade to JNI is a bit sad

I'm not quite sure what you mean that "upgrades" the library to a "JNI library"? Nor why this is sad?

JNI libraries are more restrictive than non-JNI libraries, as JNI libraries can only be loaded by a single class loader. The lifetime of the class loader is re-used as the lifetime of the library, and tells us when OnLoad and OnUnload should be called. This is in particular not needed in this case, and gets us away from the loaded-by-single-class-loader restricted, but as Maurizio says, making this library a JNI library is probably okay, since this class should always be loaded by the boot loader.

[magicus]

This was news to me. I'm thinking about what this means in terms of the static build, where all native JDK libraries are "loaded" as soon as the application starts executing. Do we have native libraries in the JDK that are not loaded by the boot loader? If so, have we introduced some corner case effects by marking them with the static JNI symbol? 🤔

[AlanBateman]

Do we have native libraries in the JDK that are not loaded by the boot loader?

There are few, e.g. java.security.jgss, jdk.security.auth and jdk.crypto.cryptoki are mapped to the platform class loader, and jdk.attach mapped to the application class loader.

[jianglizhou]

@magicus For existing JDK JNI native libraries, I think adding DEF_STATIC_JNI_OnLoad (if missing) does not affect the classloader usage associated with the library. The static support for built-in JNI native library affects the native "load" operation (e.g. on Linux dlopen) only (Java_jdk_internal_loader_NativeLibraries_load. Double checking Java_jdk_internal_loader_NativeLibraries_unload, the existing code also makes sure JVM_UnloadLibrary is not called to "unload" (e.g. on Linux dlclose) built-in JNI native library.

The reason why boot loader question/consideration brought up in this thread was because libsyslookup was not associated with the boot loader but now does with the change in src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java.