jdk JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader

Allow support for both .a and .so files in AIX. If .so file is not found, allow fallback to .a extension. JBS Issue: JDK-8319516

Progress

[ ] 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-8319516: Native library suffix impact on the library loading in AIX- Java Class Loader (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17945

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

Using diff file

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

Webrev

Link to Webrev Comment

Feb 21 '24 11:02 suchismith1993

:wave: Welcome back sroy! 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.

Feb 21 '24 11:02 bridgekeeper[bot]

@suchismith1993 To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

build
client
compiler
core-libs
graal
hotspot
hotspot-compiler
hotspot-gc
hotspot-jfr
hotspot-runtime
i18n
ide-support
javadoc
jdk
jmx
kulla
net
nio
security
serviceability
shenandoah

Feb 21 '24 11:02 openjdk[bot]

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

8319516: AIX System::loadLibrary needs support to load a shared library from an archive object

Reviewed-by: mdoerr, mchung

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

fd331ff17330329a656181cb58714f1bd1623fcb: 8325469: Freeze/Thaw code can crash in the presence of OSR frames
9fd78022b19149ade40f92749f0b585ecfd41410: 8325494: C2: Broken graph after not skipping CastII node anymore for Assertion Predicates after JDK-8309902
192ec387bc074b25465decf598a4dd87651cbcbb: 8329595: spurious variable "might not have been initialized" on static final field
03e84178ebfd2ca48b89d65d8f3c291e0c622fb5: 8329948: Remove string template feature
ff3e76fd0caf6e5820d618e3e7b82a1a5d008070: 8330053: JFR: Use LocalDateTime instead ZonedDateTime
811aadd9e77356b294c9820e4d5aede81940537c: 8324683: Unify AttachListener code for Posix platforms
5841cb3b51e45e7c3aaa086e179815fa8184f22d: 8330107: Separate out "awt" libraries from Awt2dLibraries.gmk
89129e3f672d8af9613ad2a72e64322661836c96: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
9445047d059a87d49ed0923b438d2ec49340d78e: 8330163: C2: improve CMoveNode::Value() when condition is always true or false
d2f9a1eb9709dbd8b1e7b0d1c14b7876281d7f23: Merge
... and 267 more: https://git.openjdk.org/jdk/compare/d0a265039a36292d87b249af0e8977982e5acc7b...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TheRealMDoerr, @mlchung) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Mar 13 '24 20:03 openjdk[bot]

/label add core-libs

Mar 14 '24 10:03 suchismith1993

@suchismith1993 The core-libs label was successfully added.

Mar 14 '24 10:03 openjdk[bot]

Webrevs

Mar 14 '24 10:03 mlbridge[bot]

I think ideally the member suffix should be added by System.mapLibraryName since that takes care of mapping the platform agnostic library name to a platform specific string that identifies a library. Is it possible to infer the member name from the library name? (e.g. from clang infer clang(libclang.so.16)).

FWIW, there exists an API that is a more direct proxy for dlopen, which is SymbolLookup.libaryLookup.

Mar 21 '24 19:03 JornVernee

JDK-8313616 added the support of loading library members on AIX in os::dll_load in JDK 22. On AIX, is a shared object always a member object loaded from libname.a(libname.so)? If so, changing mapLibraryName implementation seems to be a proper fix. If you want to load a specific version, it should call ~~loadLibrary~~load("libclang.a(libclang.so.16)") instead.

As @jaikiran suggests, loadLibraryOnlyIfPresent() should return false for AIX as the file does not exist. The library implementation may need to adjust as the current implementation uses a file path name.

Mar 21 '24 20:03 mlchung

os::dll_load already handles loading from .a on AIX when attempting loading .so (see JDK-8320005). For example, if loading libclang.so fails, it attempts to load from libclang.a. This issue is essentially revisiting the fix for JDK-8320005 and needs to understand the right mapping.

Mar 21 '24 21:03 mlchung

This problem seems relatively similar to what happens for versioned library names on e.g. linux distributions - e.g. libclang.so.2. In such cases users are stuck between a rock and a hard place: using System.loadLibrary("libclang") is too little: it will be expanded to libclang.so, and will not be found. But there's no way to pass the version name to loadLibrary, as, to do that, you have to also pass the .so extension, and that doesn't work. So the only option is to use the full absolute name with System::load (not loadLibrary).

My feeling is that it is not possible to "infer" the desired member name (because we don't know which version the member library has), in the same way as, in my system, it is not possible to infer libc.so.6 from just the library name c. As @JornVernee mentioned, this is why SymbolLookup::libraryLookup exists, so that library names can be passed straight to dlopen, w/o further interpretation from the JDK. It seems that at least part of the issue here is that the jextract code loads its own library (libclang) using System::loadLibrary, which doesn't do the right thing on AIX when only given "clang" as the library name. But I'm not exactly sure there's a fix for that at the System::loadLibrary level if dlopen really expects clang.a(libclang.so.16) to be passed as parameter.

In other words, a fix to mapLibraryName only works as long as dlopen on AIX is able to do something with a name that is mechanically inferred, such as clang.a(libclang.so). Is that the case? (I'm pessimistic)

Mar 21 '24 22:03 mcimadamore

(I'm pessimistic)

To summarize: I think that allowing version-specific names (even if surrounded by parenthesis) in System::loadLibrary would be very odd. After all, System::loadLibrary doesn't support versioned names, even on other systems (and adding support for versioned library names across different platforms is a much bigger effort).

For this reason, the only thing that would make sense for loadLibrary to support is clang which will be expanded (by mapLibraryName) to clang(libclang.so). But, even assuming this works: wouldn't we still have an issue? As far as I understand (and given the code in this patch), we don't really know (before calling dlopen) whether the suffix is needed or not: whether it's an archive with an .so inside, or whether it's a plain .so. So how can the behavior of mapLibraryName be deterministic?

Mar 21 '24 22:03 mcimadamore

As @jaikiran suggests, loadLibraryOnlyIfPresent() should return false for AIX as the file does not exist. The library implementation may need to adjust as the current implementation uses a file path name.

Had kept it as true, as it was in J9. but changing it to false works. So that would mean we are letting the java Runtime to handle paths that don't exist ?

Mar 22 '24 09:03 suchismith1993

For this reason, the only thing that would make sense for loadLibrary to support is clang which will be expanded (by mapLibraryName) to clang(libclang.so). But, even assuming this works: wouldn't we still have an issue? As far as I understand (and given the code in this patch), we don't really know (before calling dlopen) whether the suffix is needed or not: whether it's an archive with an .so inside, or whether it's a plain .so. So how can the behavior of mapLibraryName be deterministic?

The behaviour is deterministic when we have a case where, a .so file maps to .a file without specifying any members. This was the original intention for mapAlternateName so that we can use loadLibrary to load shared objects with .so format, and on failure we fallback and check if .a exists.

But to mention the member object, it looks to me that we must specify the full name and there is no direct mapping. So then we restrict that to only System.load and not System.loadLibrary ?

Mar 22 '24 10:03 suchismith1993

This problem seems relatively similar to what happens for versioned library names on e.g. linux distributions - e.g. libclang.so.2. In such cases users are stuck between a rock and a hard place: using System.loadLibrary("libclang") is too little: it will be expanded to libclang.so, and will not be found. But there's no way to pass the version name to loadLibrary, as, to do that, you have to also pass the .so extension, and that doesn't work. So the only option is to use the full absolute name with System::load (not loadLibrary).

My feeling is that it is not possible to "infer" the desired member name (because we don't know which version the member library has), in the same way as, in my system, it is not possible to infer libc.so.6 from just the library name c. As @JornVernee mentioned, this is why SymbolLookup::libraryLookup exists, so that library names can be passed straight to dlopen, w/o further interpretation from the JDK. It seems that at least part of the issue here is that the jextract code loads its own library (libclang) using System::loadLibrary, which doesn't do the right thing on AIX when only given "clang" as the library name. But I'm not exactly sure there's a fix for that at the System::loadLibrary level if dlopen really expects clang.a(libclang.so.16) to be passed as parameter.

In other words, a fix to mapLibraryName only works as long as dlopen on AIX is able to do something with a name that is mechanically inferred, such as clang.a(libclang.so). Is that the case? (I'm pessimistic)

Yes, dlopen expects the full name. If i just pass clang in loadLibrary() and not the member i get exec error. System error: Exec format error

Mar 22 '24 10:03 suchismith1993

dlopen really expects libclang.a(libclang.so.16). I don't think this can currently be passed by any of the functions System.loadLibrary, System.load or SymbolLookup.libraryLookup. The .16 suffix is only for clang 16. Clang 15 uses .15. Note that AIX uses archive members quite often. E.g. libc.a(shr_64.o), libpthreads.a(shr_xpg5_64.o), libz.a(libz.so.1), ... Only the Java developer can select the right member.

Mar 22 '24 14:03 TheRealMDoerr

Only the Java developer can select the right member.

Right, and I think this problem is isomporphic to the problem we have in Linux distros of selecting between libclang.so.1 and libclang.so.2. System::loadLibrary cannot do it. So, I think it would feel odd if, say AIX was able to load precise version of a library using the member syntax, but in all other platforms that would not be possible. For better or worse, the story for now is: if you want version, use System::load.

In this case, the problem might be that System::load doesn't work out of the box, as the path doesn't really exist. But this doesn't seem different to loading non-existing framework paths in MacOS - e.g. it is a special case that can be added for AIX (when using System::load).

All that said, if one is doing System::load/loadLibrary just because they want to use FFM and Linker, a better way to do that is to just use SymbolLookup::libraryLookup directly, which gives you direct access to whatever string dlopen accepts in your system. Now, I realize that jextract is currently using SymbolLookup::loaderLookup, but I believe we can change that, which in turn will make adding support for AIX a bit easier.

Mar 22 '24 15:03 mcimadamore

In case of jextract (jdk22 branch), we would then need something like the following if we want AIX to behave like the other platforms?

diff --git a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
index 14eba30..c069c3b 100644
--- a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
+++ b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
@@ -27,11 +27,13 @@
 
 package org.openjdk.jextract.clang.libclang;
 
+import java.io.File;
 import java.lang.invoke.*;
 import java.lang.foreign.*;
 import java.nio.ByteOrder;
 import java.util.*;
 import java.util.function.*;
+import java.util.StringTokenizer;
 import java.util.stream.*;
 
 import static java.lang.foreign.ValueLayout.*;
@@ -101,8 +103,21 @@ public class Index_h {
     }
 
     static {
-        String libName = System.getProperty("os.name").startsWith("Windows") ? "libclang" : "clang";
-        System.loadLibrary(libName);
+        String osName = System.getProperty("os.name");
+        String libName = "";
+        if (osName.startsWith("AIX")) {
+            String libPath = System.getProperty("java.library.path");
+            StringTokenizer parser = new StringTokenizer(libPath, ":");
+            while (parser.hasMoreTokens()) {
+                libName = parser.nextToken() + "/libclang.a";
+                File f = new File(libName);
+                if (f.exists() && !f.isDirectory()) break;
+            }
+            SymbolLookup.libraryLookup(libName + "(libclang.so.16)", Arena.global());
+        } else {
+            libName = osName.startsWith("Windows") ? "libclang" : "clang";
+            System.loadLibrary(libName);
+        }
     }
 
     static final SymbolLookup SYMBOL_LOOKUP = SymbolLookup.loaderLookup()

If I try this, I get "UnsatisfiedLinkError: unresolved symbol: clang_createIndex". So, seems like the lib gets found, but symbols not loaded. Seems like there are more changes needed.

Mar 22 '24 16:03 TheRealMDoerr

In case of jextract (jdk22 branch), we would then need something like the following if we want AIX to behave like the other platforms?

diff --git a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
index 14eba30..c069c3b 100644
--- a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
+++ b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
@@ -27,11 +27,13 @@
 
 package org.openjdk.jextract.clang.libclang;
 
+import java.io.File;
 import java.lang.invoke.*;
 import java.lang.foreign.*;
 import java.nio.ByteOrder;
 import java.util.*;
 import java.util.function.*;
+import java.util.StringTokenizer;
 import java.util.stream.*;
 
 import static java.lang.foreign.ValueLayout.*;
@@ -101,8 +103,21 @@ public class Index_h {
     }
 
     static {
-        String libName = System.getProperty("os.name").startsWith("Windows") ? "libclang" : "clang";
-        System.loadLibrary(libName);
+        String osName = System.getProperty("os.name");
+        String libName = "";
+        if (osName.startsWith("AIX")) {
+            String libPath = System.getProperty("java.library.path");
+            StringTokenizer parser = new StringTokenizer(libPath, ":");
+            while (parser.hasMoreTokens()) {
+                libName = parser.nextToken() + "/libclang.a";
+                File f = new File(libName);
+                if (f.exists() && !f.isDirectory()) break;
+            }
+            SymbolLookup.libraryLookup(libName + "(libclang.so.16)", Arena.global());
+        } else {
+            libName = osName.startsWith("Windows") ? "libclang" : "clang";
+            System.loadLibrary(libName);
+        }
     }
 
     static final SymbolLookup SYMBOL_LOOKUP = SymbolLookup.loaderLookup()

If I try this, I get "UnsatisfiedLinkError: unresolved symbol: clang_createIndex". So, seems like the lib gets found, but symbols not loaded. Seems like there are more changes needed.

Something like that seems good. When you call "find" on that lookup, it should just forward to dlsym... I suggest you try first to simply use the lookup in a standalone file (w/o jextract) and try to lookup a symbol in clang (e.g. clang_getClangVersion), and see what happens.

If that doesn't work (likely), I'd suggest to write the equivalent C program with dlopen/dlsym, and make sure that works, and also note which DLOPEN/DLSYM parameters are used (maybe those are different from the ones used by the JDK?).

Mar 22 '24 19:03 mcimadamore

Ah, right. Thanks! I should use that SymbolLookup:

diff --git a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
index 14eba30..4f92f43 100644
--- a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
+++ b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
@@ -27,11 +27,13 @@
 
 package org.openjdk.jextract.clang.libclang;
 
+import java.io.File;
 import java.lang.invoke.*;
 import java.lang.foreign.*;
 import java.nio.ByteOrder;
 import java.util.*;
 import java.util.function.*;
+import java.util.StringTokenizer;
 import java.util.stream.*;
 
 import static java.lang.foreign.ValueLayout.*;
@@ -100,14 +102,27 @@ public class Index_h {
         throw new IllegalArgumentException("Invalid type for ABI: " + c.getTypeName());
     }
 
+    static SymbolLookup SYMBOL_LOOKUP;
+
     static {
-        String libName = System.getProperty("os.name").startsWith("Windows") ? "libclang" : "clang";
-        System.loadLibrary(libName);
+        String osName = System.getProperty("os.name");
+        String libName = "";
+        if (osName.startsWith("AIX")) {
+            String libPath = System.getProperty("java.library.path");
+            StringTokenizer parser = new StringTokenizer(libPath, ":");
+            while (parser.hasMoreTokens()) {
+                libName = parser.nextToken() + "/libclang.a";
+                File f = new File(libName);
+                if (f.exists() && !f.isDirectory()) break;
+            }
+            SYMBOL_LOOKUP = SymbolLookup.libraryLookup(libName + "(libclang.so.16)", Arena.global());
+        } else {
+            libName = osName.startsWith("Windows") ? "libclang" : "clang";
+            System.loadLibrary(libName);
+            SYMBOL_LOOKUP = SymbolLookup.loaderLookup().or(Linker.nativeLinker().defaultLookup());
+        }
     }
 
-    static final SymbolLookup SYMBOL_LOOKUP = SymbolLookup.loaderLookup()
-            .or(Linker.nativeLinker().defaultLookup());
-
     public static final ValueLayout.OfBoolean C_BOOL = ValueLayout.JAVA_BOOLEAN;
     public static final ValueLayout.OfByte C_CHAR = ValueLayout.JAVA_BYTE;
     public static final ValueLayout.OfShort C_SHORT = ValueLayout.JAVA_SHORT;

The symbols get found and the JVM can really call into the libclang.a(libclang.so.16). Impressive! (That doesn't mean that jextract is working. clang crashes the way we are calling it. Maybe because of a thread stack size or other memory management problem.) So, there is basically a valid solution for loading such libraries. Only not very smooth for Java programmers.

Mar 22 '24 19:03 TheRealMDoerr

The symbols get found and the JVM can really call into the libclang.a(libclang.so.16). Impressive! (That doesn't mean that jextract is working. clang crashes the way we are calling it. Maybe because of a thread stack size or other memory management problem.) So, there is basically a valid solution for loading such libraries. Only not very smooth for Java programmers.

Note that the jextract code itself depends on bindings generated via jextract (!!). So, I wonder if there might be some incompatibility in the generated layouts/descriptors, which is then causing the crash.The generated bindings are here:

https://github.com/openjdk/jextract/tree/master/src/main/java/org/openjdk/jextract/clang/libclang

These bindings are effectively shared across Linux/x64, Macos/x64, Macos/arm64 and Win/x64 - that is, all the layouts in there are valid and portable on these platforms (except for C_LONG on Windows, but libclang is quite disciplined and only uses long long).

I'd try few steps:

check that your libclang.so is not broken, by calling clang_version in a C program and making sure that works w/o crashing
then try to do the same (w/o jextract) using FFM, from Java, and see if that still works

If even (1) fails, you might just have a bad libclang, or one that is not for your system (not all the binary downloads in the LLVM website worked on my machine, even if they were supposedly compatible).

If (1) succeds, but (2) fails, that would indicate some general issue with libclang and the JVM. There is an issue that we currently have to workaround, where libclang tries to install its own signal handlers, which mess up the JVM's signal handler to deal with NPEs (and that causes a random JVM crash). This is documented here:

https://reviews.llvm.org/D23662

We try to call "setenv" to disable that logic, but that might fail or not be supported on your platform, so worth checking that.

Finally, if (1) and (2) both succeed, but you get spurious JVM crashes with jextract, then I'd start looking at jextract's bindings (the ones in the folder above), pick a struct (e.g. CXCursor, or CXString) and then inspect the layout and make sure that corresponds to what the layout should be in AIX - it is possible that the AIX compiler inserts some extra padding, and then passing structs with the wrong size in and out of libclang would explain the issues.

Hope this helps!

Mar 23 '24 00:03 mcimadamore

I'd like to uplevel the discussion a bit. This PR started off to tweak the way in which System::load worked in AIX. We then discussed a bunch of options, talked about Symbol::libraryLookup and verified that this lookup allows to load libraries as expected in AIX. There's some jextract issues which need to be worked out, but that's outside the scope of this PR.

That said, is there anything that we feel could be improved in terms of library loading support with System::load ? My conclusion was that, given that in this case we needed a fully versioned archive member, it is hard to implement and/or to expose as a simple mapLibraryName add-on. Is that correct?

If you feel that there's not much that System::load can do for these cases, then I'd suggest we close this PR, and perhaps move the discussion about jextract either on jextract-dev or on panama-dev. Thanks!

Mar 23 '24 00:03 mcimadamore

Thanks! Agreed. jextract is crashing in libclang.a::llvm::MemoryBuffer::getMemBufferRef() in a new llvm thread (could be related to the signal handler issue). That seems to be unrelated to this issue and shouldn't be discussed here. It would be nice if loading such libraries could be made easier for Java developers.

Mar 23 '24 12:03 TheRealMDoerr

I'd like to uplevel the discussion a bit. This PR started off to tweak the way in which System::load worked in AIX. We then discussed a bunch of options, talked about Symbol::libraryLookup and verified that this lookup allows to load libraries as expected in AIX. There's some jextract issues which need to be worked out, but that's outside the scope of this PR.

That said, is there anything that we feel could be improved in terms of library loading support with System::load ? My conclusion was that, given that in this case we needed a fully versioned archive member, it is hard to implement and/or to expose as a simple mapLibraryName add-on. Is that correct?

If you feel that there's not much that System::load can do for these cases, then I'd suggest we close this PR, and perhaps move the discussion about jextract either on jextract-dev or on panama-dev. Thanks!

I Feel it is good to at least keep the option for Java developers to use system.load for member objects. Also , the original intention of the PR was to allow .so to .a mapping if .so is not there. This fix had gone into J9 and I think it would be good to keep this support in OpenJDK too. In summary, the functionality to parse members can be kept limited to System.load and the deterministic .so to .a mapping as part of System.loadLibrary().

Mar 23 '24 12:03 suchismith1993

JDK-8313616 changes HotSpot VM to support loading member objects from .a on AIX dlopen. That RFE was a reasonable change as AIX JDK may load native libraries from an archive file of a named member object e.g. /usr/lib/libperfstat.a(shr_64.o)

JDK-8320005 was the first attempt to fix dcstartup and second attempt by this issue in JDK 23. This changes HotSpot VM to load from an alternate path if os::dll_load(lib_path), which in turn calls dlopenon AIX, fails to loadlib_pathwith.sosuffix, it tries to load with an alternatve path by replacing.sowith.a`.

This issue (JDK-8319516) attempts to change the library implementation as follows:

System::loadLibrary("foo") to call dlopen("/usr/lib/libfoo.so") first; if fails, dlopen("/usr/lib/libfoo.a")
System::load("/usr/lib/libfoo.so(libfoo.so.1)" that will call (1) dlopen("/usr/lib/libfoo.so((libfoo.so.1)"), if fails (2) dlopen("/usr/lib/libfoo.a((libfoo.so.1)").

On AIX, loading shared objects from an archive object via System::loadLibrary and System::load never work before.

Do you expect if developers start to package shared objects in the format of archive objects? If so, it would be reasonable to support #1 for compatibility.

For #2, System::loadLibrary("foo(libfoo.so.1)", System::load("/usr/lib/libfoo.so((libfoo.so.1)") and System::load("/usr/lib/libfoo.a((libfoo.so.1)") are never supported. I think applications should use SymbolLookup::libraryLookup instead on Java 22 and later. I don't use libclang as the example here because that's related to jextract and has nothing to do with System::load as Maurizio said.

Mar 25 '24 18:03 mlchung

Do you expect if developers start to package shared objects in the format of archive objects? If so, it would be reasonable to support #1 for compatibility.

Yes. I agree. We should keep this compatibility for entire flow from classloader to the hotspot code. And it has been in J9 for long time for the exact same reason, which actually brought out the issue for dcstartup in OpenJDK.

Mar 25 '24 18:03 suchismith1993

For #2, System::loadLibrary("foo(libfoo.so.1)", System::load("/usr/lib/libfoo.so((libfoo.so.1)") and System::load("/usr/lib/libfoo.a((libfoo.so.1)") are never supported. I think applications should use SymbolLookup::libraryLookup instead on Java 22 and later. I don't use libclang as the example here because that's related to jextract and has nothing to do with System::load as Maurizio said.

I just feel keeping load functionality is still applicable, which does not expect a deterministic mapping (or does it ? ). if loadLibrary expects this to be deterministic, then i agree, we should drop that.

Mar 25 '24 18:03 suchismith1993

And it has been in J9 for long time for the exact same reason, which actually brought out the issue for dcstartup in OpenJDK.

dcstartup fails because it fails to load an agent library specified via -agentlib:am_ibm_16 that was fixed by JDK-8320005. I assume that's what you referred to "J9 had for a long time". This does not use System::loadLibrary. It's unclear if any JNI native library is changed to load from an archive object. Any customer reporting this issue?

I just feel keeping load functionality is still applicable, which does not expect a deterministic mapping (or does it ? ). if loadLibrary expects this to be deterministic, then i agree, we should drop that.

It's not about deterministic mapping or not. System::load is intended for loading JNI native libraries but not for providing equivalent functionality to dlopen.

Mar 25 '24 20:03 mlchung

dcstartup fails because it fails to load an agent library specified via -agentlib:am_ibm_16 that was fixed by JDK-8320005. I assume that's what you referred to "J9 had for a long time". This does not use System::loadLibrary. It's unclear if any JNI native library is changed to load from an archive object. Any customer reporting this issue?

So when this was reported in J9, the fix was made at the classloader level. Thats when i replicated this change to OpenJDK. But on further debug i saw that in this case, directly the hotspot code was called and the classloader was not in picture.

I need to confirm about customer issue again. I will do that in a few days. But is there any particular reason why we don't support loading shared archives via loadLibrary() ? It can be an insurance against future issues too.

Mar 26 '24 09:03 suchismith1993

I think just changing loadLibraryOnlyIfPresent to return false should be sufficient to make System.loadLibrary("foo") loading from "libfoo.a" and System.load("/usr/lib/libfoo.a(libfoo.so)") to work.

Mar 26 '24 21:03 mlchung

@suchismith1993 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

Mar 27 '24 17:03 openjdk[bot]

jdk jdk copied to clipboard

JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader

Progress

Issue

Reviewing

Webrev

Webrevs

jdk
jdk copied to clipboard