Fix a bunch of issues with cloning.

* Caught while trying to use cloned instructions in interpreter. So far, cloning was only being used for ensure-blocks and in IRBuilder where these bugs were less problematic or at least never caused problems so far.
jruby · Oct 11, 2014 · c02df12 · c02df12
1 parent 7a8bf5f
commit c02df12
Showing 8 changed files with 19 additions and 11 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRClosure.java b/core/src/main/java/org/jruby/ir/IRClosure.java
@@ -285,7 +285,9 @@ public int getNestingDepth() {
 
     protected IRClosure cloneForInlining(CloneInfo ii, IRClosure clone) {
         clone.nestingDepth  = this.nestingDepth;
-        clone.parameterList = this.parameterList;
+        // FIXME: This is fragile. Untangle this state.
+        // Why is this being copied over to InterpretedIRBlockBody?
+        clone.setParameterList(this.parameterList);
 
         SimpleCloneInfo clonedII = ii.cloneForCloningClosure(clone);
 

diff --git a/core/src/main/java/org/jruby/ir/instructions/Instr.java b/core/src/main/java/org/jruby/ir/instructions/Instr.java
@@ -177,9 +177,7 @@ public void renameVars(Map<Operand, Operand> renameMap) {
      *                    args and return values.
      * @return a new instruction that can be used in the target scope.
      */
-    public Instr clone(CloneInfo info) {
-        throw new RuntimeException("clone: Not implemented for: " + this.getOperation());
-    }
+    public abstract Instr clone(CloneInfo info);
 
     /**
      * This method takes as input a map of operands to their values, and outputs

diff --git a/core/src/main/java/org/jruby/ir/instructions/LineNumberInstr.java b/core/src/main/java/org/jruby/ir/instructions/LineNumberInstr.java
@@ -29,8 +29,8 @@ public String toString() {
 
     @Override
     public Instr clone(CloneInfo ii) {
-        // SSS FIXME: This is buggy! 'scope' might have changed because of cloning.
-        return this;
+        // Use original scope even if inlined -- so debugging / stack-traces are meaningful
+        return new LineNumberInstr(scope, lineNumber);
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/instructions/RaiseArgumentErrorInstr.java b/core/src/main/java/org/jruby/ir/instructions/RaiseArgumentErrorInstr.java
@@ -54,7 +54,7 @@ public int getRest() {
 
     @Override
     public Instr clone(CloneInfo ii) {
-        return this;
+        return new RaiseArgumentErrorInstr(required, opt, rest, numArgs);
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/instructions/RaiseRequiredKeywordArgumentError.java b/core/src/main/java/org/jruby/ir/instructions/RaiseRequiredKeywordArgumentError.java
@@ -3,6 +3,7 @@
 import org.jruby.ir.IRVisitor;
 import org.jruby.ir.Operation;
 import org.jruby.ir.operands.Operand;
+import org.jruby.ir.transformations.inlining.CloneInfo;
 import org.jruby.parser.StaticScope;
 import org.jruby.runtime.DynamicScope;
 import org.jruby.runtime.ThreadContext;
@@ -25,6 +26,11 @@ public Operand[] getOperands() {
         return new Operand[0];
     }
 
+    @Override
+    public Instr clone(CloneInfo ii) {
+        return new RaiseRequiredKeywordArgumentError(name);
+    }
+
     @Override
     public Object interpret(ThreadContext context, StaticScope currScope, DynamicScope currDynScope, IRubyObject self, Object[] temp) {
         throw context.runtime.newArgumentError("missing keyword: " + name);

diff --git a/core/src/main/java/org/jruby/ir/instructions/ThreadPollInstr.java b/core/src/main/java/org/jruby/ir/instructions/ThreadPollInstr.java
@@ -26,10 +26,10 @@ public Operand[] getOperands() {
 
     @Override
     public Instr clone(CloneInfo ii) {
-        if (ii instanceof SimpleCloneInfo) return this;
+        if (ii instanceof SimpleCloneInfo) return new ThreadPollInstr(onBackEdge);
 
         // Get rid of non-back-edge thread-poll instructions when scopes are inlined
-        return onBackEdge ? this : null;
+        return onBackEdge ? new ThreadPollInstr(onBackEdge) : null;
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/operands/DynamicSymbol.java b/core/src/main/java/org/jruby/ir/operands/DynamicSymbol.java
@@ -35,7 +35,7 @@ public void addUsedVariables(List<Variable> l) {
     }
 
     public Operand cloneForInlining(CloneInfo ii) {
-        return symbolName.cloneForInlining(ii);
+        return new DynamicSymbol(symbolName.cloneForInlining(ii));
     }
 
     @Override

diff --git a/core/src/main/java/org/jruby/ir/operands/Label.java b/core/src/main/java/org/jruby/ir/operands/Label.java
@@ -50,7 +50,9 @@ public boolean equals(Object o) {
     }
 
     public Label clone() {
-        return new Label(prefix, id);
+        Label newL = new Label(prefix, id);
+        newL.setTargetPC(getTargetPC()); // Strictly not necessary, but, copy everything over
+        return newL;
     }
 
     @Override