Mark blocks as escaped and raise LJE for break when appropriate.

Fixes #4686. Fixes #4577. The logic here adds finally wrappers around all call paths that receive a block. When the call exits, the block is marked "escaped" since it no longer has a method activation to go with it. This indicates that non-local flow, like break, should immediately trigger a LocalJumpError. This passes specs but has not been tested extensively with other types of call forms that receive blocks.
jruby · Jun 27, 2017 · 203daad · 203daad
1 parent d3a5ee2
commit 203daad
Showing 8 changed files with 57 additions and 7 deletions.
diff --git a/core/pom.rb b/core/pom.rb
@@ -57,7 +57,7 @@
   jar 'org.jruby.jcodings:jcodings:1.0.18'
   jar 'org.jruby:dirgra:0.3'
 
-  jar 'com.headius:invokebinder:1.7'
+  jar 'com.headius:invokebinder:1.8-SNAPSHOT'
   jar 'com.headius:options:1.4'
   jar 'com.headius:unsafe-fences:1.0'
 

diff --git a/core/pom.xml b/core/pom.xml
@@ -193,7 +193,7 @@ DO NOT MODIFIY - GENERATED CODE
     <dependency>
       <groupId>com.headius</groupId>
       <artifactId>invokebinder</artifactId>
-      <version>1.7</version>
+      <version>1.8-SNAPSHOT</version>
     </dependency>
     <dependency>
       <groupId>com.headius</groupId>

diff --git a/core/src/main/java/org/jruby/ir/instructions/CallBase.java b/core/src/main/java/org/jruby/ir/instructions/CallBase.java
@@ -425,6 +425,14 @@ public Object interpret(ThreadContext context, StaticScope currScope, DynamicSco
         IRubyObject[] values = prepareArguments(context, self, currScope, dynamicScope, temp);
         Block preparedBlock = prepareBlock(context, self, currScope, dynamicScope, temp);
 
+        if (getClosureArg() != null) {
+            try {
+                return callSite.call(context, self, object, values, preparedBlock);
+            } finally {
+                preparedBlock.escape();
+            }
+        }
+
         return callSite.call(context, self, object, values, preparedBlock);
     }
 

