Clean out some oddities from NonlocalReturn and Break instructions

* This patch includes work from 11e3766 which was reverted in 3af83b3 since it was incomplete. This patch takes it to completion. * NonlocalReturnInstr and BreakInstr are really exception-raising instructions. Let them go through exception handling paths for running ensure code instead of duplicating ensure path code and then preventing these exceptions from being caught in the startup interpreter. * This also pushes lambda non-local returns & breaks through the local exception path so that non-local returns and breaks are handled uniformed wrt exception / ensure handling without special cases. * This also ensures that in the block call protocol scenario, for lambdas as well as non-lambdas, all non-return control flow (exceptions, breaks, nonlocal-returns) goes through the GEB which ensures correct popping of frame/scope state.
jruby · Dec 19, 2015 · 5f3422c · 5f3422c
1 parent a968fc7
commit 5f3422c
Showing 5 changed files with 41 additions and 18 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -886,10 +886,6 @@ public Operand buildBreak(BreakNode breakNode) {
         IRLoop currLoop = getCurrentLoop();
 
         Operand rv = build(breakNode.getValueNode());
-        // If we have ensure blocks, have to run those first!
-        if (!activeEnsureBlockStack.empty()) {
-            emitEnsureBlocks(currLoop);
-        }
 
         if (currLoop != null) {
             addInstr(new CopyInstr(currLoop.loopResult, rv));
@@ -3246,14 +3242,12 @@ public Operand buildReturn(ReturnNode returnNode) {
             // closure is a proc. If the closure is a lambda, then this becomes a normal return.
             boolean maybeLambda = scope.getNearestMethod() == null;
             addInstr(new CheckForLJEInstr(maybeLambda));
-            retVal = processEnsureRescueBlocks(retVal);
             addInstr(new NonlocalReturnInstr(retVal, maybeLambda ? "--none--" : scope.getNearestMethod().getName()));
         } else if (scope.isModuleBody()) {
             IRMethod sm = scope.getNearestMethod();
 
             // Cannot return from top-level module bodies!
             if (sm == null) addInstr(new ThrowExceptionInstr(IRException.RETURN_LocalJumpError));
-            retVal = processEnsureRescueBlocks(retVal);
             if (sm != null) addInstr(new NonlocalReturnInstr(retVal, sm.getName()));
         } else {
             retVal = processEnsureRescueBlocks(retVal);

diff --git a/core/src/main/java/org/jruby/ir/Operation.java b/core/src/main/java/org/jruby/ir/Operation.java
@@ -88,10 +88,11 @@ public enum Operation {
 
     /** returns -- returns unwind stack, etc. */
     RETURN(OpFlags.f_has_side_effect | OpFlags.f_is_return),
-    NONLOCAL_RETURN(OpFlags.f_has_side_effect | OpFlags.f_is_return),
-    /* BREAK is a return because it can only be used within closures
+    /* These two insructions use exceptions to exit closures
+     * BREAK is a return because it can only be used within closures
      * and the net result is to return from the closure. */
-    BREAK(OpFlags.f_has_side_effect | OpFlags.f_is_return),
+    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),
 
     /** defines **/
     ALIAS(OpFlags.f_has_side_effect| OpFlags.f_modifies_code | OpFlags.f_can_raise_exception),

diff --git a/core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java
@@ -118,8 +118,7 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel
             } catch (Throwable t) {
                 if (debug) extractToMethodToAvoidC2Crash(instr, t);
 
-                if (rescuePCs == null || rescuePCs.empty() || (t instanceof IRBreakJump && instr instanceof BreakInstr) ||
-                        (t instanceof IRReturnJump && instr instanceof NonlocalReturnInstr)) {
+                if (rescuePCs == null || rescuePCs.empty()) {
                     ipc = -1;
                 } else {
                     ipc = rescuePCs.pop();

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -121,9 +121,9 @@ public static void checkForLJE(ThreadContext context, DynamicScope dynScope, boo
      * Handle non-local returns (ex: when nested in closures, root scopes of module/class/sclass bodies)
      */
     public static IRubyObject initiateNonLocalReturn(ThreadContext context, DynamicScope dynScope, Block.Type blockType, IRubyObject returnValue) {
-        // If not in a lambda, check if this was a non-local return
-        if (IRRuntimeHelpers.inLambda(blockType)) return returnValue;
+        if (IRRuntimeHelpers.inLambda(blockType)) throw new IRWrappedLambdaReturnValue(returnValue);
 
+        // If not in a lambda, check if this was a non-local return
         while (dynScope != null) {
             StaticScope ss = dynScope.getStaticScope();
             // SSS FIXME: Why is scopeType empty? Looks like this static-scope
@@ -168,10 +168,11 @@ public static IRubyObject handleNonlocalReturn(StaticScope scope, DynamicScope d
 
     public static IRubyObject initiateBreak(ThreadContext context, DynamicScope dynScope, IRubyObject breakValue, Block.Type blockType) throws RuntimeException {
         if (inLambda(blockType)) {
-            // Ensures would already have been run since the IR builder makes
-            // sure that ensure code has run before we hit the break.  Treat
-            // the break as a regular return from the closure.
-            return breakValue;
+            // Wrap the return value in an exception object
+            // and push it through the break exception paths so
+            // that ensures are run, frames/scopes are popped
+            // from runtime stacks, etc.
+            throw new IRWrappedLambdaReturnValue(breakValue);
         } else {
             StaticScope scope = dynScope.getStaticScope();
             IRScopeType scopeType = scope.getScopeType();
@@ -193,7 +194,13 @@ public static IRubyObject initiateBreak(ThreadContext context, DynamicScope dynS
 
     @JIT
     public static IRubyObject handleBreakAndReturnsInLambdas(ThreadContext context, StaticScope scope, DynamicScope dynScope, Object exc, Block.Type blockType) throws RuntimeException {
-        if ((exc instanceof IRBreakJump) && inNonMethodBodyLambda(scope, blockType)) {
+        if (exc instanceof IRWrappedLambdaReturnValue) {
+            // Wrap the return value in an exception object
+            // and push it through the nonlocal return exception paths so
+            // that ensures are run, frames/scopes are popped
+            // from runtime stacks, etc.
+            return ((IRWrappedLambdaReturnValue)exc).returnValue;
+        } else if ((exc instanceof IRBreakJump) && inNonMethodBodyLambda(scope, blockType)) {
             // We just unwound all the way up because of a non-local break
             context.setSavedExceptionInLambda(IRException.BREAK_LocalJumpError.getException(context.getRuntime()));
             return null;

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRWrappedLambdaReturnValue.java b/core/src/main/java/org/jruby/ir/runtime/IRWrappedLambdaReturnValue.java
@@ -0,0 +1,22 @@
+package org.jruby.ir.runtime;
+
+import org.jruby.exceptions.Unrescuable;
+import org.jruby.runtime.builtin.IRubyObject;
+
+// This class is just a thin wrapper around a return value
+// from nonlocal-return and break instructions.
+//
+// At IR build time, we don't know if a return/break in a block
+// will be a non-local return / break (as in a proc or block)
+// or will just be a regular return (as in a lambda). To ensure
+// uniform instruction semantics at runtime, we push the local return
+// through an exception object (just like IRReturnJump and IRBreakJump)
+// and let it go through exception handlers which ensure that frame/scope
+// are updated properly and ruby-level ensure code is run.
+public class IRWrappedLambdaReturnValue extends RuntimeException implements Unrescuable {
+    public final IRubyObject returnValue;
+
+    public IRWrappedLambdaReturnValue(IRubyObject v) {
+        this.returnValue = v;
+    }
+}