jruby · May 9, 2015 · May 9, 2015
Showing with 26 additions and 60 deletions.

+26 −60 core/src/main/java/org/jruby/ir/IRBuilder.java
diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -133,20 +133,12 @@ public IRLoop(IRScope s, IRLoop outerLoop) {
     }
 
     private static class RescueBlockInfo {
-        RescueNode rescueNode;             // Rescue node for which we are tracking info
         Label      entryLabel;             // Entry of the rescue block
         Variable   savedExceptionVariable; // Variable that contains the saved $! variable
-        IRLoop     innermostLoop;          // Innermost loop within which this rescue block is nested, if any
 
-        public RescueBlockInfo(RescueNode n, Label l, Variable v, IRLoop loop) {
-            rescueNode = n;
+        public RescueBlockInfo(Label l, Variable v) {
             entryLabel = l;
             savedExceptionVariable = v;
-            innermostLoop = loop;
-        }
-
-        public void restoreException(IRBuilder b, IRLoop currLoop) {
-            if (currLoop == innermostLoop) b.addInstr(new PutGlobalVarInstr("$!", savedExceptionVariable));
         }
     }
 
@@ -253,6 +245,7 @@ public void cloneIntoHostScope(IRBuilder builder) {
         }
     }
 
+    // SSS FIXME: Currently only used for retries -- we should be able to eliminate this
     // Stack of nested rescue blocks -- this just tracks the start label of the blocks
     private Stack<RescueBlockInfo> activeRescueBlockStack = new Stack<>();
 
@@ -355,9 +348,7 @@ private void emitEnsureBlocks(IRLoop loop) {
             if (loop != null && ebi.innermostLoop != loop) break;
 
             // SSS FIXME: Should $! be restored before or after the ensure block is run?
-            if (ebi.savedGlobalException != null) {
-                addInstr(new PutGlobalVarInstr("$!", ebi.savedGlobalException));
-            }
+            addInstr(new PutGlobalVarInstr("$!", ebi.savedGlobalException));
 
             // Clone into host scope
             ebi.cloneIntoHostScope(this);
@@ -902,8 +893,6 @@ public Operand buildBreak(BreakNode breakNode) {
         // If we have ensure blocks, have to run those first!
         if (!activeEnsureBlockStack.empty()) {
             emitEnsureBlocks(currLoop);
-        } else if (!activeRescueBlockStack.empty()) {
-            activeRescueBlockStack.peek().restoreException(this, currLoop);
         }
 
         if (currLoop != null) {
@@ -2215,21 +2204,14 @@ Exception region start marker_2 (protected by whichever block handles exceptions
 
      * ****************************************************************/
     public Operand buildEnsureNode(final EnsureNode ensureNode) {
+        return buildEnsureInternal(ensureNode.getBodyNode(), ensureNode.getEnsureNode());
+    }
+
+    public Operand buildEnsureInternal(Node ensureBodyNode, Node ensurerNode) {
+        // Save $!
         final Variable savedGlobalException = createTemporaryVariable();
         addInstr(new GetGlobalVariableInstr(savedGlobalException, "$!"));
 
-        CodeBlock ensureCodeBuilder = new CodeBlock() {
-            public Operand run() {
-                Operand retVal = (ensureNode.getEnsureNode() == null) ? manager.getNil() : build(ensureNode.getEnsureNode());
-                // Restore "$!"
-                addInstr(new PutGlobalVarInstr("$!", savedGlobalException));
-                return retVal;
-            };
-        };
-        return buildEnsureInternal(ensureNode.getBodyNode(), ensureCodeBuilder);
-    }
-
-    public Operand buildEnsureInternal(Node ensureBodyNode, CodeBlock ensureCodeBuilder) {
         // ------------ Build the body of the ensure block ------------
         //
         // The ensure code is built first so that when the protected body is being built,
@@ -2243,7 +2225,10 @@ public Operand buildEnsureInternal(Node ensureBodyNode, CodeBlock ensureCodeBuil
             activeRescuers.peek());
 
         ensureBodyBuildStack.push(ebi);
-        Operand ensureRetVal = ensureCodeBuilder.run();
+        Operand ensureRetVal = ensurerNode == null ? manager.getNil() : build(ensurerNode);
+        // Restore $!
+        addInstr(new PutGlobalVarInstr("$!", savedGlobalException));
+        ebi.savedGlobalException = savedGlobalException;
         ensureBodyBuildStack.pop();
 
         // ------------ Build the protected region ------------
@@ -2768,7 +2753,6 @@ public Operand buildNext(final NextNode nextNode) {
 
         // If we have ensure blocks, have to run those first!
         if (!activeEnsureBlockStack.empty()) emitEnsureBlocks(currLoop);
-        else if (!activeRescueBlockStack.empty()) activeRescueBlockStack.peek().restoreException(this, currLoop);
 
         if (currLoop != null) {
             // If a regular loop, the next is simply a jump to the end of the iteration
@@ -3036,30 +3020,15 @@ public Operand buildRegexp(RegexpNode reNode) {
     }
 
     public Operand buildRescue(RescueNode node) {
-        final Variable savedGlobalException = createTemporaryVariable();
-        addInstr(new GetGlobalVariableInstr(savedGlobalException, "$!"));
-
-        CodeBlock ensureBodyBuilder = new CodeBlock() {
-            public Operand run() {
-                // Restore "$!"
-                addInstr(new PutGlobalVarInstr("$!", savedGlobalException));
-                return manager.getNil();
-            };
-        };
-        return buildEnsureInternal(node, ensureBodyBuilder);
+        return buildEnsureInternal(node, null);
     }
 
     private Operand buildRescueInternal(RescueNode rescueNode, EnsureBlockInfo ensure) {
         // Labels marking start, else, end of the begin-rescue(-ensure)-end block
-        Label rBeginLabel = ensure == null ? getNewLabel() : ensure.regionStart;
-        Label rEndLabel   = ensure == null ? getNewLabel() : ensure.end;
+        Label rBeginLabel = ensure.regionStart;
+        Label rEndLabel   = ensure.end;
         Label rescueLabel = getNewLabel(); // Label marking start of the first rescue code.
 
-        // Save $! in a temp var so it can be restored when the exception gets handled.
-        Variable savedGlobalException = createTemporaryVariable();
-        addInstr(new GetGlobalVariableInstr(savedGlobalException, "$!"));
-        ensure.savedGlobalException = savedGlobalException;
-
         addInstr(new LabelInstr(rBeginLabel));
 
         // Placeholder rescue instruction that tells rest of the compiler passes the boundaries of the rescue block.
@@ -3087,7 +3056,7 @@ private Operand buildRescueInternal(RescueNode rescueNode, EnsureBlockInfo ensur
         //
         // The retry should jump to 1, not 2.
         // If we push the rescue block before building the body, we will jump to 2.
-        RescueBlockInfo rbi = new RescueBlockInfo(rescueNode, rBeginLabel, savedGlobalException, getCurrentLoop());
+        RescueBlockInfo rbi = new RescueBlockInfo(rBeginLabel, ensure.savedGlobalException);
         activeRescueBlockStack.push(rbi);
 
         // Since rescued regions are well nested within Ruby, this bare marker is sufficient to
@@ -3186,17 +3155,13 @@ private void buildRescueBodyInternal(RescueBodyNode rescueBodyNode, Variable rv,
         Node realBody = skipOverNewlines(rescueBodyNode.getBodyNode());
         Operand x = build(realBody);
         if (x != U_NIL) { // can be U_NIL if the rescue block has an explicit return
-            // Restore "$!"
-            RescueBlockInfo rbi = activeRescueBlockStack.peek();
-            addInstr(new PutGlobalVarInstr("$!", rbi.savedExceptionVariable));
-
             // Set up node return value 'rv'
             addInstr(new CopyInstr(rv, x));
 
-            // If we have a matching ensure block, clone it so ensure block runs here
-            if (!activeEnsureBlockStack.empty() && rbi.rescueNode == activeEnsureBlockStack.peek().matchingRescueNode) {
-                activeEnsureBlockStack.peek().cloneIntoHostScope(this);
-            }
+            // Clone the topmost ensure block (which will be a wrapper
+            // around the current rescue block)
+            activeEnsureBlockStack.peek().cloneIntoHostScope(this);
+
             addInstr(new JumpInstr(endLabel));
         }
     }
@@ -3205,6 +3170,12 @@ public Operand buildRetry() {
         // JRuby only supports retry when present in rescue blocks!
         // 1.9 doesn't support retry anywhere else.
 
+        // SSS FIXME: We should be able to use activeEnsureBlockStack for this
+        // But, see the code in buildRescueInternal that pushes/pops these and
+        // the documentation for retries.  There is a small ordering issue
+        // which is preventing me from getting rid of activeRescueBlockStack
+        // altogether!
+        //
         // Jump back to the innermost rescue block
         // We either find it, or we add code to throw a runtime exception
         if (activeRescueBlockStack.empty()) {
@@ -3225,14 +3196,9 @@ private Operand processEnsureRescueBlocks(Operand retVal) {
         // Before we return,
         // - have to go execute all the ensure blocks if there are any.
         //   this code also takes care of resetting "$!"
-        // - if we have a rescue block, reset "$!".
         if (!activeEnsureBlockStack.empty()) {
             retVal = addResultInstr(new CopyInstr(createTemporaryVariable(), retVal));
             emitEnsureBlocks(null);
-        } else if (!activeRescueBlockStack.empty()) {
-            // Restore $!
-            RescueBlockInfo rbi = activeRescueBlockStack.peek();
-            addInstr(new PutGlobalVarInstr("$!", rbi.savedExceptionVariable));
         }
        return retVal;
     }