[GR-7953] IsNull flag not properly restored if exception in NFI occurred.

PullRequest: fastr/1367

com.oracle.truffle.r.engine/src/com/oracle/truffle/r/engine/interop/RNullMR.java

 /*
- * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,20 +28,18 @@ import com.oracle.truffle.api.interop.Resolve;
 import com.oracle.truffle.api.interop.TruffleObject;
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.r.ffi.impl.interop.NativePointer;
+import com.oracle.truffle.r.runtime.context.RContext;
 import com.oracle.truffle.r.runtime.data.NativeDataAccess;
 import com.oracle.truffle.r.runtime.data.RNull;
+import com.oracle.truffle.r.runtime.interop.RNullMRContextState;
 
 @MessageResolution(receiverType = RNull.class)
 public class RNullMR {
-    /**
-     * Workaround to avoid NFI converting {@link RNull} to {@code null}.
-     */
-    private static boolean isNull = true;
 
     @Resolve(message = "IS_NULL")
     public abstract static class RNullIsNullNode extends Node {
         protected Object access(@SuppressWarnings("unused") RNull receiver) {
-            return isNull;
+            return RContext.getInstance().stateRNullMR.isNull();
         }
     }
 
@@ -81,9 +79,13 @@ public class RNullMR {
         }
     }
 
+    /**
+     * Workaround to avoid NFI converting {@link RNull} to {@code null}.
+     */
     static boolean setIsNull(boolean value) {
-        boolean prev = isNull;
-        isNull = value;
+        RNullMRContextState state = RContext.getInstance().stateRNullMR;
+        boolean prev = state.isNull();
+        state.setIsNull(value);
         return prev;
     }
 }

com.oracle.truffle.r.ffi.impl/src/com/oracle/truffle/r/ffi/impl/nfi/TruffleNFI_Call.java

 /*
- * Copyright (c) 2014, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -90,11 +90,11 @@ public class TruffleNFI_Call implements CallRFFI {
                         @Cached("nativeCallInfo.address.asTruffleObject()") TruffleObject cachedAddress,
                         @Cached("getFunction(cachedArgsLength)") TruffleObject cachedFunction) {
             Object result = null;
-            boolean isNullSetting = prepareCall(nativeCallInfo.name, args, ffiWrapNodes);
             Object[] realArgs = new Object[cachedArgsLength + 1];
-            System.arraycopy(args, 0, realArgs, 1, cachedArgsLength);
-            realArgs[0] = cachedAddress;
+            boolean isNullSetting = prepareCall(nativeCallInfo.name, args, ffiWrapNodes);
             try {
+                System.arraycopy(args, 0, realArgs, 1, cachedArgsLength);
+                realArgs[0] = cachedAddress;
                 result = ForeignAccess.sendExecute(executeNode, cachedFunction, realArgs);
                 return unwrap.execute(result);
             } catch (InteropException ex) {
@@ -112,11 +112,11 @@ public class TruffleNFI_Call implements CallRFFI {
                         @Cached("create()") FFIUnwrapNode unwrap,
                         @Cached("createExecute(cachedArgsLength)") Node executeNode) {
             Object result = null;
-            boolean isNullSetting = prepareCall(nativeCallInfo.name, args, ffiWrapNodes);
             Object[] realArgs = new Object[cachedArgsLength + 1];
-            System.arraycopy(args, 0, realArgs, 1, cachedArgsLength);
-            realArgs[0] = nativeCallInfo.address.asTruffleObject();
+            boolean isNullSetting = prepareCall(nativeCallInfo.name, args, ffiWrapNodes);
             try {
+                System.arraycopy(args, 0, realArgs, 1, cachedArgsLength);
+                realArgs[0] = nativeCallInfo.address.asTruffleObject();
                 result = ForeignAccess.sendExecute(executeNode, getFunction(cachedArgsLength), realArgs);
                 return unwrap.execute(result);
             } catch (InteropException ex) {
@@ -181,6 +181,7 @@ public class TruffleNFI_Call implements CallRFFI {
     }
 
     private static void prepareReturn(String name, Object result, boolean isNullSetting) {
+        RContext.getRForeignAccessFactory().setIsNull(isNullSetting);
         if (traceEnabled()) {
             traceDownCallReturn(name, result);
         }
@@ -191,7 +192,6 @@ public class TruffleNFI_Call implements CallRFFI {
             nfiCtx.setLastUpCallException(null);
             throw lastUpCallEx;
         }
-        RContext.getRForeignAccessFactory().setIsNull(isNullSetting);
     }
 
     @Override

com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/context/RContext.java

@@ -103,6 +103,7 @@ import com.oracle.truffle.r.runtime.ffi.RFFIContext;
 import com.oracle.truffle.r.runtime.ffi.RFFIFactory;
 import com.oracle.truffle.r.runtime.instrument.InstrumentationState;
 import com.oracle.truffle.r.runtime.interop.FastrInteropTryContextState;
+import com.oracle.truffle.r.runtime.interop.RNullMRContextState;
 import com.oracle.truffle.r.runtime.nodes.RCodeBuilder;
 import com.oracle.truffle.r.runtime.nodes.RSyntaxNode;
 import com.oracle.truffle.r.runtime.rng.RRNG;
@@ -350,6 +351,7 @@ public final class RContext {
     public final InstrumentationState stateInstrumentation;
     public final ContextStateImpl stateInternalCode;
     public final DLL.ContextStateImpl stateDLL;
+    public final RNullMRContextState stateRNullMR;
 
     @CompilationFinal private RFFIContext stateRFFI;
 
@@ -370,7 +372,7 @@ public final class RContext {
     private ContextState[] contextStates() {
         return new ContextState[]{stateREnvVars, stateRLocale, stateRProfile, stateTempPath, stateROptions, stateREnvironment, stateRErrorHandling, stateRConnection, stateStdConnections, stateRNG,
                         stateRFFI,
-                        stateRSerialize, stateLazyDBCache, stateInstrumentation, stateDLL};
+                        stateRSerialize, stateLazyDBCache, stateInstrumentation, stateDLL, stateRNullMR};
     }
 
     public static void setEmbedded() {
@@ -454,6 +456,7 @@ public final class RContext {
         this.stateInstrumentation = InstrumentationState.newContextState(instrumenter);
         this.stateInternalCode = ContextStateImpl.newContextState();
         this.stateDLL = DLL.ContextStateImpl.newContextState();
+        this.stateRNullMR = RNullMRContextState.newContextState();
         this.engine = RContext.getRRuntimeASTAccess().createEngine(this);
         state.add(State.CONSTRUCTED);
 
@@ -527,6 +530,7 @@ public final class RContext {
         stateLazyDBCache.initialize(this);
         stateInstrumentation.initialize(this);
         stateInternalCode.initialize(this);
+        stateRNullMR.initialize(this);
         state.add(State.INITIALIZED);
 
         if (!embedded) {

com.oracle.truffle.r.runtime/src/com/oracle/truffle/r/runtime/interop/RNullMRContextState.java

+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package com.oracle.truffle.r.runtime.interop;
+
+import com.oracle.truffle.r.runtime.context.RContext;
+import com.oracle.truffle.r.runtime.data.RNull;
+
+public class RNullMRContextState implements RContext.ContextState {
+    /**
+     * Workaround to avoid NFI converting {@link RNull} to {@code null}.
+     */
+    private boolean isNull = true;
+
+    public static RNullMRContextState newContextState() {
+        return new RNullMRContextState();
+    }
+
+    public boolean isNull() {
+        return isNull;
+    }
+
+    public void setIsNull(boolean isNull) {
+        this.isNull = isNull;
+    }
+}

