On Wed, 23 Apr 2025 00:56:18 GMT, Jiangli Zhou <jian...@openjdk.org> wrote:

>> 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.
>
> Jiangli Zhou has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Merge branch 'JDK-8355080' of ssh://github.com/jianglizhou/jdk into 
> JDK-8355080
>  - Address henryjen@ comment:
>    - Remove '#include <jni.h>'.

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

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](https://openjdk.org/jeps/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.

-------------

Marked as reviewed by mcimadamore (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24801#pullrequestreview-2798483910
PR Comment: https://git.openjdk.org/jdk/pull/24801#issuecomment-2834427351

Reply via email to