From 6e1a80666033f0428156f24d464d35441e77fd57 Mon Sep 17 00:00:00 2001
From: Lukas Stadler <lukas.stadler@oracle.com>
Date: Tue, 14 Nov 2017 10:04:58 +0100
Subject: [PATCH] fix typos and minor problems, add TODOs in FFI implementation

---
 .../truffle/r/ffi/impl/llvm/TruffleLLVM_Base.java     |  6 +++---
 .../truffle/r/ffi/impl/managed/Managed_Base.java      |  2 +-
 .../truffle/r/ffi/impl/nfi/TruffleNFI_Base.java       |  6 +++---
 .../truffle/r/ffi/impl/nfi/TruffleNFI_Context.java    |  4 ++--
 .../r/ffi/impl/nfi/TruffleNFI_UpCallsRFFIImpl.java    | 11 +++++++++--
 .../oracle/truffle/r/ffi/impl/nodes/RfEvalNode.java   |  2 +-
 .../oracle/truffle/r/ffi/processor/FFIProcessor.java  | 11 +++--------
 .../base/foreign/CallAndExternalFunctions.java        |  5 +++--
 .../builtin/base/foreign/FortranAndCFunctions.java    |  1 +
 .../truffle/r/runtime/data/NativeDataAccess.java      |  1 +
 .../com/oracle/truffle/r/runtime/ffi/BaseRFFI.java    |  8 ++++----
 .../src/com/oracle/truffle/r/runtime/ffi/CRFFI.java   |  2 ++
 .../com/oracle/truffle/r/runtime/ffi/RFFIContext.java |  9 ++++++++-
 .../truffle/r/test/generate/GnuROneShotRSession.java  |  3 ---
 14 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/llvm/TruffleLLVM_Base.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/llvm/TruffleLLVM_Base.java
index 4b8ba51e9c..4b1ce29f03 100644
--- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/llvm/TruffleLLVM_Base.java
+++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/llvm/TruffleLLVM_Base.java
@@ -172,7 +172,7 @@ public class TruffleLLVM_Base implements BaseRFFI {
         }
     }
 
