On Wed, 2 Jun 2021 20:17:20 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 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 <jornver...@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
