Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8265222: revisit foreign library loading #526

Closed

Conversation

@sundararajana
Copy link
Member

@sundararajana sundararajana commented May 5, 2021

LibraryLookup removed. CLinker.findNative added. System.loadLibrary has to be used to load library


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/526/head:pull/526
$ git checkout pull/526

Update a local copy of the PR:
$ git checkout pull/526
$ git pull https://git.openjdk.java.net/panama-foreign pull/526/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 526

View PR using the GUI difftool:
$ git pr show -t 526

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/526.diff

findNative returns Optional<MemoryAddress>. default lookup code removed.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 5, 2021

👋 Welcome back sundar! A progress list of the required criteria for merging this PR into foreign-memaccess+abi will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label May 5, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 5, 2021

Webrevs

EXPORT struct tm *libc_gmtime(const time_t *timer);
EXPORT void libc_qsort(void *base, size_t nitems, size_t size, int (*compar)(const void *, const void*));
EXPORT int libc_rand(void);
EXPORT int vprintf(const char *format, va_list arg);
Copy link
Member

@JornVernee JornVernee May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declaration of vprintf gives an error on Windows, is it needed? The test passes without as well. (should came of stdio.h I'd think)

test\jdk\java\foreign\libStdLibTest.h(43): error C2375: 'vprintf': redefinition; different linkage
c:\progra~2\wi3cf2~1\10\include\100183~1.0\ucrt\stdio.h(738): note: see declaration of 'vprintf'
   ... (rest of output omitted)

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks like a good simplification! Some comments added.

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 5, 2021

Microbenchmarks also need updating - otherwise build fails here:

test/micro/org/openjdk/bench/jdk/incubator/foreign/CallOverheadHelper.java:28: error: cannot find symbol
import jdk.incubator.foreign.LibraryLookup;
                            ^
  symbol:   class LibraryLookup
  location: package jdk.incubator.foreign
test/micro/org/openjdk/bench/jdk/incubator/foreign/StrLenTest.java:30: error: cannot find symbol
import jdk.incubator.foreign.LibraryLookup;
                            ^
  symbol:   class LibraryLookup
  location: package jdk.incubator.foreign
test/micro/org/openjdk/bench/jdk/incubator/foreign/Upcalls.java:26: error: cannot find symbol
import jdk.incubator.foreign.LibraryLookup;
                            ^
  symbol:   class LibraryLookup
  location: package jdk.incubator.foreign
   ... (rest of output omitted)

To fix these, I came up with following patch:

diff --git a/test/micro/org/openjdk/bench/jdk/incubator/foreign/CallOverheadHelper.java b/test/micro/org/openjdk/bench/jdk/incubator/foreign/CallOverheadHelper.java
index 453ab59e2aa..c5c0dea664b 100644
--- a/test/micro/org/openjdk/bench/jdk/incubator/foreign/CallOverheadHelper.java
+++ b/test/micro/org/openjdk/bench/jdk/incubator/foreign/CallOverheadHelper.java
@@ -25,7 +25,6 @@ package org.openjdk.bench.jdk.incubator.foreign;
 import jdk.incubator.foreign.Addressable;
 import jdk.incubator.foreign.CLinker;
 import jdk.incubator.foreign.FunctionDescriptor;
-import jdk.incubator.foreign.LibraryLookup;
 import jdk.incubator.foreign.MemoryAddress;
 import jdk.incubator.foreign.MemoryLayout;
 import jdk.incubator.foreign.MemorySegment;
@@ -91,9 +90,9 @@ public class CallOverheadHelper {
     static {
         System.loadLibrary("CallOverheadJNI");
 
-        LibraryLookup ll = LibraryLookup.ofLibrary("CallOverhead");
+        System.loadLibrary("CallOverhead");
         {
-            func_addr = ll.lookup("func").orElseThrow();
+            func_addr = CLinker.findNative("func").orElseThrow();
             MethodType mt = MethodType.methodType(void.class);
             FunctionDescriptor fd = FunctionDescriptor.ofVoid();
             func_v = abi.downcallHandle(mt, fd);
@@ -102,7 +101,7 @@ public class CallOverheadHelper {
             func_trivial = insertArguments(func_trivial_v, 0, func_addr);
         }
         {
-            identity_addr = ll.lookup("identity").orElseThrow();
+            identity_addr = CLinker.findNative("identity").orElseThrow();
             MethodType mt = MethodType.methodType(int.class, int.class);
             FunctionDescriptor fd = FunctionDescriptor.of(C_INT, C_INT);
             identity_v = abi.downcallHandle(mt, fd);
@@ -110,49 +109,49 @@ public class CallOverheadHelper {
             identity_trivial_v = abi.downcallHandle(mt, fd.withAttribute(FunctionDescriptor.TRIVIAL_ATTRIBUTE_NAME, true));
             identity_trivial = insertArguments(identity_trivial_v, 0, identity_addr);
         }
-        identity_struct_addr = ll.lookup("identity_struct").orElseThrow();
+        identity_struct_addr = CLinker.findNative("identity_struct").orElseThrow();
         identity_struct_v = abi.downcallHandle(
                 MethodType.methodType(MemorySegment.class, MemorySegment.class),
                 FunctionDescriptor.of(POINT_LAYOUT, POINT_LAYOUT));
         identity_struct = insertArguments(identity_struct_v, 0, identity_struct_addr);
 
-        identity_memory_address_addr = ll.lookup("identity_memory_address").orElseThrow();
+        identity_memory_address_addr = CLinker.findNative("identity_memory_address").orElseThrow();
         identity_memory_address_v = abi.downcallHandle(
                 MethodType.methodType(MemoryAddress.class, MemoryAddress.class),
                 FunctionDescriptor.of(C_POINTER, C_POINTER));
         identity_memory_address = insertArguments(identity_memory_address_v, 0, identity_memory_address_addr);
 
-        args1_addr = ll.lookup("args1").orElseThrow();
+        args1_addr = CLinker.findNative("args1").orElseThrow();
         args1_v = abi.downcallHandle(
                 MethodType.methodType(void.class, long.class),
                 FunctionDescriptor.ofVoid(C_LONG_LONG));
         args1 = insertArguments(args1_v, 0, args1_addr);
 
-        args2_addr = ll.lookup("args2").orElseThrow();
+        args2_addr = CLinker.findNative("args2").orElseThrow();
         args2_v = abi.downcallHandle(
                 MethodType.methodType(void.class, long.class, double.class),
                 FunctionDescriptor.ofVoid(C_LONG_LONG, C_DOUBLE));
         args2 = insertArguments(args2_v, 0, args2_addr);
 
-        args3_addr = ll.lookup("args3").orElseThrow();
+        args3_addr = CLinker.findNative("args3").orElseThrow();
         args3_v = abi.downcallHandle(
                 MethodType.methodType(void.class, long.class, double.class, long.class),
                 FunctionDescriptor.ofVoid(C_LONG_LONG, C_DOUBLE, C_LONG_LONG));
         args3 = insertArguments(args3_v, 0, args3_addr);
 
-        args4_addr = ll.lookup("args4").orElseThrow();
+        args4_addr = CLinker.findNative("args4").orElseThrow();
         args4_v = abi.downcallHandle(
                 MethodType.methodType(void.class, long.class, double.class, long.class, double.class),
                 FunctionDescriptor.ofVoid(C_LONG_LONG, C_DOUBLE, C_LONG_LONG, C_DOUBLE));
         args4 = insertArguments(args4_v, 0, args4_addr);
 
-        args5_addr = ll.lookup("args5").orElseThrow();
+        args5_addr = CLinker.findNative("args5").orElseThrow();
         args5_v = abi.downcallHandle(
                 MethodType.methodType(void.class, long.class, double.class, long.class, double.class, long.class),
                 FunctionDescriptor.ofVoid(C_LONG_LONG, C_DOUBLE, C_LONG_LONG, C_DOUBLE, C_LONG_LONG));
         args5 = insertArguments(args5_v, 0, args5_addr);
 
-        args10_addr = ll.lookup("args10").orElseThrow();
+        args10_addr = CLinker.findNative("args10").orElseThrow();
         args10_v = abi.downcallHandle(
                 MethodType.methodType(void.class, long.class, double.class, long.class, double.class, long.class,
                                                   double.class, long.class, double.class, long.class, double.class),
diff --git a/test/micro/org/openjdk/bench/jdk/incubator/foreign/StrLenTest.java b/test/micro/org/openjdk/bench/jdk/incubator/foreign/StrLenTest.java
index 95f3fcd31a7..6129609b2a9 100644
--- a/test/micro/org/openjdk/bench/jdk/incubator/foreign/StrLenTest.java
+++ b/test/micro/org/openjdk/bench/jdk/incubator/foreign/StrLenTest.java
@@ -27,7 +27,6 @@ package org.openjdk.bench.jdk.incubator.foreign;
 
 import jdk.incubator.foreign.CLinker;
 import jdk.incubator.foreign.FunctionDescriptor;
-import jdk.incubator.foreign.LibraryLookup;
 import jdk.incubator.foreign.MemoryAccess;
 import jdk.incubator.foreign.MemoryAddress;
 import jdk.incubator.foreign.MemorySegment;
@@ -78,19 +77,18 @@ public class StrLenTest {
     static final MethodHandle FREE_TRIVIAL;
 
     static {
-        LibraryLookup lookup = LibraryLookup.ofDefault();
         CLinker abi = CLinker.getInstance();
-        STRLEN = abi.downcallHandle(lookup.lookup("strlen").get(),
+        STRLEN = abi.downcallHandle(CLinker.findNative("strlen_raw").get(),
                 MethodType.methodType(int.class, MemoryAddress.class),
                 FunctionDescriptor.of(C_INT, C_POINTER));
-        STRLEN_TRIVIAL = abi.downcallHandle(lookup.lookup("strlen").get(),
+        STRLEN_TRIVIAL = abi.downcallHandle(CLinker.findNative("strlen_raw").get(),
                 MethodType.methodType(int.class, MemoryAddress.class),
                 FunctionDescriptor.of(C_INT, C_POINTER).withAttribute(FunctionDescriptor.TRIVIAL_ATTRIBUTE_NAME, true));
-        MALLOC_TRIVIAL = abi.downcallHandle(lookup.lookup("malloc").get(),
+        MALLOC_TRIVIAL = abi.downcallHandle(CLinker.findNative("malloc_raw").get(),
                 MethodType.methodType(MemoryAddress.class, long.class),
                 FunctionDescriptor.of(C_POINTER, C_LONG_LONG).withAttribute(FunctionDescriptor.TRIVIAL_ATTRIBUTE_NAME, true));
 
-        FREE_TRIVIAL = abi.downcallHandle(lookup.lookup("free").get(),
+        FREE_TRIVIAL = abi.downcallHandle(CLinker.findNative("free_raw").get(),
                 MethodType.methodType(void.class, MemoryAddress.class),
                 FunctionDescriptor.ofVoid(C_POINTER).withAttribute(FunctionDescriptor.TRIVIAL_ATTRIBUTE_NAME, true));
     }
diff --git a/test/micro/org/openjdk/bench/jdk/incubator/foreign/Upcalls.java b/test/micro/org/openjdk/bench/jdk/incubator/foreign/Upcalls.java
index af30a0c326c..aaae4a9fd38 100644
--- a/test/micro/org/openjdk/bench/jdk/incubator/foreign/Upcalls.java
+++ b/test/micro/org/openjdk/bench/jdk/incubator/foreign/Upcalls.java
@@ -23,7 +23,6 @@
 package org.openjdk.bench.jdk.incubator.foreign;
 
 import jdk.incubator.foreign.FunctionDescriptor;
-import jdk.incubator.foreign.LibraryLookup;
 import jdk.incubator.foreign.MemoryAddress;
 import jdk.incubator.foreign.CLinker;
 import jdk.incubator.foreign.ResourceScope;
@@ -80,13 +79,13 @@ public class Upcalls {
         cb_args10_jni = makeCB(className, "args10", "(JDJDJDJDJD)V");
 
         try {
-            LibraryLookup ll = LibraryLookup.ofLibrary("Upcalls");
+            System.loadLibrary("Upcalls");
             {
                 String name = "blank";
                 MethodType mt = MethodType.methodType(void.class);
                 FunctionDescriptor fd = FunctionDescriptor.ofVoid();
 
-                blank = linkFunc(ll, name, mt, fd);
+                blank = linkFunc(name, mt, fd);
                 cb_blank = makeCB(name, mt, fd);
             }
             {
@@ -94,7 +93,7 @@ public class Upcalls {
                 MethodType mt = MethodType.methodType(int.class, int.class);
                 FunctionDescriptor fd = FunctionDescriptor.of(C_INT, C_INT);
 
-                identity = linkFunc(ll, name, mt, fd);
+                identity = linkFunc(name, mt, fd);
                 cb_identity = makeCB(name, mt, fd);
             }
             {
@@ -104,7 +103,7 @@ public class Upcalls {
                 FunctionDescriptor fd = FunctionDescriptor.ofVoid(
                         C_LONG_LONG, C_DOUBLE, C_LONG_LONG, C_DOUBLE, C_LONG_LONG);
 
-                args5 = linkFunc(ll, name, mt, fd);
+                args5 = linkFunc(name, mt, fd);
                 cb_args5 = makeCB(name, mt, fd);
             }
             {
@@ -116,7 +115,7 @@ public class Upcalls {
                         C_LONG_LONG, C_DOUBLE, C_LONG_LONG, C_DOUBLE, C_LONG_LONG,
                         C_DOUBLE, C_LONG_LONG, C_DOUBLE, C_LONG_LONG, C_DOUBLE);
 
-                args10 = linkFunc(ll, name, mt, fd);
+                args10 = linkFunc(name, mt, fd);
                 cb_args10 = makeCB(name, mt, fd);
             }
         } catch (ReflectiveOperationException e) {
@@ -124,9 +123,9 @@ public class Upcalls {
         }
     }
 
-    static MethodHandle linkFunc(LibraryLookup ll, String name, MethodType baseType, FunctionDescriptor baseDesc) {
+    static MethodHandle linkFunc(String name, MethodType baseType, FunctionDescriptor baseDesc) {
         return abi.downcallHandle(
-            ll.lookup(name).orElseThrow(),
+            CLinker.findNative(name).orElseThrow(),
             baseType.insertParameterTypes(baseType.parameterCount(), MemoryAddress.class),
             baseDesc.withAppendedArgumentLayouts(C_POINTER)
         );
diff --git a/test/micro/org/openjdk/bench/jdk/incubator/foreign/VaList.java b/test/micro/org/openjdk/bench/jdk/incubator/foreign/VaList.java
index 60c8a8b56f5..6b8a56a14a0 100644
--- a/test/micro/org/openjdk/bench/jdk/incubator/foreign/VaList.java
+++ b/test/micro/org/openjdk/bench/jdk/incubator/foreign/VaList.java
@@ -23,7 +23,6 @@
 package org.openjdk.bench.jdk.incubator.foreign;
 
 import jdk.incubator.foreign.FunctionDescriptor;
-import jdk.incubator.foreign.LibraryLookup;
 import jdk.incubator.foreign.CLinker;
 import jdk.incubator.foreign.ResourceScope;
 import org.openjdk.jmh.annotations.Benchmark;
@@ -54,16 +53,18 @@ import static jdk.incubator.foreign.CLinker.asVarArg;
 public class VaList {
 
     static final CLinker linker = CLinker.getInstance();
-    static final LibraryLookup lookup = LibraryLookup.ofLibrary("VaList");
+    static {
+        System.loadLibrary("VaList");
+    }
 
     static final MethodHandle MH_ellipsis;
     static final MethodHandle MH_vaList;
 
     static {
-        MH_ellipsis = linker.downcallHandle(lookup.lookup("ellipsis").get(),
+        MH_ellipsis = linker.downcallHandle(CLinker.findNative("ellipsis").get(),
                 MethodType.methodType(void.class, int.class, int.class, double.class, long.class),
                 FunctionDescriptor.ofVoid(C_INT, asVarArg(C_INT), asVarArg(C_DOUBLE), asVarArg(C_LONG_LONG)));
-        MH_vaList = linker.downcallHandle(lookup.lookup("vaList").get(),
+        MH_vaList = linker.downcallHandle(CLinker.findNative("vaList").get(),
                 MethodType.methodType(void.class, int.class, VaList.class),
                 FunctionDescriptor.ofVoid(C_INT, C_VA_LIST));
     }
diff --git a/test/micro/org/openjdk/bench/jdk/incubator/foreign/libStrLen.c b/test/micro/org/openjdk/bench/jdk/incubator/foreign/libStrLen.c
index ebbc2f2caa1..6556cc065ad 100644
--- a/test/micro/org/openjdk/bench/jdk/incubator/foreign/libStrLen.c
+++ b/test/micro/org/openjdk/bench/jdk/incubator/foreign/libStrLen.c
@@ -27,9 +27,27 @@
 #include <stdlib.h>
 #include <string.h>
 
+#ifdef _WIN64
+#define EXPORT __declspec(dllexport)
+#else
+#define EXPORT
+#endif
+
 JNIEXPORT jint JNICALL Java_org_openjdk_bench_jdk_incubator_foreign_StrLenTest_strlen(JNIEnv *const env, const jclass cls, const jstring text) {
     const char *str = (*env)->GetStringUTFChars(env, text, NULL);
     int len = (int)strlen(str);
     (*env)->ReleaseStringUTFChars(env, text, str);
     return len;
 }
+
+EXPORT int strlen_raw(const char *str) {
+    return (int)strlen(str);
+}
+
+EXPORT void* malloc_raw(size_t size) {
+    return malloc(size);
+}
+
+EXPORT void free_raw(void* ptr) {
+    return free(ptr);
+}
diff --git a/test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java b/test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
index e8eee07a63d..0791ad75f3b 100644
--- a/test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
+++ b/test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
@@ -23,7 +23,6 @@
 package org.openjdk.bench.jdk.incubator.foreign.points.support;
 
 import jdk.incubator.foreign.FunctionDescriptor;
-import jdk.incubator.foreign.LibraryLookup;
 import jdk.incubator.foreign.MemoryAddress;
 import jdk.incubator.foreign.MemoryLayout;
 import jdk.incubator.foreign.MemorySegment;
@@ -51,14 +50,14 @@ public class PanamaPoint implements AutoCloseable {
 
     static {
         CLinker abi = CLinker.getInstance();
-        LibraryLookup lookup = LibraryLookup.ofLibrary("Point");
+        System.loadLibrary("Point");
         MH_distance = abi.downcallHandle(
-            lookup.lookup("distance").get(),
+            CLinker.findNative("distance").get(),
             methodType(double.class, MemorySegment.class, MemorySegment.class),
             FunctionDescriptor.of(C_DOUBLE, LAYOUT, LAYOUT)
         );
         MH_distance_ptrs = abi.downcallHandle(
-            lookup.lookup("distance_ptrs").get(),
+                CLinker.findNative("distance_ptrs").get(),
             methodType(double.class, MemoryAddress.class, MemoryAddress.class),
             FunctionDescriptor.of(C_DOUBLE, C_POINTER, C_POINTER)
         );

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks good - added some javadoc suggestions

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Some more javadoc comments

sundararajana and others added 2 commits May 6, 2021
…package-info.java

Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
…CLinker.java

Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2021

@sundararajana This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8265222: revisit foreign library loading

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the foreign-memaccess+abi branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the foreign-memaccess+abi branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 6, 2021
@sundararajana
Copy link
Member Author

@sundararajana sundararajana commented May 6, 2021

/integrate

@openjdk openjdk bot closed this May 6, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2021

@sundararajana Pushed as commit e8043ed.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@sundararajana sundararajana deleted the JDK-8265222 branch May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants