Fix #3237: Deal with overzealous $! restore from #2910 fixes

Fix for #2910 added a try-catch-finally around bare rescue nodes
to make sure $! was restored on non-local exit paths. However,
we need to make sure that the restore didn't happen on exception
exit paths (for ex. when the exception was not rescued).
Hence #3237.

core/src/main/java/org/jruby/ir/IRBuilder.java

@@ -2225,11 +2225,15 @@ public Operand buildEnsureInternal(Node ensureBodyNode, Node ensurerNode) {
             activeRescuers.peek());
 
         ensureBodyBuildStack.push(ebi);
-        // Restore $! if we the exception was rescued
+
+        // Restore $! if we had a non-empty rescue node
         if (ensureBodyNode != null && ensureBodyNode instanceof RescueNode) {
-            addInstr(new PutGlobalVarInstr("$!", savedGlobalException));
+            if (ensurerNode == null) {
+                addInstr(new PutGlobalVarInstr("$!", savedGlobalException));
+            }
             ebi.savedGlobalException = savedGlobalException;
         }
+
         Operand ensureRetVal = ensurerNode == null ? manager.getNil() : build(ensurerNode);
         ensureBodyBuildStack.pop();
 
@@ -2251,6 +2255,8 @@ public Operand buildEnsureInternal(Node ensureBodyNode, Node ensurerNode) {
         // Clone the ensure body and jump to the end.
         // Don't bother if the protected body ended in a return
         // OR if we are really processing a rescue node
+        //
+        // SSS FIXME: How can ensureBodyNode be anything but a RescueNode (if non-null)
         if (ensurerNode != null && rv != U_NIL && !(ensureBodyNode instanceof RescueNode)) {
             ebi.cloneIntoHostScope(this);
             addInstr(new JumpInstr(ebi.end));
@@ -2266,8 +2272,14 @@ public Operand buildEnsureInternal(Node ensureBodyNode, Node ensurerNode) {
         addInstr(new LabelInstr(ebi.dummyRescueBlockLabel));
         addInstr(new ReceiveJRubyExceptionInstr(exc));
 
+        // Emit code to conditionally restore $!
+        Variable ret = createTemporaryVariable();
+        addInstr(new RuntimeHelperCall(ret, RESTORE_PERLY_EXC, new Operand[]{exc, savedGlobalException} ));
+
         // Now emit the ensure body's stashed instructions
-        ebi.emitBody(this);
+        if (ensurerNode != null) {
+            ebi.emitBody(this);
+        }
 
         // 1. Ensure block has no explicit return => the result of the entire ensure expression is the result of the protected body.
         // 2. Ensure block has an explicit return => the result of the protected body is ignored.

core/src/main/java/org/jruby/ir/instructions/RuntimeHelperCall.java

@@ -1,6 +1,8 @@
 package org.jruby.ir.instructions;
 
 import org.jruby.RubyModule;
+import org.jruby.ir.runtime.IRReturnJump;
+import org.jruby.ir.runtime.IRBreakJump;
 import org.jruby.ir.IRScope;
 import org.jruby.ir.IRVisitor;
 import org.jruby.ir.Operation;
@@ -23,7 +25,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, RESTORE_PERLY_EXC;
 
         public static Methods fromOrdinal(int value) {
             return value < 0 || value >= values().length ? null : values()[value];
@@ -126,6 +128,16 @@ public IRubyObject callHelper(ThreadContext context, StaticScope currScope, Dyna
             case MERGE_KWARGS:
                 return IRRuntimeHelpers.mergeKeywordArguments(context, (IRubyObject) arg1,
                         (IRubyObject) getArgs()[1].retrieve(context, self, currScope, currDynScope, temp));
+            case RESTORE_PERLY_EXC:
+                Object exc = getArgs()[0].retrieve(context, self, currScope, currDynScope, temp);
+                // SSS FIXME: These are non-local control-flow exit scenarios that just
+                // happen to use exceptions for exiting scopes and we should
+                // continue to clear $! for them.
+                if (exc instanceof IRReturnJump || exc instanceof IRBreakJump) {
+                    IRubyObject savedExc = (IRubyObject)getArgs()[1].retrieve(context, self, currScope, currDynScope, temp);
+                    context.runtime.getGlobalVariables().set("$!", savedExc);
+                }
+                return null;
         }
 
         throw new RuntimeException("Unknown IR runtime helper method: " + helperMethod + "; INSTR: " + this);