Implement correct exception restoring.

fangerer · fangerer · commit e1bf4a3653a5 · 2018-07-18T18:54:33.000+02:00
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/ImpModuleBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/ImpModuleBuiltins.java
@@ -191,8 +191,17 @@ private Object loadDynamicModuleWithSpec(String name, String path, Node readNode
                 throw raise(ImportError, "no function PyInit_%s found in %s", basename, path);
             }
             try {
+                // save current exception state
+                PException exceptionState = getContext().getCurrentException();
+                // clear current exception such that native code has clean environment
+                getContext().setCurrentException(null);
+
                 Object nativeResult = ForeignAccess.sendExecute(executeNode, pyinitFunc);
                 TruffleCextBuiltins.checkFunctionResult(getContext(), isNullNode, "PyInit_" + basename, nativeResult);
+
+                // restore previous exception state
+                getContext().setCurrentException(exceptionState);
+
                 Object result = AsPythonObjectNode.doSlowPath(nativeResult);
                 if (!(result instanceof PythonModule)) {
                     // PyModuleDef_Init(pyModuleDef)
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/TruffleCextBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/TruffleCextBuiltins.java
@@ -548,6 +548,8 @@ private Object getLongItem(PDict nativeMembers, String key) {
     // roughly equivalent to _Py_CheckFunctionResult in Objects/call.c
     public static Object checkFunctionResult(PythonContext context, Node isNullNode, String name, Object result) {
         PException currentException = context.getCurrentException();
+        // consume exception
+        context.setCurrentException(null);
         boolean errOccurred = currentException != null;
         if (PGuards.isForeignObject(result) && ForeignAccess.sendIsNull(isNullNode, (TruffleObject) result) || result == PNone.NO_VALUE) {
             if (!errOccurred) {
@@ -613,7 +615,12 @@ public Object execute(VirtualFrame frame) {
                 }
                 // save current exception state
                 PException exceptionState = getContext().getCurrentException();
+                // clear current exception such that native code has clean environment
+                getContext().setCurrentException(null);
+
                 Object result = fromNative(asPythonObjectNode.execute(checkFunctionResult(getContext(), isNullNode, name, ForeignAccess.sendExecute(executeNode, fun, arguments))));
+
+                // restore previous exception state
                 getContext().setCurrentException(exceptionState);
                 return result;
             } catch (UnsupportedTypeException | ArityException | UnsupportedMessageException e) {
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/generator/GeneratorTryExceptNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/generator/GeneratorTryExceptNode.java
@@ -65,8 +65,10 @@ public GeneratorTryExceptNode(PNode body, ExceptNode[] exceptNodes, PNode orelse
     public Object execute(VirtualFrame frame) {
         Object result = PNone.NONE;
 
+        PException exceptionState = getContext().getCurrentException();
+
         if (isActive(frame, exceptFlag)) {
-            catchException(frame, getActiveException(frame));
+            catchException(frame, getActiveException(frame), exceptionState);
             return doReturn(frame, result);
         }
 
@@ -80,7 +82,7 @@ public Object execute(VirtualFrame frame) {
         } catch (PException ex) {
             setActive(frame, exceptFlag, true);
             setActiveException(frame, ex);
-            catchException(frame, ex);
+            catchException(frame, ex, exceptionState);
             return doReturn(frame, result);
         }
 
@@ -95,7 +97,7 @@ private Object doReturn(VirtualFrame frame, Object result) {
     }
 
     @ExplodeLoop
-    private void catchException(VirtualFrame frame, PException exception) {
+    private void catchException(VirtualFrame frame, PException exception, PException exceptionState) {
         ExceptNode[] exceptNodes = getExceptNodes();
         final int matchingExceptNodeIndex = getIndex(frame, exceptIndex);
         boolean wasHandled = false;
@@ -108,7 +110,7 @@ private void catchException(VirtualFrame frame, PException exception) {
                     ExceptNode exceptNode = exceptNodes[i];
                     setIndex(frame, exceptIndex, i + 1);
                     if (exceptNode.matchesException(frame, exception)) {
-                        runExceptionHandler(frame, exception, exceptNode);
+                        runExceptionHandler(frame, exception, exceptNode, exceptionState);
                         wasHandled = true;
                     }
                 }
@@ -117,7 +119,7 @@ private void catchException(VirtualFrame frame, PException exception) {
             // we already found the right except handler, jump back into
             // it directly
             ExceptNode exceptNode = exceptNodes[matchingExceptNodeIndex - 1];
-            runExceptionHandler(frame, exception, exceptNode);
+            runExceptionHandler(frame, exception, exceptNode, exceptionState);
             wasHandled = true;
         }
         reset(frame);
@@ -127,11 +129,15 @@ private void catchException(VirtualFrame frame, PException exception) {
         }
     }
 
-    private static void runExceptionHandler(VirtualFrame frame, PException exception, ExceptNode exceptNode) {
+    private void runExceptionHandler(VirtualFrame frame, PException exception, ExceptNode exceptNode, PException exceptionState) {
         try {
             exceptNode.executeExcept(frame, exception);
         } catch (ExceptionHandledException e) {
             return;
+        } finally {
+            // restore previous exception state, this won't happen if the except block raises an
+            // exception
+            getContext().setCurrentException(exceptionState);
         }
     }
 
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/statement/ExceptNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/statement/ExceptNode.java
@@ -64,14 +64,7 @@ public void executeExcept(VirtualFrame frame, PException e) {
             getCore().setCurrentException(e);
         }
 
-        try {
-            body.execute(frame);
-        } finally {
-            // clear the exception after executing the except body.
-            if (context != null) {
-                context.setCurrentException(null);
-            }
-        }
+        body.execute(frame);
         throw ExceptionHandledException.INSTANCE;
     }
 
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/statement/TryExceptNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/statement/TryExceptNode.java
@@ -60,10 +60,12 @@ public TryExceptNode(PNode body, ExceptNode[] exceptNodes, PNode orelse) {
 
     @Override
     public Object execute(VirtualFrame frame) {
+        // store current exception state for later restore
+        PException exceptionState = getContext().getCurrentException();
         try {
             body.execute(frame);
         } catch (PException ex) {
-            catchException(frame, ex);
+            catchException(frame, ex, exceptionState);
             return PNone.NONE;
         } catch (Exception e) {
             if (!seenException) {
@@ -78,7 +80,7 @@ public Object execute(VirtualFrame frame) {
                     PException pe = new PException(getBaseException(e), this);
                     pe.getExceptionObject().setException(pe);
                     try {
-                        catchException(frame, pe);
+                        catchException(frame, pe, exceptionState);
                     } catch (PException pe_thrown) {
                         if (pe_thrown != pe) {
                             throw e;
@@ -98,7 +100,7 @@ private PBaseException getBaseException(Exception t) {
     }
 
     @ExplodeLoop
-    private void catchException(VirtualFrame frame, PException exception) {
+    private void catchException(VirtualFrame frame, PException exception, PException exceptionState) {
         boolean wasHandled = false;
         for (ExceptNode exceptNode : exceptNodes) {
             // we want a constant loop iteration count for ExplodeLoop to work,
@@ -109,6 +111,10 @@ private void catchException(VirtualFrame frame, PException exception) {
                         exceptNode.executeExcept(frame, exception);
                     } catch (ExceptionHandledException e) {
                         wasHandled = true;
+                    } finally {
+                        // restore previous exception state, this won't happen if the except block
+                        // raises an exception
+                        getContext().setCurrentException(exceptionState);
                     }
                 }
             }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/exception/PException.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/exception/PException.java
@@ -138,35 +138,27 @@ public boolean isExit() {
         return exit;
     }
 
-    private static void popException(PythonCore core) {
-        core.getContext().setCurrentException(null);
-    }
-
     public void expectIndexError(PythonCore core, ConditionProfile profile) {
         if (profile.profile(getType() != core.getErrorClass(PythonErrorType.IndexError))) {
             throw this;
         }
-        popException(core);
     }
 
     public void expectStopIteration(PythonCore core, ConditionProfile profile) {
         if (profile.profile(getType() != core.getErrorClass(PythonErrorType.StopIteration))) {
             throw this;
         }
-        popException(core);
     }
 
     public void expectAttributeError(PythonCore core, ConditionProfile profile) {
         if (profile.profile(getType() != core.getErrorClass(PythonErrorType.AttributeError))) {
             throw this;
         }
-        popException(core);
     }
 
     public void expect(PythonErrorType error, PythonCore core, ConditionProfile profile) {
         if (profile.profile(getType() != core.getErrorClass(error))) {
             throw this;
         }
-        popException(core);
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -65,8 +65,10 @@ public GeneratorTryExceptNode(PNode body, ExceptNode[] exceptNodes, PNode orelse`
`65`	`65`	`public Object execute(VirtualFrame frame) {`
`66`	`66`	`Object result = PNone.NONE;`
`67`	`67`
	`68`	`+ PException exceptionState = getContext().getCurrentException();`
	`69`	`+`
`68`	`70`	`if (isActive(frame, exceptFlag)) {`
`69`		`- catchException(frame, getActiveException(frame));`
	`71`	`+ catchException(frame, getActiveException(frame), exceptionState);`
`70`	`72`	`return doReturn(frame, result);`
`71`	`73`	`}`
`72`	`74`
`@@ -80,7 +82,7 @@ public Object execute(VirtualFrame frame) {`
`80`	`82`	`} catch (PException ex) {`
`81`	`83`	`setActive(frame, exceptFlag, true);`
`82`	`84`	`setActiveException(frame, ex);`
`83`		`- catchException(frame, ex);`
	`85`	`+ catchException(frame, ex, exceptionState);`
`84`	`86`	`return doReturn(frame, result);`
`85`	`87`	`}`
`86`	`88`
`@@ -95,7 +97,7 @@ private Object doReturn(VirtualFrame frame, Object result) {`
`95`	`97`	`}`
`96`	`98`
`97`	`99`	`@ExplodeLoop`
`98`		`- private void catchException(VirtualFrame frame, PException exception) {`
	`100`	`+ private void catchException(VirtualFrame frame, PException exception, PException exceptionState) {`
`99`	`101`	`ExceptNode[] exceptNodes = getExceptNodes();`
`100`	`102`	`final int matchingExceptNodeIndex = getIndex(frame, exceptIndex);`
`101`	`103`	`boolean wasHandled = false;`
`@@ -108,7 +110,7 @@ private void catchException(VirtualFrame frame, PException exception) {`
`108`	`110`	`ExceptNode exceptNode = exceptNodes[i];`
`109`	`111`	`setIndex(frame, exceptIndex, i + 1);`
`110`	`112`	`if (exceptNode.matchesException(frame, exception)) {`
`111`		`- runExceptionHandler(frame, exception, exceptNode);`
	`113`	`+ runExceptionHandler(frame, exception, exceptNode, exceptionState);`
`112`	`114`	`wasHandled = true;`
`113`	`115`	`}`
`114`	`116`	`}`
`@@ -117,7 +119,7 @@ private void catchException(VirtualFrame frame, PException exception) {`
`117`	`119`	`// we already found the right except handler, jump back into`
`118`	`120`	`// it directly`
`119`	`121`	`ExceptNode exceptNode = exceptNodes[matchingExceptNodeIndex - 1];`
`120`		`- runExceptionHandler(frame, exception, exceptNode);`
	`122`	`+ runExceptionHandler(frame, exception, exceptNode, exceptionState);`
`121`	`123`	`wasHandled = true;`
`122`	`124`	`}`
`123`	`125`	`reset(frame);`
`@@ -127,11 +129,15 @@ private void catchException(VirtualFrame frame, PException exception) {`
`127`	`129`	`}`
`128`	`130`	`}`
`129`	`131`
`130`		`- private static void runExceptionHandler(VirtualFrame frame, PException exception, ExceptNode exceptNode) {`
	`132`	`+ private void runExceptionHandler(VirtualFrame frame, PException exception, ExceptNode exceptNode, PException exceptionState) {`
`131`	`133`	`try {`
`132`	`134`	`exceptNode.executeExcept(frame, exception);`
`133`	`135`	`} catch (ExceptionHandledException e) {`
`134`	`136`	`return;`
	`137`	`+ } finally {`
	`138`	`+ // restore previous exception state, this won't happen if the except block raises an`
	`139`	`+ // exception`
	`140`	`+ getContext().setCurrentException(exceptionState);`
`135`	`141`	`}`
`136`	`142`	`}`
`137`	`143`
Original file line number	Diff line number	Diff line change
`@@ -64,14 +64,7 @@ public void executeExcept(VirtualFrame frame, PException e) {`
`64`	`64`	`getCore().setCurrentException(e);`
`65`	`65`	`}`
`66`	`66`
`67`		`- try {`
`68`		`- body.execute(frame);`
`69`		`- } finally {`
`70`		`- // clear the exception after executing the except body.`
`71`		`- if (context != null) {`
`72`		`- context.setCurrentException(null);`
`73`		`- }`
`74`		`- }`
	`67`	`+ body.execute(frame);`
`75`	`68`	`throw ExceptionHandledException.INSTANCE;`
`76`	`69`	`}`
`77`	`70`
Original file line number	Diff line number	Diff line change
`@@ -138,35 +138,27 @@ public boolean isExit() {`
`138`	`138`	`return exit;`
`139`	`139`	`}`
`140`	`140`
`141`		`- private static void popException(PythonCore core) {`
`142`		`- core.getContext().setCurrentException(null);`
`143`		`- }`
`144`		`-`
`145`	`141`	`public void expectIndexError(PythonCore core, ConditionProfile profile) {`
`146`	`142`	`if (profile.profile(getType() != core.getErrorClass(PythonErrorType.IndexError))) {`
`147`	`143`	`throw this;`
`148`	`144`	`}`
`149`		`- popException(core);`
`150`	`145`	`}`
`151`	`146`
`152`	`147`	`public void expectStopIteration(PythonCore core, ConditionProfile profile) {`
`153`	`148`	`if (profile.profile(getType() != core.getErrorClass(PythonErrorType.StopIteration))) {`
`154`	`149`	`throw this;`
`155`	`150`	`}`
`156`		`- popException(core);`
`157`	`151`	`}`
`158`	`152`
`159`	`153`	`public void expectAttributeError(PythonCore core, ConditionProfile profile) {`
`160`	`154`	`if (profile.profile(getType() != core.getErrorClass(PythonErrorType.AttributeError))) {`
`161`	`155`	`throw this;`
`162`	`156`	`}`
`163`		`- popException(core);`
`164`	`157`	`}`
`165`	`158`
`166`	`159`	`public void expect(PythonErrorType error, PythonCore core, ConditionProfile profile) {`
`167`	`160`	`if (profile.profile(getType() != core.getErrorClass(error))) {`
`168`	`161`	`throw this;`
`169`	`162`	`}`
`170`		`- popException(core);`
`171`	`163`	`}`
`172`	`164`	`}`