com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/engine/interop/AbstractMRTest.java

 /*
- * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -46,7 +46,6 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import org.graalvm.polyglot.Context;
 import org.junit.Test;
-import static com.oracle.truffle.r.test.generate.FastRSession.execInContext;
 import java.util.concurrent.Callable;
 
 public abstract class AbstractMRTest {

com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/engine/interop/RFunctionMRTest.java

 /*
- * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -42,29 +42,32 @@ public class RFunctionMRTest extends AbstractMRTest {
 
     @Test
     public void testExecute() throws UnsupportedTypeException, ArityException, UnsupportedMessageException {
-        RFunction f = create("function() {}");
-        assertTrue(ForeignAccess.sendIsExecutable(Message.IS_EXECUTABLE.createNode(), f));
+        execInContext(() -> {
+            RFunction f = create("function() {}");
+            assertTrue(ForeignAccess.sendIsExecutable(Message.IS_EXECUTABLE.createNode(), f));
 
-        TruffleObject result = (TruffleObject) ForeignAccess.sendExecute(Message.createExecute(0).createNode(), f);
-        assertTrue(ForeignAccess.sendIsNull(Message.IS_NULL.createNode(), result));
+            TruffleObject result = (TruffleObject) ForeignAccess.sendExecute(Message.createExecute(0).createNode(), f);
+            assertTrue(ForeignAccess.sendIsNull(Message.IS_NULL.createNode(), result));
 
-        f = create("function() {1L}");
-        assertEquals(1, ForeignAccess.sendExecute(Message.createExecute(0).createNode(), f));
+            f = create("function() {1L}");
+            assertEquals(1, ForeignAccess.sendExecute(Message.createExecute(0).createNode(), f));
 
-        f = create("function() {1}");
-        assertEquals(1.0, ForeignAccess.sendExecute(Message.createExecute(0).createNode(), f));
+            f = create("function() {1}");
+            assertEquals(1.0, ForeignAccess.sendExecute(Message.createExecute(0).createNode(), f));
 
-        f = create("function() {TRUE}");
-        assertEquals(true, ForeignAccess.sendExecute(Message.createExecute(0).createNode(), f));
+            f = create("function() {TRUE}");
+            assertEquals(true, ForeignAccess.sendExecute(Message.createExecute(0).createNode(), f));
 
-        f = create("function(a) {a}");
-        assertEquals("abc", ForeignAccess.sendExecute(Message.createExecute(1).createNode(), f, "abc"));
+            f = create("function(a) {a}");
+            assertEquals("abc", ForeignAccess.sendExecute(Message.createExecute(1).createNode(), f, "abc"));
 
-        f = create("function(a) { is.logical(a) }");
-        assertEquals(true, ForeignAccess.sendExecute(Message.createExecute(1).createNode(), f, true));
+            f = create("function(a) { is.logical(a) }");
+            assertEquals(true, ForeignAccess.sendExecute(Message.createExecute(1).createNode(), f, true));
 
-        f = create("function(a) { as.external.short(a) }");
-        assertTrue(ForeignAccess.sendExecute(Message.createExecute(1).createNode(), f, 123) instanceof Short);
+            f = create("function(a) { as.external.short(a) }");
+            assertTrue(ForeignAccess.sendExecute(Message.createExecute(1).createNode(), f, 123) instanceof Short);
+            return null;
+        });
 
     }