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

[Erik Joelsson]

On Thu, 3 Jun 2021 16:43:51 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 with a new target base due to a merge or a rebase. The pull request now contains 16 commits:
> 
>  - Merge branch 'master' into symbolLookup
>  - Forgot to add makefile for building shim library
>  - Address review comments
>  - Update test/jdk/java/foreign/TestIntrinsics.java
>    
>    Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
>  - Update test/jdk/java/foreign/valist/VaListTest.java
>    
>    Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
>  - Update test/jdk/java/foreign/TestVarArgs.java
>    
>    Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
>  - Update test/jdk/java/foreign/TestUpcall.java
>    
>    Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
>  - Update test/jdk/java/foreign/TestIllegalLink.java
>    
>    Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
>  - Update test/jdk/java/foreign/TestSymbolLookup.java
>    
>    Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
>  - Update test/jdk/java/foreign/TestDowncall.java
>    
>    Co-authored-by: Paul Sandoz <paul.d.sandoz at googlemail.com>
>  - ... and 6 more: https://git.openjdk.java.net/jdk/compare/52d8215a...2545e2b6

Looks pretty good, just a few comments.

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

> 26: include LibCommon.gmk
> 27: 
> 28: ifeq ($(call isTargetOs, linux), true)

Please indent everything inside the ifeq-block 2 spaces. (See http://openjdk.java.net/groups/build/doc/code-conventions.html)

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

> 32:     CFLAGS := $(CFLAGS_JDKLIB), \
> 33:     CXXFLAGS := $(CXXFLAGS_JDKLIB), \
> 34:     LDFLAGS := -Wl$(COMMA)--no-as-needed -lc -lm -ldl $(LDFLAGS_JDKLIB) $(call SET_SHARED_LIBRARY_ORIGIN), \

Unless you link with any other library in the JDK (typically libjava and/or libjvm), I don't think there is a need for adding SET_SHARED_LIBRARY_ORIGIN.

Please put all the -l* flags in LIBS rather than LDFLAGS.

I also recommend putting any additional flags after the general LDFLAGS_JDKLIB. That way you are guaranteed that your flag takes precedence over anything that may be added to LDFLAGS_JDKLIB in the future.

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

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