-    private static class TruffleLLVM_StrolNode extends TruffleLLVM_DownCallNode implements StrolNode {
+    private static class TruffleLLVM_StrtolNode extends TruffleLLVM_DownCallNode implements StrtolNode {
 
         @Override
         protected NativeFunction getFunction() {
@@ -259,8 +259,8 @@ public class TruffleLLVM_Base implements BaseRFFI {
     }
 
     @Override
-    public StrolNode createStrolNode() {
-        return new TruffleLLVM_StrolNode();
+    public StrtolNode createStrtolNode() {
+        return new TruffleLLVM_StrtolNode();
     }
 
     @Override
diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/managed/Managed_Base.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/managed/Managed_Base.java
index 20c7d3f17d..185cf45f55 100644
--- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/managed/Managed_Base.java
+++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/managed/Managed_Base.java
@@ -155,7 +155,7 @@ public class Managed_Base implements BaseRFFI {
     }
 
     @Override
-    public StrolNode createStrolNode() {
+    public StrtolNode createStrtolNode() {
         return null;
     }
 
diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Base.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Base.java
index ffd4b48ce0..7628b86c76 100644
--- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Base.java
+++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Base.java
@@ -165,7 +165,7 @@ public class TruffleNFI_Base implements BaseRFFI {
         }
     }
 
-    private static class TruffleNFI_StrolNode extends TruffleNFI_DownCallNode implements StrolNode {
+    private static class TruffleNFI_StrtolNode extends TruffleNFI_DownCallNode implements StrtolNode {
 
         @Override
         protected NativeFunction getFunction() {
@@ -250,8 +250,8 @@ public class TruffleNFI_Base implements BaseRFFI {
     }
 
     @Override
-    public StrolNode createStrolNode() {
-        return new TruffleNFI_StrolNode();
+    public StrtolNode createStrtolNode() {
+        return new TruffleNFI_StrtolNode();
     }
 
     @Override
diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Context.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Context.java
index 126d085089..0da0076b08 100644
--- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Context.java
+++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Context.java
@@ -190,8 +190,8 @@ final class TruffleNFI_Context extends RFFIContext {
         try {
             Node bind = Message.createInvoke(1).createNode();
             Node executeNode = Message.createExecute(1).createNode();
-            TruffleObject addCallbackFunction = (TruffleObject) ForeignAccess.sendInvoke(bind, DLL.findSymbol("Rinternals_getCallbacksAddress", null).asTruffleObject(), "bind", "(): sint64");
-            callbacksAddress = (long) ForeignAccess.sendExecute(executeNode, addCallbackFunction);
+            TruffleObject getCallbacksAddressFunction = (TruffleObject) ForeignAccess.sendInvoke(bind, DLL.findSymbol("Rinternals_getCallbacksAddress", null).asTruffleObject(), "bind", "(): sint64");
+            callbacksAddress = (long) ForeignAccess.sendExecute(executeNode, getCallbacksAddressFunction);
         } catch (InteropException ex) {
             throw RInternalError.shouldNotReachHere(ex);
         }
diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_UpCallsRFFIImpl.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_UpCallsRFFIImpl.java
index 80e7535bfb..81e786669c 100644
--- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_UpCallsRFFIImpl.java
+++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_UpCallsRFFIImpl.java
@@ -92,6 +92,8 @@ public class TruffleNFI_UpCallsRFFIImpl extends JavaUpCallsRFFIImpl {
             }
         }
 
+        // TODO: with separate version of this for the different types, it would be more efficient
+        // and not need the dispatch
         public abstract static class DispatchAllocate extends Node {
             private static final long EMPTY_DATA_ADDRESS = 0x1BAD;
 
@@ -128,8 +130,9 @@ public class TruffleNFI_UpCallsRFFIImpl extends JavaUpCallsRFFIImpl {
             }
 
             @Specialization
-            protected static long get(RNull nullValue) {
-                // Note: GnuR is OK with e.g. INTEGER(RNull), it probably returns some garbage
+            protected static long get(@SuppressWarnings("unused") RNull nullValue) {
+                // Note: GnuR is OK with, e.g., INTEGER(NULL), but it's illegal to read from or
+                // write to the resulting address.
                 return EMPTY_DATA_ADDRESS;
             }
 
@@ -139,6 +142,8 @@ public class TruffleNFI_UpCallsRFFIImpl extends JavaUpCallsRFFIImpl {
             }
         }
 
+        // TODO: "TO_NATIVE" should do the actual work of transferring the data
+
         @Resolve(message = "AS_POINTER")
         public abstract static class IntVectorWrapperNativeAsPointerNode extends Node {
             @Child private DispatchAllocate dispatch = DispatchAllocateNodeGen.create();
@@ -151,6 +156,8 @@ public class TruffleNFI_UpCallsRFFIImpl extends JavaUpCallsRFFIImpl {
             }
         }
 
+        // TODO: "READ", "WRITE"
+
         @CanResolve
         public abstract static class VectorWrapperCheck extends Node {
             protected static boolean test(TruffleObject receiver) {
diff --git a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nodes/RfEvalNode.java b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nodes/RfEvalNode.java
index 2ed5333a7a..da3d249e0d 100644
--- a/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nodes/RfEvalNode.java
+++ b/com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nodes/RfEvalNode.java
@@ -110,7 +110,7 @@ public abstract class RfEvalNode extends FFIUpCallNode.Arg2 {
     }
 
     @TruffleBoundary
-    private Object evalFunction(RFunction f, REnvironment env, ArgumentsSignature argsNames, Object... args) {
+    private static Object evalFunction(RFunction f, REnvironment env, ArgumentsSignature argsNames, Object... args) {
         return RContext.getEngine().evalFunction(f, env == REnvironment.globalEnv() ? null : env.getFrame(), RCaller.topLevel, true, argsNames, args);
     }
 
diff --git a/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java b/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java
index ef15cc0dbd..a50310fc8e 100644
--- a/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java
+++ b/com.oracle.truffle.r.ffi.processor/src/com/oracle/truffle/r/ffi/processor/FFIProcessor.java
@@ -293,6 +293,7 @@ public final class FFIProcessor extends AbstractProcessor {
             }
 
         }
+        // TODO: turn this field into "@Child private", initialize lazily
         w.append("                HandleUpCallExceptionNode handleExceptionNode = HandleUpCallExceptionNode.create();");
         w.append("\n");
         w.append("                @Override\n");
@@ -384,17 +385,11 @@ public final class FFIProcessor extends AbstractProcessor {
         w.append("    }\n\n");
 
         w.append("    public static void createCalls(UpCallsRFFI upCallsRFFIImpl) {\n");
-        w.append("        for (Callbacks callback : values()) {\n");
-        w.append("            switch (callback) {\n");
         for (int i = 0; i < methods.length; i++) {
             ExecutableElement m = methods[i];
-            String callName = m.getSimpleName().toString() + "Call";
-            w.append("                case ").append(m.getSimpleName().toString()).append(":\n");
-            w.append("                    callback.call = new ").append(callName).append("(upCallsRFFIImpl);\n");
-            w.append("                    break;\n\n");
+            String callName = m.getSimpleName().toString();
+            w.append("        ").append(callName).append(".call = new ").append(callName).append("Call(upCallsRFFIImpl);\n");
         }
-        w.append("            }\n");
-        w.append("        }\n");
         w.append("    }\n");
         w.append("}\n");
         w.close();
diff --git a/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/foreign/CallAndExternalFunctions.java b/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/foreign/CallAndExternalFunctions.java
index 6a3bd609f5..10d7ed90de 100644
--- a/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/foreign/CallAndExternalFunctions.java
+++ b/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/foreign/CallAndExternalFunctions.java
@@ -581,8 +581,6 @@ public class CallAndExternalFunctions {
                 case "pacf1":
                 case "ar2ma":
                 case "Burg":
-                case "intgrt_vec":
-                    return PPSum.IntgrtVecNode.create();
                 case "pp_sum":
                 case "Fexact":
                 case "Fisher_sim":
@@ -590,6 +588,9 @@ public class CallAndExternalFunctions {
                 case "d2x2xk":
                     return new UnimplementedExternal(name);
 
+                case "intgrt_vec":
+                    return PPSum.IntgrtVecNode.create();
+
                 case "updateform":
                     return getExternalModelBuiltinNode("updateform");
 
diff --git a/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/foreign/FortranAndCFunctions.java b/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/foreign/FortranAndCFunctions.java
index a4a9546852..cc83d8dbe5 100644
--- a/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/foreign/FortranAndCFunctions.java
+++ b/com.oracle.truffle.r.nodes.builtin/src/com/oracle/truffle/r/nodes/builtin/base/foreign/FortranAndCFunctions.java
@@ -69,6 +69,7 @@ public class FortranAndCFunctions {
      */
     @RBuiltin(name = ".Fortran", kind = PRIMITIVE, parameterNames = {".NAME", "...", "NAOK", "DUP", "PACKAGE", "ENCODING"}, behavior = COMPLEX)
     public abstract static class Fortran extends CRFFIAdapter implements Lookup {
+        // TODO: rename to DotFortran
 
         static {
             Casts.noCasts(Fortran.class);
diff --git a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java
index ae2352a003..efe8ee17d0 100644
--- a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java
+++ b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/data/NativeDataAccess.java
@@ -160,6 +160,7 @@ public final class NativeDataAccess {
             assert this.length == 0 || dataAddress != EMPTY_DATA_ADDRESS;
         }
 
+        // TODO: turn this into reference queues
         @Override
         protected void finalize() throws Throwable {
             super.finalize();
diff --git a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/BaseRFFI.java b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/BaseRFFI.java
index 575bfeeaa5..ed2fdc7b53 100644
--- a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/BaseRFFI.java
+++ b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/BaseRFFI.java
@@ -116,14 +116,14 @@ public interface BaseRFFI {
         }
     }
 
-    interface StrolNode extends NodeInterface {
+    interface StrtolNode extends NodeInterface {
         /**
          * Convert string to long.
          */
         long execute(String s, int base) throws IllegalArgumentException;
 
-        static StrolNode create() {
-            return RFFIFactory.getBaseRFFI().createStrolNode();
+        static StrtolNode create() {
+            return RFFIFactory.getBaseRFFI().createStrtolNode();
         }
     }
 
@@ -183,7 +183,7 @@ public interface BaseRFFI {
 
     ChmodNode createChmodNode();
 
-    StrolNode createStrolNode();
+    StrtolNode createStrtolNode();
 
     UnameNode createUnameNode();
 
diff --git a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/CRFFI.java b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/CRFFI.java
index 0126a9fdfe..1175bc95eb 100644
--- a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/CRFFI.java
+++ b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/CRFFI.java
@@ -96,6 +96,8 @@ class TemporaryWrapperMR {
 
     @Resolve(message = "WRITE")
     public abstract static class TemporaryWrapperWriteNode extends Node {
+        // TODO: maybe this should lazily create a copy?
+
         protected Object access(TemporaryWrapper receiver, long index, Object value) {
             receiver.write(index, value);
             return value;
diff --git a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java
index d70d6a0cc4..3e9013d19e 100644
--- a/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java
+++ b/com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/ffi/RFFIContext.java
@@ -81,6 +81,10 @@ public abstract class RFFIContext extends RFFI {
 
     public abstract TruffleObject lookupNativeFunction(NativeFunction function);
 
+    /**
+     * @param canRunGc {@code true} if this upcall can cause a gc on GNU R, and therefore can clear
+     *            the list of preserved objects.
+     */
     public void beforeUpcall(boolean canRunGc) {
     }
 
@@ -95,6 +99,9 @@ public abstract class RFFIContext extends RFFI {
         return 0;
     }
 
+    /**
+     * @param before the value returned by the corresponding call to {@link #beforeDowncall()}.
+     */
     public void afterDowncall(long before) {
         callDepth--;
         cooperativeGc();
@@ -104,7 +111,7 @@ public abstract class RFFIContext extends RFFI {
         return callDepth;
     }
 
-    // this emulates the GNUR's cooperative GC
+    // this emulates GNUR's cooperative GC
     @TruffleBoundary
     private void cooperativeGc() {
         protectedNativeReferences.clear();
diff --git a/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/generate/GnuROneShotRSession.java b/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/generate/GnuROneShotRSession.java
index 26d783f600..56b29da808 100644
--- a/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/generate/GnuROneShotRSession.java
+++ b/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/generate/GnuROneShotRSession.java
@@ -26,10 +26,8 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.nio.file.FileSystems;
-import java.nio.file.Path;
 import java.util.concurrent.TimeUnit;
 
-import com.oracle.truffle.r.launcher.RVersionNumber;
 import com.oracle.truffle.r.runtime.ProcessOutputManager;
 import com.oracle.truffle.r.runtime.REnvVars;
 import com.oracle.truffle.r.runtime.context.ChildContextInfo;
@@ -48,7 +46,6 @@ public class GnuROneShotRSession implements RSession {
 
     private static final String[] GNUR_COMMANDLINE = new String[]{"<R>", "--vanilla", "--slave", "--silent", "--no-restore"};
     private static final String FASTR_TESTGEN_GNUR = "FASTR_TESTGEN_GNUR";
-    private static final String NATIVE_PROJECT = "com.oracle.truffle.r.native";
     private static final int DEFAULT_TIMEOUT_MINS = 5;
     private static int timeoutMins = DEFAULT_TIMEOUT_MINS;
 
-- 
GitLab