diff --git a/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
@@ -433,7 +433,7 @@ protected static IRubyObject processReturnOp(ThreadContext context, Block block,
                 // This assumes that scopes with break instr. have a frame / dynamic scope
                 // pushed so that we can get to its static scope. For-loops now always have
                 // a dyn-scope pushed onto stack which makes this work in all scenarios.
-                return IRRuntimeHelpers.initiateBreak(context, currDynScope, rv, blockType);
+                return IRRuntimeHelpers.initiateBreak(context, currDynScope, rv, block, blockType);
             }
             case NONLOCAL_RETURN: {
                 NonlocalReturnInstr ri = (NonlocalReturnInstr)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
@@ -159,15 +159,21 @@ private static IRScopeType ensureScopeIsClosure(ThreadContext context, DynamicSc
     }
 
     // FIXME: When we recompile lambdas we can eliminate this binary code path and we can emit as a NONLOCALRETURN directly.
-    public static IRubyObject initiateBreak(ThreadContext context, DynamicScope dynScope, IRubyObject breakValue, Block.Type blockType) throws RuntimeException {
+    public static IRubyObject initiateBreak(ThreadContext context, DynamicScope dynScope, IRubyObject breakValue, Block block, Block.Type blockType) throws RuntimeException {
         // 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.
         if (inLambda(blockType)) throw new IRWrappedLambdaReturnValue(breakValue);
 
         IRScopeType scopeType = ensureScopeIsClosure(context, dynScope);
 
+        DynamicScope parentScope = dynScope.getParentScope();
+
+        if (block.isEscaped()) {
+            throw context.runtime.newLocalJumpError(RubyLocalJumpError.Reason.BREAK, breakValue, "unexpected break");
+        }
+
         // Raise a break jump so we can bubble back down the stack to the appropriate place to break from.
-        throw IRBreakJump.create(dynScope.getParentScope(), breakValue, scopeType.isEval()); // weirdly evals are impld as closures...yes yes.
+        throw IRBreakJump.create(parentScope, breakValue, scopeType.isEval()); // weirdly evals are impld as closures...yes yes.
     }
 
     // Are we within the scope where we want to return the value we are passing down the stack?

diff --git a/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter6.java b/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter6.java
@@ -500,7 +500,7 @@ public void invoke(String file, int lineNumber, String name, int arity, boolean
                 break;
         }
 
-        adapter2.invokevirtual(p(CachingCallSite.class), "call", outgoingSig);
+        adapter2.invokevirtual(p(CachingCallSite.class), hasClosure ? "callIter" : "call", outgoingSig);
         adapter2.areturn();
         adapter2.end();
 

diff --git a/core/src/main/java/org/jruby/ir/targets/InvokeSite.java b/core/src/main/java/org/jruby/ir/targets/InvokeSite.java
@@ -33,6 +33,9 @@
 import java.lang.invoke.MethodType;
 import java.lang.invoke.MutableCallSite;
 import java.lang.invoke.SwitchPoint;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
 import java.util.concurrent.atomic.AtomicLong;
 
 import static java.lang.invoke.MethodHandles.lookup;
@@ -53,6 +56,7 @@ public abstract class InvokeSite extends MutableCallSite {
     protected final String file;
     protected final int line;
     private boolean boundOnce;
+    private boolean closure;
     CacheEntry cache = CacheEntry.NULL_CACHE;
 
     private static final Logger LOG = LoggerFactory.getLogger(InvokeSite.class);
@@ -95,6 +99,7 @@ public InvokeSite(MethodType type, String name, CallType callType, String file,
             }
             startSig = startSig.appendArg("block", Block.class);
             fullSignature = signature = startSig;
+            closure = true;
         } else {
             arity = type.parameterCount() - argOffset;
 
@@ -140,11 +145,41 @@ public IRubyObject invoke(ThreadContext context, IRubyObject caller, IRubyObject
 
         MethodHandle mh = getHandle(self, selfClass, method);
 
+        if (closure) {
+            mh = Binder.from(mh.type())
+                    .tryFinally(getBlockEscape(signature))
+                    .invoke(mh);
+        }
+
         updateInvocationTarget(mh, self, selfClass, entry.method, switchPoint);
 
+        if (closure) {
+            try {
+                return method.call(context, self, selfClass, methodName, args, block);
+            } finally {
+                block.escape();
+            }
+        }
+
         return method.call(context, self, selfClass, methodName, args, block);
     }
 
+    private static final MethodHandle ESCAPE_BLOCK = Binder.from(void.class, Block.class).invokeVirtualQuiet(lookup(), "escape");
+    private static final Map<Signature, MethodHandle> BLOCK_ESCAPES = Collections.synchronizedMap(new HashMap<Signature, MethodHandle>());
+
+    private static MethodHandle getBlockEscape(Signature signature) {
+        Signature voidSignature = signature.changeReturn(void.class);
+        MethodHandle escape = BLOCK_ESCAPES.get(voidSignature);
+        if (escape == null) {
+            escape = SmartBinder.from(voidSignature)
+                    .permute("block")
+                    .invoke(ESCAPE_BLOCK)
+                    .handle();
+            BLOCK_ESCAPES.put(voidSignature, escape);
+        }
+        return escape;
+    }
+
     /**
      * Failover version uses a monomorphic cache and DynamicMethod.call, as in non-indy.
      */

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -854,8 +854,9 @@ public void BreakInstr(BreakInstr breakInstr) {
         jvmMethod().loadContext();
         jvmLoadLocal(DYNAMIC_SCOPE);
         visit(breakInstr.getReturnValue());
+        jvmMethod().loadSelfBlock();
         jvmMethod().loadBlockType();
-        jvmAdapter().invokestatic(p(IRRuntimeHelpers.class), "initiateBreak", sig(IRubyObject.class, ThreadContext.class, DynamicScope.class, IRubyObject.class, Block.Type.class));
+        jvmAdapter().invokestatic(p(IRRuntimeHelpers.class), "initiateBreak", sig(IRubyObject.class, ThreadContext.class, DynamicScope.class, IRubyObject.class, Block.class, Block.Type.class));
         jvmMethod().returnValue();
 
     }