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