From 53decd075367dac701c06a61dba72f5124e22377 Mon Sep 17 00:00:00 2001
From: Mick Jordan <mick.jordan@oracle.com>
Date: Fri, 17 Jun 2016 09:31:40 -0700
Subject: [PATCH] Handle errors properly in R_tryEval

---
 .../fficall/src/jni/Rinternals.c              | 21 ++++---------------
 .../r/runtime/ffi/jnr/CallRFFIHelper.java     | 17 ++++-----------
 .../packages/testrffi/testrffi/src/testrffi.c | 12 ++++++++++-
 3 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/com.oracle.truffle.r.native/fficall/src/jni/Rinternals.c b/com.oracle.truffle.r.native/fficall/src/jni/Rinternals.c
index 5151b22ef3..9793421c1d 100644
--- a/com.oracle.truffle.r.native/fficall/src/jni/Rinternals.c
+++ b/com.oracle.truffle.r.native/fficall/src/jni/Rinternals.c
@@ -87,10 +87,7 @@ static jmethodID OBJECT_MethodID;
 static jmethodID DUPLICATE_ATTRIB_MethodID;
 static jmethodID isS4ObjectMethodID;
 static jmethodID logObject_MethodID;
-static jclass TryEvalResultClass;
 static jmethodID R_tryEvalMethodID;
-static jfieldID TryEvalResultValueFieldID;
-static jfieldID TryEvalResultErrorFieldID;
 
 static jclass RExternalPtrClass;
 static jmethodID createExternalPtrMethodID;
@@ -170,10 +167,7 @@ void init_internals(JNIEnv *env) {
 	DUPLICATE_ATTRIB_MethodID = checkGetMethodID(env, CallRFFIHelperClass, "DUPLICATE_ATTRIB", "(Ljava/lang/Object;Ljava/lang/Object;)V", 1);
 	isS4ObjectMethodID = checkGetMethodID(env, CallRFFIHelperClass, "isS4Object", "(Ljava/lang/Object;)I", 1);
 	logObject_MethodID = checkGetMethodID(env, CallRFFIHelperClass, "logObject", "(Ljava/lang/Object;)V", 1);
-	TryEvalResultClass = checkFindClass(env, "com/oracle/truffle/r/runtime/ffi/jnr/CallRFFIHelper$TryEvalResult");
-	R_tryEvalMethodID = checkGetMethodID(env, CallRFFIHelperClass, "R_tryEval", "(Ljava/lang/Object;Ljava/lang/Object;Z)Lcom/oracle/truffle/r/runtime/ffi/jnr/CallRFFIHelper$TryEvalResult;", 1);
-	TryEvalResultValueFieldID = checkGetFieldID(env, TryEvalResultClass, "value", "Ljava/lang/Object;", 0);
-	TryEvalResultErrorFieldID = checkGetFieldID(env, TryEvalResultClass, "error", "Z", 0);
+	R_tryEvalMethodID = checkGetMethodID(env, CallRFFIHelperClass, "R_tryEval", "(Ljava/lang/Object;Ljava/lang/Object;Z)Ljava/lang/Object;", 1);
 
 	RExternalPtrClass = checkFindClass(env, "com/oracle/truffle/r/runtime/data/RExternalPtr");
 	createExternalPtrMethodID = checkGetMethodID(env, RDataFactoryClass, "createExternalPtr", "(JLjava/lang/Object;Ljava/lang/Object;)Lcom/oracle/truffle/r/runtime/data/RExternalPtr;", 1);
@@ -1380,20 +1374,13 @@ SEXP Rf_asS4(SEXP x, Rboolean b, int i) {
 static SEXP R_tryEvalInternal(SEXP x, SEXP y, int *ErrorOccurred, jboolean silent) {
 	JNIEnv *thisenv = getEnv();
 	jobject tryResult =  (*thisenv)->CallStaticObjectMethod(thisenv, CallRFFIHelperClass, R_tryEvalMethodID, x, y, silent);
-	SEXP value = (*thisenv)->GetObjectField(thisenv, tryResult, TryEvalResultValueFieldID);
-	jboolean error = (*thisenv)->GetBooleanField(thisenv, tryResult, TryEvalResultErrorFieldID);
+	// If tryResult is NULL, an error occurred
 	if (ErrorOccurred) {
-		*ErrorOccurred = error = JNI_TRUE;
-	}
-	if (error == JNI_TRUE) {
-		return NULL;
-	} else {
-		return value;
+		*ErrorOccurred = tryResult == NULL;
 	}
+	return tryResult;
 }
 
-
-
 SEXP R_tryEval(SEXP x, SEXP y, int *ErrorOccurred) {
 	return R_tryEvalInternal(x, y, ErrorOccurred, JNI_FALSE);
 }
diff --git a/com.oracle.truffle.r.runtime.ffi/src/com/oracle/truffle/r/runtime/ffi/jnr/CallRFFIHelper.java b/com.oracle.truffle.r.runtime.ffi/src/com/oracle/truffle/r/runtime/ffi/jnr/CallRFFIHelper.java
index 7a850063c2..7723fd633e 100644
--- a/com.oracle.truffle.r.runtime.ffi/src/com/oracle/truffle/r/runtime/ffi/jnr/CallRFFIHelper.java
+++ b/com.oracle.truffle.r.runtime.ffi/src/com/oracle/truffle/r/runtime/ffi/jnr/CallRFFIHelper.java
@@ -712,26 +712,17 @@ public class CallRFFIHelper {
         throw unimplemented();
     }
 
-    private static class TryEvalResult {
-        @SuppressWarnings("unused") final Object value;
-        @SuppressWarnings("unused") final boolean error;
-
-        TryEvalResult(Object value, boolean error) {
-            this.value = value;
-            this.error = error;
-        }
-    }
-
     @SuppressWarnings("unused")
-    public static TryEvalResult R_tryEval(Object expr, Object env, boolean silent) {
+    public static Object R_tryEval(Object expr, Object env, boolean silent) {
         Object handlerStack = RErrorHandling.getHandlerStack();
         Object restartStack = RErrorHandling.getRestartStack();
         try {
             // TODO handle silent
             RErrorHandling.resetStacks();
             Object result = Rf_eval(expr, env);
-            // TODO did an error occur?
-            return new TryEvalResult(result, false);
+            return result;
+        } catch (Throwable t) {
+            return null;
         } finally {
             RErrorHandling.restoreStacks(handlerStack, restartStack);
         }
diff --git a/com.oracle.truffle.r.test.native/packages/testrffi/testrffi/src/testrffi.c b/com.oracle.truffle.r.test.native/packages/testrffi/testrffi/src/testrffi.c
index 739efe32d6..aadc29d3f1 100644
--- a/com.oracle.truffle.r.test.native/packages/testrffi/testrffi/src/testrffi.c
+++ b/com.oracle.truffle.r.test.native/packages/testrffi/testrffi/src/testrffi.c
@@ -141,7 +141,17 @@ SEXP interactive(void) {
 }
 
 SEXP tryEval(SEXP expr, SEXP env) {
-	return R_tryEval(expr, env, NULL);
+	int error;
+	SEXP r = R_tryEval(expr, env, &error);
+	SEXP v;
+	PROTECT(v = allocVector(VECSXP, 2));
+	if (error) {
+		r = R_NilValue;
+	}
+	SET_VECTOR_ELT(v, 0, r);
+	SET_VECTOR_ELT(v, 1, ScalarLogical(error));
+	UNPROTECT(v);
+	return v;
 }
 
 SEXP rHomeDir() {
-- 
GitLab