Cleaner attempt at #3577.

The problems in #3577 stemmed from the introduction of a second exit instruction into global ensure blocks (rethrow saved exc). Logic elsewhere in our passes that dealt with the GEB all assumed that there would be exactly one control-transfer instruction at the end of the GEB, and inserted various logic before this point: * Stores of modified variables into the current binding. * Pops of binding and frame at the end of the method body. Once there were two instructions, it becamse a mess to know where to insert these instructions. This change modifies the "rethrow" instruction to itself be a special case of "return" that may throw a saved exception. In both cases, it is unrolling at least the current stack frame, and this allows us to have a single instruction to do both normal returns and saved exception rethrows in the GEB. This change reverts previous fixes for #3577 that attempted to work around the double-exit.
jruby · Jan 7, 2016 · 86884e7 · 86884e7
1 parent b56f3ca
commit 86884e7
Showing 10 changed files with 49 additions and 41 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -2086,8 +2086,7 @@ private void handleBreakAndReturnsInLambdas() {
         // --> IRRuntimeHelpers.handleBreakAndReturnsInLambdas(context, scope, bj, blockType)
         Variable ret = createTemporaryVariable();
         addInstr(new RuntimeHelperCall(ret, RuntimeHelperCall.Methods.HANDLE_BREAK_AND_RETURNS_IN_LAMBDA, new Operand[]{exc} ));
-        addInstr(new RethrowSavedExcInLambdaInstr());
-        addInstr(new ReturnInstr(ret));
+        addInstr(new ReturnOrRethrowSavedExcInstr(ret));
 
         // End
         addInstr(new LabelInstr(rEndLabel));

diff --git a/core/src/main/java/org/jruby/ir/IRVisitor.java b/core/src/main/java/org/jruby/ir/IRVisitor.java
@@ -119,7 +119,7 @@ private void error(Object object) {
     public void RestArgMultipleAsgnInstr(RestArgMultipleAsgnInstr restargmultipleasgninstr) { error(restargmultipleasgninstr); }
     public void RestoreBindingVisibilityInstr(RestoreBindingVisibilityInstr instr) { error(instr); }
     public void ReturnInstr(ReturnInstr returninstr) { error(returninstr); }
-    public void RethrowSavedExcInLambdaInstr(RethrowSavedExcInLambdaInstr instr) { error(instr); }
+    public void ReturnOrRethrowSavedExcInstr(ReturnOrRethrowSavedExcInstr instr) { error(instr); }
     public void RuntimeHelperCall(RuntimeHelperCall runtimehelpercall) { error(runtimehelpercall); }
     public void SaveBindingVisibilityInstr(SaveBindingVisibilityInstr instr) { error(instr); }
     public void SearchConstInstr(SearchConstInstr searchconstinstr) { error(searchconstinstr); }

diff --git a/core/src/main/java/org/jruby/ir/Operation.java b/core/src/main/java/org/jruby/ir/Operation.java
@@ -93,6 +93,7 @@ public enum Operation {
      * and the net result is to return from the closure. */
     NONLOCAL_RETURN(OpFlags.f_has_side_effect | OpFlags.f_is_return | OpFlags.f_can_raise_exception),
     BREAK(OpFlags.f_has_side_effect | OpFlags.f_is_return | OpFlags.f_can_raise_exception),
+    RETURN_OR_RETHROW_SAVED_EXC(OpFlags.f_has_side_effect | OpFlags.f_is_return),
 
     /** defines **/
     ALIAS(OpFlags.f_has_side_effect| OpFlags.f_modifies_code | OpFlags.f_can_raise_exception),
@@ -215,7 +216,6 @@ public enum Operation {
     POP_BINDING(OpFlags.f_is_book_keeping_op | OpFlags.f_has_side_effect),
     SAVE_BINDING_VIZ(OpFlags.f_is_book_keeping_op | OpFlags.f_has_side_effect),
     RESTORE_BINDING_VIZ(OpFlags.f_is_book_keeping_op | OpFlags.f_has_side_effect),
-    RETHROW_SAVED_EXC_IN_LAMBDA(OpFlags.f_is_book_keeping_op | OpFlags.f_has_side_effect),
     TOGGLE_BACKTRACE(OpFlags.f_is_book_keeping_op | OpFlags.f_has_side_effect),
     UPDATE_BLOCK_STATE(OpFlags.f_is_book_keeping_op | OpFlags.f_has_side_effect),
 

diff --git a/core/src/main/java/org/jruby/ir/instructions/RethrowSavedExcInLambdaInstr.java b/core/src/main/java/org/jruby/ir/instructions/RethrowSavedExcInLambdaInstr.java
diff --git a/core/src/main/java/org/jruby/ir/instructions/ReturnInstr.java b/core/src/main/java/org/jruby/ir/instructions/ReturnInstr.java
@@ -12,7 +12,11 @@
 
 public class ReturnInstr extends ReturnBase implements FixedArityInstr {
     public ReturnInstr(Operand returnValue) {
-        super(Operation.RETURN, returnValue);
+        this(Operation.RETURN, returnValue);
+    }
+
+    public ReturnInstr(Operation operation, Operand returnValue) {
+        super(operation, returnValue);
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/instructions/ReturnOrRethrowSavedExcInstr.java b/core/src/main/java/org/jruby/ir/instructions/ReturnOrRethrowSavedExcInstr.java
@@ -0,0 +1,28 @@
+package org.jruby.ir.instructions;
+
+import org.jruby.ir.IRVisitor;
+import org.jruby.ir.Operation;
+import org.jruby.ir.operands.Operand;
+import org.jruby.ir.persistence.IRReaderDecoder;
+import org.jruby.ir.persistence.IRWriterEncoder;
+import org.jruby.ir.transformations.inlining.CloneInfo;
+
+public class ReturnOrRethrowSavedExcInstr extends ReturnInstr {
+    public ReturnOrRethrowSavedExcInstr(Operand returnValue) {
+        super(Operation.RETURN_OR_RETHROW_SAVED_EXC, returnValue);
+    }
+
+    @Override
+    public Instr clone(CloneInfo ii) {
+        return this; // FIXME: Needs update
+    }
+
+    public static ReturnOrRethrowSavedExcInstr decode(IRReaderDecoder d) {
+        return new ReturnOrRethrowSavedExcInstr(d.decodeOperand());
+    }
+
+    @Override
+    public void visit(IRVisitor visitor) {
+        visitor.ReturnOrRethrowSavedExcInstr(this);
+    }
+}
diff --git a/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
@@ -24,6 +24,7 @@
 import org.jruby.ir.instructions.RestoreBindingVisibilityInstr;
 import org.jruby.ir.instructions.ResultInstr;
 import org.jruby.ir.instructions.ReturnBase;
+import org.jruby.ir.instructions.ReturnOrRethrowSavedExcInstr;
 import org.jruby.ir.instructions.RuntimeHelperCall;
 import org.jruby.ir.instructions.SaveBindingVisibilityInstr;
 import org.jruby.ir.instructions.SearchConstInstr;
@@ -381,9 +382,6 @@ protected static void processBookKeepingOp(ThreadContext context, Block block, I
             case POP_BINDING:
                 context.popScope();
                 break;
-            case RETHROW_SAVED_EXC_IN_LAMBDA:
-                IRRuntimeHelpers.rethrowSavedExcInLambda(context);
-                break; // may not be reachable
             case THREAD_POLL:
                 if (IRRuntimeHelpers.inProfileMode()) Profiler.clockTick();
                 context.callThreadPoll();
@@ -435,6 +433,10 @@ protected static IRubyObject processReturnOp(ThreadContext context, Block block,
                 IRubyObject rv = (IRubyObject)retrieveOp(ri.getReturnValue(), context, self, currDynScope, currScope, temp);
                 return IRRuntimeHelpers.initiateNonLocalReturn(context, currDynScope, blockType, rv);
             }
+            case RETURN_OR_RETHROW_SAVED_EXC: {
+                IRubyObject retVal = (IRubyObject) retrieveOp(((ReturnBase) instr).getReturnValue(), context, self, currDynScope, currScope, temp);
+                return IRRuntimeHelpers.returnOrRethrowSavedException(context, retVal);
+            }
         }
         return null;
     }

diff --git a/core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java b/core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java
@@ -156,10 +156,6 @@ public Object execute(IRScope scope, Object... data) {
                         if (requireBinding) fixReturn(scope, (ReturnInstr)i, instrs);
                         // Add before the break/return
                         i = instrs.previous();
-                        if (i instanceof RethrowSavedExcInLambdaInstr) {
-                            // back up one more to precede rethrow
-                            i = instrs.previous();
-                        }
                         popSavedState(scope, bb == geb, requireBinding, requireFrame, savedViz, savedFrame, instrs);
                         if (bb == geb) gebProcessed = true;
                         break;

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -221,7 +221,7 @@ public static IRubyObject handleBreakAndReturnsInLambdas(ThreadContext context,
     }
 
     @JIT
-    public static void rethrowSavedExcInLambda(ThreadContext context) {
+    public static IRubyObject returnOrRethrowSavedException(ThreadContext context, IRubyObject value) {
         // This rethrows the exception saved in handleBreakAndReturnsInLambda
         // after additional code to pop frames, bindings, etc. are done.
         Throwable exc = context.getSavedExceptionInLambda();
@@ -230,6 +230,9 @@ public static void rethrowSavedExcInLambda(ThreadContext context) {
             context.setSavedExceptionInLambda(null);
             Helpers.throwException(exc);
         }
+
+        // otherwise, return value
+        return value;
     }
 
     @JIT

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -1734,9 +1734,11 @@ public void RestoreBindingVisibilityInstr(RestoreBindingVisibilityInstr instr) {
     }
 
     @Override
-    public void RethrowSavedExcInLambdaInstr(RethrowSavedExcInLambdaInstr instr) {
+    public void ReturnOrRethrowSavedExcInstr(ReturnOrRethrowSavedExcInstr instr) {
         jvmMethod().loadContext();
-        jvmMethod().invokeIRHelper("rethrowSavedExcInLambda", sig(void.class, ThreadContext.class));
+        visit(instr.getReturnValue());
+        jvmMethod().invokeIRHelper("returnOrRethrowSavedException", sig(IRubyObject.class, ThreadContext.class, IRubyObject.class));
+        jvmMethod().returnValue();
     }
 
     @Override