On Thu, 3 Jun 2021 16:43:51 GMT, Maurizio Cimadamore
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
> - Update test/jdk/java/foreign/valist/VaListTest.java
>
>Co-authored-by: Paul Sandoz
> - Update test/jdk/java/foreign/TestVarArgs.java
>
>Co-authored-by: Paul Sandoz
> - Update test/jdk/java/foreign/TestUpcall.java
>
>Co-authored-by: Paul Sandoz
> - Update test/jdk/java/foreign/TestIllegalLink.java
>
>Co-authored-by: Paul Sandoz
> - Update test/jdk/java/foreign/TestSymbolLookup.java
>
>Co-authored-by: Paul Sandoz
> - Update test/jdk/java/foreign/TestDowncall.java
>
>Co-authored-by: Paul Sandoz
> - ... 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
