Fix 2132: Split LJE check from NonLocalReturnInstr into separate instr

* Non local returns can throw LJEs in the scope they originate. However they could be rescued by existing rescue-handlers. * In the previous design where the LJE-checking happened inside the runtime, it was too late for it to be rescued and not propagate outwards. For example, ensure blocks from the scope would run before the non-local return executed. However, if the LJE had been rescued, control wouldn't have reached the outer begin-end construct to execute the ensure block (see the regression spec to make sense of this). * This patch splits out the LJE check into its own runtime helper instruction so that the non-local return itself is a return without any exception that could be raised. * Added a regression spec.
jruby · Nov 11, 2014 · 9ba99c1 · 9ba99c1
1 parent db1919c
commit 9ba99c1
Showing 10 changed files with 83 additions and 27 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -3205,9 +3205,7 @@ public Operand buildRetry(Node node, IRScope s) {
         return manager.getNil();
     }
 
-    public Operand buildReturn(ReturnNode returnNode, IRScope s) {
-        Operand retVal = (returnNode.getValueNode() == null) ? manager.getNil() : build(returnNode.getValueNode(), s);
-
+    private Operand processEnsureRescueBlocks(IRScope s, Operand retVal) { 
         // Before we return,
         // - have to go execute all the ensure blocks if there are any.
         //   this code also takes care of resetting "$!"
@@ -3220,20 +3218,29 @@ public Operand buildReturn(ReturnNode returnNode, IRScope s) {
             RescueBlockInfo rbi = activeRescueBlockStack.peek();
             addInstr(s, new PutGlobalVarInstr("$!", rbi.savedExceptionVariable));
         }
+       return retVal;
+    }
+
+    public Operand buildReturn(ReturnNode returnNode, IRScope s) {
+        Operand retVal = (returnNode.getValueNode() == null) ? manager.getNil() : build(returnNode.getValueNode(), s);
 
         if (s instanceof IRClosure) {
             // If 'm' is a block scope, a return returns from the closest enclosing method.
             // If this happens to be a module body, the runtime throws a local jump error if the
             // closure is a proc. If the closure is a lambda, then this becomes a normal return.
             IRMethod m = s.getNearestMethod();
-            addInstr(s, new NonlocalReturnInstr(retVal, m == null ? "--none--" : m.getName(), m == null));
+            addInstr(s, new RuntimeHelperCall(null, CHECK_FOR_LJE, new Operand[] { new Boolean(m == null) }));
+            retVal = processEnsureRescueBlocks(s, retVal);
+            addInstr(s, new NonlocalReturnInstr(retVal, m == null ? "--none--" : m.getName()));
         } else if (s.isModuleBody()) {
             IRMethod sm = s.getNearestMethod();
 
             // Cannot return from top-level module bodies!
             if (sm == null) addInstr(s, new ThrowExceptionInstr(IRException.RETURN_LocalJumpError));
-            else addInstr(s, new NonlocalReturnInstr(retVal, sm.getName(), false));
+            retVal = processEnsureRescueBlocks(s, retVal);
+            if (sm != null) addInstr(s, new NonlocalReturnInstr(retVal, sm.getName()));
         } else {
+            retVal = processEnsureRescueBlocks(s, retVal);
             addInstr(s, new ReturnInstr(retVal));
         }
 

diff --git a/core/src/main/java/org/jruby/ir/Operation.java b/core/src/main/java/org/jruby/ir/Operation.java
@@ -85,8 +85,7 @@ public enum Operation {
 
     /** returns -- returns unwind stack, etc. */
     RETURN(OpFlags.f_has_side_effect | OpFlags.f_is_return),
-    /** a non local return Can thrown a LocalJumpError */
-    NONLOCAL_RETURN(OpFlags.f_has_side_effect | OpFlags.f_is_return | OpFlags.f_can_raise_exception),
+    NONLOCAL_RETURN(OpFlags.f_has_side_effect | OpFlags.f_is_return),
     /* 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),

diff --git a/core/src/main/java/org/jruby/ir/instructions/NonlocalReturnInstr.java b/core/src/main/java/org/jruby/ir/instructions/NonlocalReturnInstr.java
@@ -11,22 +11,20 @@
 
 public class NonlocalReturnInstr extends ReturnBase implements FixedArityInstr {
     public final String methodName; // Primarily a debugging aid
-    public final boolean maybeLambda;
 
-    public NonlocalReturnInstr(Operand returnValue, String methodName, boolean maybeLambda) {
+    public NonlocalReturnInstr(Operand returnValue, String methodName) {
         super(Operation.NONLOCAL_RETURN, returnValue);
         this.methodName = methodName;
-        this.maybeLambda = maybeLambda;
     }
 
     @Override
     public Operand[] getOperands() {
-        return new Operand[] { returnValue, new StringLiteral(methodName), new Boolean(maybeLambda) };
+        return new Operand[] { returnValue, new StringLiteral(methodName) };
     }
 
     @Override
     public String toString() {
-        return getOperation() + "(" + returnValue + ", <" + methodName + ":" + maybeLambda + ">" + ")";
+        return getOperation() + "(" + returnValue + ", <" + methodName + ">" + ")";
     }
 
     public boolean computeScopeFlags(IRScope scope) {
@@ -36,7 +34,7 @@ public boolean computeScopeFlags(IRScope scope) {
 
     @Override
     public Instr clone(CloneInfo info) {
-        if (info instanceof SimpleCloneInfo) return new NonlocalReturnInstr(returnValue.cloneForInlining(info), methodName, maybeLambda);
+        if (info instanceof SimpleCloneInfo) return new NonlocalReturnInstr(returnValue.cloneForInlining(info), methodName);
 
         InlineCloneInfo ii = (InlineCloneInfo) info;
         if (ii.isClosure()) {
@@ -46,7 +44,7 @@ public Instr clone(CloneInfo info) {
                 return v == null ? null : new CopyInstr(v, returnValue.cloneForInlining(ii));
             }
 
-            return new NonlocalReturnInstr(returnValue.cloneForInlining(ii), methodName, maybeLambda);
+            return new NonlocalReturnInstr(returnValue.cloneForInlining(ii), methodName);
         } else {
             throw new UnsupportedOperationException("Nonlocal returns shouldn't show up outside closures.");
         }

diff --git a/core/src/main/java/org/jruby/ir/instructions/RuntimeHelperCall.java b/core/src/main/java/org/jruby/ir/instructions/RuntimeHelperCall.java
@@ -24,7 +24,7 @@ public enum Methods {
         HANDLE_PROPAGATE_BREAK, HANDLE_NONLOCAL_RETURN, HANDLE_BREAK_AND_RETURNS_IN_LAMBDA,
         IS_DEFINED_BACKREF, IS_DEFINED_NTH_REF, IS_DEFINED_GLOBAL, IS_DEFINED_INSTANCE_VAR,
         IS_DEFINED_CLASS_VAR, IS_DEFINED_SUPER, IS_DEFINED_METHOD, IS_DEFINED_CALL,
-        IS_DEFINED_CONSTANT_OR_METHOD, MERGE_KWARGS
+        IS_DEFINED_CONSTANT_OR_METHOD, MERGE_KWARGS, CHECK_FOR_LJE
     };
 
     Variable  result;
@@ -150,6 +150,9 @@ public IRubyObject callHelper(ThreadContext context, StaticScope currScope, Dyna
                 return IRRuntimeHelpers.mergeKeywordArguments(context,
                         (IRubyObject) args[0].retrieve(context, self, currScope, currDynScope, temp),
                         (IRubyObject) args[1].retrieve(context, self, currScope, currDynScope, temp));
+            case CHECK_FOR_LJE:
+                IRRuntimeHelpers.checkForLJE(context, currDynScope, ((Boolean)args[0]).isTrue(), blockType);
+                return null;
         }
 
         throw new RuntimeException("Unknown IR runtime helper method: " + helperMethod + "; INSTR: " + this);

diff --git a/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java b/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java
@@ -440,7 +440,7 @@ private static IRubyObject processReturnOp(ThreadContext context, Instr instr, O
         case NONLOCAL_RETURN: {
             NonlocalReturnInstr ri = (NonlocalReturnInstr)instr;
             IRubyObject rv = (IRubyObject)retrieveOp(ri.getReturnValue(), context, self, currDynScope, currScope, temp);
-            return IRRuntimeHelpers.initiateNonLocalReturn(context, currDynScope, blockType, ri.maybeLambda, rv);
+            return IRRuntimeHelpers.initiateNonLocalReturn(context, currDynScope, blockType, rv);
         }
         }
         return null;
@@ -494,7 +494,9 @@ private static void processOtherOp(ThreadContext context, Instr instr, Operation
         case RUNTIME_HELPER: {
             RuntimeHelperCall rhc = (RuntimeHelperCall)instr;
             result = rhc.callHelper(context, currScope, currDynScope, self, temp, blockType);
-            setResult(temp, currDynScope, rhc.getResult(), result);
+            if (rhc.getResult() != null) {
+                setResult(temp, currDynScope, rhc.getResult(), result);
+            }
             break;
         }
 

diff --git a/core/src/main/java/org/jruby/ir/persistence/InstrDecoderMap.java b/core/src/main/java/org/jruby/ir/persistence/InstrDecoderMap.java
@@ -86,7 +86,7 @@ public Instr decodeInner(Operation operation) {
             case MATCH2: return new Match2Instr(d.decodeVariable(), d.decodeOperand(), d.decodeOperand());
             case MATCH3: return new Match3Instr(d.decodeVariable(), d.decodeOperand(), d.decodeOperand());
             case METHOD_LOOKUP: return new MethodLookupInstr(d.decodeVariable(), d.decodeOperand(), d.decodeOperand());
-            case NONLOCAL_RETURN: return new NonlocalReturnInstr(d.decodeOperand(), d.decodeString(), d.decodeBoolean());
+            case NONLOCAL_RETURN: return new NonlocalReturnInstr(d.decodeOperand(), d.decodeString());
             case NOP: return NopInstr.NOP;
             case NORESULT_CALL: return decodeNoResultCall();
             case POP_BINDING: return new PopBindingInstr();

diff --git a/core/src/main/java/org/jruby/ir/persistence/InstrEncoderMap.java b/core/src/main/java/org/jruby/ir/persistence/InstrEncoderMap.java
@@ -356,7 +356,6 @@ private void encodeMethodLookupInstr(MethodLookupInstr instr) {
     private void encodeNonlocalReturnInstr(NonlocalReturnInstr instr) {
         e.encode(instr.getReturnValue());
         e.encode(instr.methodName);
-        e.encode(instr.maybeLambda);
     }
 
     private void encodeCallBaseInstr(CallBase instr) {

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -67,12 +67,8 @@ public static boolean inProc(Block.Type blockType) {
         return blockType == Block.Type.PROC;
     }
 
-    /*
-     * 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, boolean maybeLambda, IRubyObject returnValue) {
-        // If not in a lambda, check if this was a non-local return
-        if (IRRuntimeHelpers.inLambda(blockType)) return returnValue;
+    public static void checkForLJE(ThreadContext context, DynamicScope dynScope, boolean maybeLambda, Block.Type blockType) {
+        if (IRRuntimeHelpers.inLambda(blockType)) return;
 
         StaticScope scope = dynScope.getStaticScope();
         IRScopeType scopeType = scope.getScopeType();
@@ -109,6 +105,32 @@ public static IRubyObject initiateNonLocalReturn(ThreadContext context, DynamicS
             // Cannot return from the call that we have long since exited.
             throw IRException.RETURN_LocalJumpError.getException(context.runtime);
         }
+    }
+
+    /*
+     * 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;
+
+        IRScopeType scopeType = dynScope.getStaticScope().getScopeType();
+        while (dynScope != null) {
+            StaticScope ss = dynScope.getStaticScope();
+            // SSS FIXME: Why is scopeType empty? Looks like this static-scope
+            // was not associated with the AST scope that got converted to IR.
+            //
+            // Ruby code: lambda { Thread.new { return }.join }.call
+            //
+            // To be investigated.
+            IRScopeType ssType = ss.getScopeType();
+            if (ssType != null) {
+                if (ssType.isMethodType() || (ss.isArgumentScope() && ssType.isClosureType() && ssType != IRScopeType.EVAL_SCRIPT)) {
+                    break;
+                }
+            }
+            dynScope = dynScope.getParentScope();
+        }
 
         // methodtoReturnFrom will not be -1 for explicit returns from class/module/sclass bodies
         throw IRReturnJump.create(dynScope, returnValue);

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -1628,6 +1628,13 @@ public void RuntimeHelperCall(RuntimeHelperCall runtimehelpercall) {
                 jvmAdapter().invokestatic(p(IRRuntimeHelpers.class), "mergeKeywordArguments", sig(IRubyObject.class, ThreadContext.class, IRubyObject.class, IRubyObject.class));
                 jvmStoreLocal(runtimehelpercall.getResult());
                 break;
+            case CHECK_FOR_LJE:
+                jvmMethod().loadContext();
+                jvmLoadLocal(DYNAMIC_SCOPE);
+                jvmMethod().pushBoolean(((Boolean)runtimehelpercall.getArgs()[0]).isTrue());
+                jvmMethod().loadBlockType();
+                jvmAdapter().invokestatic(p(IRRuntimeHelpers.class), "checkForLJE", sig(void.class, ThreadContext.class, DynamicScope.class, boolean.class, Block.Type.class));
+                break;
             default:
                 throw new NotCompilableException("Unknown IR runtime helper method: " + runtimehelpercall.getHelperMethod() + "; INSTR: " + this);
         }
@@ -1638,10 +1645,9 @@ public void NonlocalReturnInstr(NonlocalReturnInstr returninstr) {
         jvmMethod().loadContext();
         jvmLoadLocal(DYNAMIC_SCOPE);
         jvmMethod().loadBlockType();
-        jvmAdapter().ldc(returninstr.maybeLambda);
         visit(returninstr.getReturnValue());
 
-        jvmMethod().invokeIRHelper("initiateNonLocalReturn", sig(IRubyObject.class, ThreadContext.class, DynamicScope.class, Block.Type.class, boolean.class, IRubyObject.class));
+        jvmMethod().invokeIRHelper("initiateNonLocalReturn", sig(IRubyObject.class, ThreadContext.class, DynamicScope.class, Block.Type.class, IRubyObject.class));
         jvmMethod().returnValue();
     }
 

diff --git a/spec/regression/GH-2132_catch_nonlocal_return_lje.rb b/spec/regression/GH-2132_catch_nonlocal_return_lje.rb
@@ -0,0 +1,20 @@
+# https://github.com/jruby/jruby/issues/2132
+
+describe 'NonLocalReturn' do
+  it 'throwing a LocalJumpError should be properly rescued' do
+    ret = [];
+    Thread.new {
+      begin
+        begin
+          return
+        rescue StandardError => e
+          ret << "LJE"
+        end
+        ret << "out"
+      ensure
+        ret << "ensured"
+      end
+    }.join
+    expect(ret).to eq(["LJE", "out", "ensured"])
+  end
+end