don't replace the operand value when creating a new stack local when merging

Timbals · Timbals · commit 949ed252630c · 2023-11-08T16:26:51.000+01:00
Instead, rely on the fact that all `convert` methods override their respective statements.
That means just changing the incoming operands will automatically correctly update the statement.

Also added a test case to make sure this works with the special case of using a normal local as a stack local.
diff --git a/shared-test-resources/miniTestSuite/java6/binary/LocalMerging.class b/shared-test-resources/miniTestSuite/java6/binary/LocalMerging.class
diff --git a/shared-test-resources/miniTestSuite/java6/source/LocalMerging.java b/shared-test-resources/miniTestSuite/java6/source/LocalMerging.java
@@ -24,4 +24,10 @@ public void localMergingWithDuplicateValue(int n) {
         // the second argument isn't replaced as well.
         System.setProperty(n == 1 ? a : "two", "two");
     }
+
+    public void localMergingWithInlining(int n) {
+        String[] arr = new String[] {"a", "b"};
+        int a = 1;
+        String b = arr[n == 1 ? 0 : a];
+    }
 }
diff --git a/sootup.java.bytecode/src/main/java/sootup/java/bytecode/frontend/AsmMethodSource.java b/sootup.java.bytecode/src/main/java/sootup/java/bytecode/frontend/AsmMethodSource.java
@@ -1215,14 +1215,13 @@ private void convertVarStoreInsn(@Nonnull VarInsnNode insn) {
     merging.mergeInputs(opr);
     Local local = getOrCreateLocal(insn.var);
     AbstractDefinitionStmt as;
-    if (opr.stackLocal == null) {
+    if (opr.stackLocal == null || opr.stackLocal == local) {
       // Can skip creating a new stack local for the operand
       // and store the value in the local directly.
       as = Jimple.newAssignStmt(local, opr.value, getStmtPositionInfo());
-      // TODO check that this works correctly with the merging
       opr.stackLocal = local;
       setStmt(opr.insn, as);
-    } else if (opr.stackLocal != local) {
+    } else {
       as = Jimple.newAssignStmt(local, opr.toImmediate(), getStmtPositionInfo());
       setStmt(insn, as);
     }
diff --git a/sootup.java.bytecode/src/main/java/sootup/java/bytecode/frontend/Operand.java b/sootup.java.bytecode/src/main/java/sootup/java/bytecode/frontend/Operand.java
@@ -46,9 +46,6 @@ class Operand {
 
   @Nonnull protected AbstractInsnNode insn;
   @Nonnull protected final Value value;
-  // TODO probably need to store the `insn` for the STORE instruction when a *real*
-  //  local is used here and use that when merging,
-  //  or more specifically when changing to a different stack local in `changeStackLocal`
   @Nullable protected Local stackLocal;
   @Nonnull private final AsmMethodSource methodSource;
 
@@ -78,11 +75,6 @@ void emitStatement() {
       return;
     }
 
-    if (methodSource.getStmt(insn) != null) {
-      // the operand is already used, which means side effects already happen as well
-      return;
-    }
-
     if (value instanceof AbstractInvokeExpr) {
       methodSource.setStmt(
           insn,
diff --git a/sootup.java.bytecode/src/main/java/sootup/java/bytecode/frontend/OperandMerging.java b/sootup.java.bytecode/src/main/java/sootup/java/bytecode/frontend/OperandMerging.java
@@ -23,10 +23,7 @@
 import java.util.ArrayList;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
-import org.objectweb.asm.tree.AbstractInsnNode;
 import sootup.core.jimple.basic.Local;
-import sootup.core.jimple.common.stmt.Stmt;
-import sootup.core.jimple.visitor.ReplaceUseStmtVisitor;
 
 /**
  * This class tracks the inputs and outputs for an instruction.
@@ -38,8 +35,6 @@
  * @author Aaloan Miftah
  */
 final class OperandMerging {
-  @Nonnull private final AbstractInsnNode insn;
-
   /**
    * Keep track of the result of the instruction. The output might get a stack local assigned when
    * it is used as a local or immediate. When another branch produces the output and calls
@@ -58,8 +53,7 @@ final class OperandMerging {
    *
    * @param src source the merging belongs to.
    */
-  OperandMerging(@Nonnull AbstractInsnNode insn, @Nonnull AsmMethodSource src) {
-    this.insn = insn;
+  OperandMerging(@Nonnull AsmMethodSource src) {
     this.src = src;
   }
 
@@ -148,8 +142,6 @@ void mergeInputs(@Nonnull Operand... oprs) {
 
       // Didn't find any pre-allocated stack local from any operand.
       // So create a new stack local.
-      // TODO use a special case when the statement is an assignment to a local since in that case
-      //  we can use the local directly instead of creating a new stack local
       if (stack == null) {
         stack = src.newStackLocal();
       }
@@ -160,28 +152,6 @@ void mergeInputs(@Nonnull Operand... oprs) {
         prevOp.changeStackLocal(stack);
       }
       newOp.changeStackLocal(stack);
-
-      // TODO `in.get(0)` is weird because of the index?
-      // TODO make it more obvious that this is only run the first time
-      // replace the operand in the statement that *started* the merge
-      ReplaceUseStmtVisitor replaceUseStmtVisitor =
-          new ReplaceUseStmtVisitor(inputOperands.get(0)[i].value, stack);
-      // TODO how to handle the same value being in the the statement multiple times but only one
-      //  time because of the operand? (Something like `System.out.println(operand, "hello")` with
-      //  the operand also having the value "two")
-      //  this might require a callback(?) to change the statement; alternative we could do the
-      //  merging *before* constructing the statement and then replace the statement if it differs
-      //  from an already existing one
-      //  This actually works right now because the `ReplaceUseExprVisitor` only checks object
-      //  equality meaning the
-      //  two instances of the constant are different and only the correct instance is replaced
-      Stmt oldStatement = this.src.getStmt(this.insn);
-      // TODO `oldStatement` might not exist when a STORE instruction was used to set the
-      //  stackLocal
-      if (oldStatement != null) {
-        oldStatement.accept(replaceUseStmtVisitor);
-        this.src.replaceStmt(oldStatement, replaceUseStmtVisitor.getResult());
-      }
     }
 
     inputOperands.add(oprs);
diff --git a/sootup.java.bytecode/src/main/java/sootup/java/bytecode/frontend/OperandStack.java b/sootup.java.bytecode/src/main/java/sootup/java/bytecode/frontend/OperandStack.java
@@ -48,7 +48,7 @@ public OperandStack(@Nonnull AsmMethodSource methodSource, int nrInsn) {
   public OperandMerging getOrCreateMerging(@Nonnull AbstractInsnNode insn) {
     OperandMerging merging = this.mergings.get(insn);
     if (merging == null) {
-      merging = new OperandMerging(insn, methodSource);
+      merging = new OperandMerging(methodSource);
       this.mergings.put(insn, merging);
     }
     return merging;
diff --git a/sootup.java.bytecode/src/test/java/sootup/java/bytecode/minimaltestsuite/java6/LocalMergingTest.java b/sootup.java.bytecode/src/test/java/sootup/java/bytecode/minimaltestsuite/java6/LocalMergingTest.java
@@ -40,6 +40,15 @@ public void test() {
                 "void",
                 Collections.singletonList("int")));
     assertJimpleStmts(methodDuplicateValue, expectedBodyStmtsDuplicateValue());
+
+    SootMethod methodWithInlining =
+        loadMethod(
+            identifierFactory.getMethodSignature(
+                getDeclaredClassSignature(),
+                "localMergingWithInlining",
+                "void",
+                Collections.singletonList("int")));
+    assertJimpleStmts(methodWithInlining, expectedBodyStmtsWithInlining());
   }
 
   /**
@@ -133,4 +142,35 @@ public List<String> expectedBodyStmtsDuplicateValue() {
             "return")
         .collect(Collectors.toList());
   }
+
+  /**
+   *
+   *
+   * <pre>
+   * public void localMergingWithInlining(int n) {
+   *     String[] arr = new String[] {"a", "b"};
+   *     int a = 1;
+   *     String b = arr[n == 1 ? 0 : a];
+   * }
+   * </pre>
+   */
+  public List<String> expectedBodyStmtsWithInlining() {
+    return Stream.of(
+            "$l0 := @this: LocalMerging",
+            "$l1 := @parameter0: int",
+            "$stack5 = newarray (java.lang.String)[2]",
+            "$stack5[0] = \"a\"",
+            "$stack5[1] = \"b\"",
+            "$l2 = $stack5",
+            "$l3 = 1",
+            "if $l1 != 1 goto label1",
+            "$stack6 = 0",
+            "goto label2",
+            "label1:",
+            "$stack6 = $l3",
+            "label2:",
+            "$l4 = $l2[$stack6]",
+            "return")
+        .collect(Collectors.toList());
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -24,4 +24,10 @@ public void localMergingWithDuplicateValue(int n) {`
`24`	`24`	`// the second argument isn't replaced as well.`
`25`	`25`	`System.setProperty(n == 1 ? a : "two", "two");`
`26`	`26`	`}`
	`27`	`+`
	`28`	`+ public void localMergingWithInlining(int n) {`
	`29`	`+ String[] arr = new String[] {"a", "b"};`
	`30`	`+ int a = 1;`
	`31`	`+ String b = arr[n == 1 ? 0 : a];`
	`32`	`+ }`
`27`	`33`	`}`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ public OperandStack(@Nonnull AsmMethodSource methodSource, int nrInsn) {`
`48`	`48`	`public OperandMerging getOrCreateMerging(@Nonnull AbstractInsnNode insn) {`
`49`	`49`	`OperandMerging merging = this.mergings.get(insn);`
`50`	`50`	`if (merging == null) {`
`51`		`- merging = new OperandMerging(insn, methodSource);`
	`51`	`+ merging = new OperandMerging(methodSource);`
`52`	`52`	`this.mergings.put(insn, merging);`
`53`	`53`	`}`
`54`	`54`	`return merging;`