RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v2]

[Paul Sandoz]

On Wed, 2 Jun 2021 20:17:20 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch overhauls the library loading mechanism used by the Foreign Linker API. We realized that, while handy, the *default* lookup abstraction (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
>> 
>> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` abstraction, a functional interface. Crucially, `SymbolLookup` does not concern with library loading, only symbol lookup. For this reason, two factories are added:
>> 
>> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to lookup symbols in libraries loaded by current loader
>> * `CLinker::systemLookup` - a more stable replacement for the *default* lookup, which looks for symbols in libc.so (or its equivalent in other platforms). The contents of this lookup are unspecified.
>> 
>> Both factories are *restricted*, so they can only be called when `--enable-native-access` is set.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
>   
>   Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>

Looks good. Just minor comments to accept/reject.

The shim library is an interesting approach. I did wonder if the libvm library could be used, but it might have some weird side-effects or bring in more symbols than necessary.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 138:

> 136:      * <p>
> 137:      * This method is <a href="package-summary.html#restricted"><em>restricted</em></a>.
> 138:      * Restricted method are unsafe, and, if used incorrectly, their use might crash

Suggestion:

     * Restricted methods are unsafe, and, if used incorrectly, their use might crash

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 160:

> 158:      * to allocate structs returned by-value.
> 159:      *
> 160:      * @see SymbolLookup

For JDK code it is more common for `@See` to occur after the parameters/return/throws, and before any `@Since`.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 418:

> 416:     private static class AllocHolder {
> 417: 
> 418:         private static final CLinker linker = getSystemLinker();

Suggestion:

        private static final CLinker SYS_LINKER = getSystemLinker();

test/jdk/java/foreign/SafeFunctionAccessTest.java line 53:

> 51:     );
> 52: 
> 53:     static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

    static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/StdLibTest.java line 158:

> 156:     static class StdLibHelper {
> 157: 
> 158:         final static SymbolLookup LOOKUP;

Suggestion:

        static final SymbolLookup LOOKUP;

test/jdk/java/foreign/TestDowncall.java line 60:

> 58:     }
> 59: 
> 60:     static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

    static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/TestIllegalLink.java line 48:

> 46: public class TestIllegalLink {
> 47: 
> 48:     private static final MemoryAddress dummyTarget = MemoryAddress.ofLong(1);

Suggestion:

    private static final MemoryAddress DUMMY_TARGET = MemoryAddress.ofLong(1);

test/jdk/java/foreign/TestIntrinsics.java line 60:

> 58:     }
> 59: 
> 60:     static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

    static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/TestSymbolLookup.java line 50:

> 48:     }
> 49: 
> 50:     static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

    static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/TestUpcall.java line 69:

> 67:     static CLinker abi = CLinker.getInstance();
> 68: 
> 69:     static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

    static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/TestVarArgs.java line 68:

> 66:     }
> 67: 
> 68:     static final MemoryAddress varargsAddr =

Suggestion:

    static final MemoryAddress VARARGS_ADDR =

test/jdk/java/foreign/valist/VaListTest.java line 74:

> 72:     }
> 73: 
> 74:     static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

    static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

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

Marked as reviewed by psandoz (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4316