[foreign-memaccess+abi] RFR: 8266814: Improve library loading with SymbolLookup abstraction

[Jorn Vernee]

On Mon, 10 May 2021 13:55:44 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This patch implements the library loading abstraction described in:
> 
> https://mail.openjdk.java.net/pipermail/panama-dev/2021-May/013684.html
> 
> That is, a functional interface called `SymbolLookup`, and a couple of factories to get a lookup for a given classloader, and to get a *system* lookup, useful to lookup C symbols.
> 
> To implement the system lookup, we load `msvcrt.dll` on Windows, while we build and load an empty library (which depends on libc) on Mac/Linux. This approach is better than relying on RTLD_DEFAULT (which can sometimes leak symbols from libraries loaded independently). Also, doing this bypasses the problem of figuring out the location of libc, which, on Linux system is particularly gnarly, because of the multi-arch support.

make/modules/jdk.incubator.foreign/Lib.gmk line 41:

> 39: ))
> 40: 
> 41: else ifeq ($(call isTargetOs, windows), false)

I feel like this code, and maybe also the associated code in SystemLookup that loads msvcrt.dll could do with a comment that explains why the same strategy is not used on Windows; namely that symbol lookup on Windows does not search a library's dependencies, as opposed to dlsym, so it's not as easy to re-export the symbols in msvcrt by creating a shim library.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SymbolLookup.java line 47:

> 45:      *
> 46:      * @param name the symbol name.
> 47:      * @return the memory address associated with the library symbol (if any).

I think we want to remove library-centric text here?
Suggestion:

     * @return the memory address associated with the symbol (if any).

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SymbolLookup.java line 60:

> 58:      * restricted methods, and use safe and supported functionalities, where possible.
> 59:      *
> 60:      * @return a symbol lookup suitable to find symbols in libraries loaded by the caller's classloader.

Should this include an `@throws` for the ICE that can be thrown the native access check?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SystemLookup.java line 45:

> 43:     final NativeLibrary syslookup = switch (CABI.current()) {
> 44:         case SysV, AArch64 -> NativeLibraries.rawNativeLibraries(SystemLookup.class, false).loadLibrary("syslookup");
> 45:         case Win64 -> NativeLibraries.rawNativeLibraries(SystemLookup.class, false)

e.g. a short comment here that explains why Windows is different.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SystemLookup.java line 46:

> 44:         case SysV, AArch64 -> NativeLibraries.rawNativeLibraries(SystemLookup.class, false).loadLibrary("syslookup");
> 45:         case Win64 -> NativeLibraries.rawNativeLibraries(SystemLookup.class, false)
> 46:                 .loadLibrary(Path.of(System.getenv("SystemRoot"), "System32", "msvcrt.dll").toString());

Looks like this loadLibrary call does not except paths:


assert name.indexOf(File.separatorChar) < 0;


So this is causing an assertion error on Windows. I'll try to find a fix.

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

PR: https://git.openjdk.java.net/panama-foreign/pull/531