Fix block call protocol IR generation for lambdas.

* Rather than immediately throwing exception in handleBreakAndReturnsInLambda, save it in ThreadContext and rethrow it after runtime state is updated (frames, dynscopes popped, etc.) * Let us see how Travis likes this.
jruby · Dec 13, 2015 · 0d35995 · 0d35995
1 parent 8a04861
commit 0d35995
Showing 9 changed files with 87 additions and 24 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -2087,6 +2087,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));
 
         // End

diff --git a/core/src/main/java/org/jruby/ir/IRVisitor.java b/core/src/main/java/org/jruby/ir/IRVisitor.java
@@ -117,6 +117,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 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
@@ -213,6 +213,7 @@ 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
@@ -0,0 +1,26 @@
+package org.jruby.ir.instructions;
+
+import org.jruby.ir.IRVisitor;
+import org.jruby.ir.Operation;
+import org.jruby.ir.persistence.IRReaderDecoder;
+import org.jruby.ir.transformations.inlining.CloneInfo;
+
+public class RethrowSavedExcInLambdaInstr extends NoOperandInstr implements FixedArityInstr {
+    public RethrowSavedExcInLambdaInstr() {
+        super(Operation.RETHROW_SAVED_EXC_IN_LAMBDA);
+    }
+
+    @Override
+    public Instr clone(CloneInfo ii) {
+        return this; // FIXME: Needs update
+    }
+
+    public static RethrowSavedExcInLambdaInstr decode(IRReaderDecoder d) {
+        return new RethrowSavedExcInLambdaInstr();
+    }
+
+    @Override
+    public void visit(IRVisitor visitor) {
+        visitor.RethrowSavedExcInLambdaInstr(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
@@ -378,6 +378,9 @@ 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();

diff --git a/core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java b/core/src/main/java/org/jruby/ir/passes/AddCallProtocolInstructions.java
@@ -23,8 +23,7 @@ public String getLabel() {
 
     private boolean explicitCallProtocolSupported(IRScope scope) {
         return scope instanceof IRMethod
-            // SSS: Turning this off till this is fully debugged
-            // || (scope instanceof IRClosure && !(scope instanceof IREvalScript))
+            || (scope instanceof IRClosure && !(scope instanceof IREvalScript))
             || (scope instanceof IRModuleBody && !(scope instanceof IRMetaClassBody));
     }
 
@@ -46,7 +45,11 @@ private void fixReturn(IRScope scope, ReturnBase i, ListIterator<Instr> instrs)
         }
     }
 
-    private void popSavedState(IRScope scope, boolean requireBinding, boolean requireFrame, Variable savedViz, Variable savedFrame, ListIterator<Instr> instrs) {
+    private void popSavedState(IRScope scope, boolean isGEB, boolean requireBinding, boolean requireFrame, Variable savedViz, Variable savedFrame, ListIterator<Instr> instrs) {
+        if (scope instanceof IRClosure && isGEB) {
+            // Add before RethrowSavedExcInLambdaInstr
+            instrs.previous();
+        }
         if (requireBinding) instrs.add(new PopBindingInstr());
         if (scope instanceof IRClosure) {
             instrs.add(new PopBlockFrameInstr(savedFrame));
@@ -143,7 +146,7 @@ public Object execute(IRScope scope, Object... data) {
                         if (requireBinding) fixReturn(scope, (ReturnInstr)i, instrs);
                         // Add before the break/return
                         instrs.previous();
-                        popSavedState(scope, requireBinding, requireFrame, savedViz, savedFrame, instrs);
+                        popSavedState(scope, bb == geb, requireBinding, requireFrame, savedViz, savedFrame, instrs);
                         if (bb == geb) gebProcessed = true;
                         break;
                     }
@@ -155,23 +158,16 @@ public Object execute(IRScope scope, Object... data) {
                         if (requireBinding) fixReturn(scope, (ReturnInstr)i, instrs);
                         instrs.previous();
                     }
-                    popSavedState(scope, requireBinding, requireFrame, savedViz, savedFrame, instrs);
+                    popSavedState(scope, bb == geb, requireBinding, requireFrame, savedViz, savedFrame, instrs);
                     if (bb == geb) gebProcessed = true;
-                }
-
-                if (!gebProcessed && bb == geb) {
+                } else if (!gebProcessed && bb == geb) {
                     // Add before throw-exception-instr which would be the last instr
                     if (i != null) {
                         // Assumption: Last instr should always be a control-transfer instruction
                         assert i.getOperation().transfersControl(): "Last instruction of GEB in scope: " + scope + " is " + i + ", not a control-xfer instruction";
                         instrs.previous();
                     }
-
-                    // SSS FIXME: This is totally broken for lambdas
-                    //
-                    // handleBreakAndReturnsInLambdas would have executed by this point,
-                    // and it might have raised an exception which would totally skip these pops.
-                    popSavedState(scope, requireBinding, requireFrame, savedViz, savedFrame, instrs);
+                    popSavedState(scope, true, requireBinding, requireFrame, savedViz, savedFrame, instrs);
                 }
             }
         }

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -151,7 +151,7 @@ public static IRubyObject initiateNonLocalReturn(ThreadContext context, DynamicS
     public static IRubyObject handleNonlocalReturn(StaticScope scope, DynamicScope dynScope, Object rjExc, Block.Type blockType) throws RuntimeException {
         if (!(rjExc instanceof IRReturnJump)) {
             Helpers.throwException((Throwable)rjExc);
-            return null;
+            return null; // Unreachable
         } else {
             IRReturnJump rj = (IRReturnJump)rjExc;
 
@@ -195,24 +195,41 @@ public static IRubyObject initiateBreak(ThreadContext context, DynamicScope dynS
     public static IRubyObject handleBreakAndReturnsInLambdas(ThreadContext context, StaticScope scope, DynamicScope dynScope, Object exc, Block.Type blockType) throws RuntimeException {
         if ((exc instanceof IRBreakJump) && inNonMethodBodyLambda(scope, blockType)) {
             // We just unwound all the way up because of a non-local break
-            throw IRException.BREAK_LocalJumpError.getException(context.getRuntime());
+            context.setSavedExceptionInLambda(IRException.BREAK_LocalJumpError.getException(context.getRuntime()));
+            return null;
         } else if (exc instanceof IRReturnJump && (blockType == null || inLambda(blockType))) {
-            // Ignore non-local return processing in non-lambda blocks.
-            // Methods have a null blocktype
-            return handleNonlocalReturn(scope, dynScope, exc, blockType);
+            try {
+                // Ignore non-local return processing in non-lambda blocks.
+                // Methods have a null blocktype
+                return handleNonlocalReturn(scope, dynScope, exc, blockType);
+            } catch (Throwable e) {
+                context.setSavedExceptionInLambda(e);
+                return null;
+            }
         } else {
-            // Propagate
-            Helpers.throwException((Throwable)exc);
-            // should not get here
+            // Propagate the exception
+            context.setSavedExceptionInLambda((Throwable)exc);
             return null;
         }
     }
 
+    @JIT
+    public static void rethrowSavedExcInLambda(ThreadContext context) {
+        // This rethrows the exception saved in handleBreakAndReturnsInLambda
+        // after additional code to pop frames, bindings, etc. are done.
+        Throwable exc = context.getSavedExceptionInLambda();
+        if (exc != null) {
+            // IMPORTANT: always clear!
+            context.setSavedExceptionInLambda(null);
+            Helpers.throwException(exc);
+        }
+    }
+
     @JIT
     public static IRubyObject handlePropagatedBreak(ThreadContext context, DynamicScope dynScope, Object bjExc, Block.Type blockType) {
         if (!(bjExc instanceof IRBreakJump)) {
             Helpers.throwException((Throwable)bjExc);
-            return null;
+            return null; // Unreachable
         }
 
         IRBreakJump bj = (IRBreakJump)bjExc;

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -1697,6 +1697,12 @@ public void RestoreBindingVisibilityInstr(RestoreBindingVisibilityInstr instr) {
         jvmAdapter().invokevirtual(p(Binding.class), "setVisibility", sig(void.class, Visibility.class));
     }
 
+    @Override
+    public void RethrowSavedExcInLambdaInstr(RethrowSavedExcInLambdaInstr instr) {
+        jvmMethod().loadContext();
+        jvmMethod().invokeIRHelper("rethrowSavedExcInLambda", sig(void.class, ThreadContext.class));
+    }
+
     @Override
     public void RuntimeHelperCall(RuntimeHelperCall runtimehelpercall) {
         switch (runtimehelpercall.getHelperMethod()) {

diff --git a/core/src/main/java/org/jruby/runtime/ThreadContext.java b/core/src/main/java/org/jruby/runtime/ThreadContext.java
@@ -128,7 +128,10 @@ public static ThreadContext newContext(Ruby runtime) {
 
     IRubyObject lastExitStatus;
 
-    private Block.Type currentBlockType;
+    // These two fields are required to support explicit call protocol
+    // (via IR instructions) for blocks.
+    private Block.Type currentBlockType; // See prepareBlockArgs code in IRRuntimeHelpers
+    private Throwable savedExcInLambda;  // See handleBreakAndReturnsInLambda in IRRuntimeHelpers
 
     public final SecureRandom secureRandom = getSecureRandom();
 
@@ -154,6 +157,7 @@ private ThreadContext(Ruby runtime) {
         this.runtime = runtime;
         this.nil = runtime.getNil();
         this.currentBlockType = Block.Type.NORMAL;
+        this.savedExcInLambda = null;
 
         if (runtime.getInstanceConfig().isProfilingEntireRun()) {
             startProfiling();
@@ -218,6 +222,14 @@ public void setCurrentBlockType(Block.Type type) {
         currentBlockType = type;
     }
 
+    public Throwable getSavedExceptionInLambda() {
+        return savedExcInLambda;
+    }
+
+    public void setSavedExceptionInLambda(Throwable e) {
+        savedExcInLambda = e;
+    }
+
     public CallType getLastCallType() {
         return lastCallType;
     }