Fix bug where CopyPropagator did not correctly handle PhiExpr

src/main/java/soot/jimple/toolkits/scalar/CopyPropagator.java

@@ -52,6 +52,8 @@
 import soot.jimple.Stmt;
 import soot.options.CPOptions;
 import soot.options.Options;
+import soot.shimple.PhiExpr;
+import soot.shimple.ShimpleBody;
 import soot.tagkit.Host;
 import soot.tagkit.LineNumberTag;
 import soot.tagkit.SourceLnPosTag;
@@ -109,16 +111,28 @@ protected void internalTransform(Body b, String phaseName, Map<String, String> o
  Timers.v().propagatorTimer.start();
  }
 
+ final boolean isShimple = b instanceof ShimpleBody;
+
  // Count number of definitions for each local.
  Map<Local, Integer> localToDefCount = new HashMap<Local, Integer>();
  for (Unit u : b.getUnits()) {
  if (u instanceof DefinitionStmt) {
- Value leftOp = ((DefinitionStmt) u).getLeftOp();
+ DefinitionStmt ds = (DefinitionStmt) u;
+ Value leftOp = ds.getLeftOp();
  if (leftOp instanceof Local) {
  Local loc = (Local) leftOp;
-
  Integer old = localToDefCount.get(loc);
- localToDefCount.put(loc, (old == null) ? 1 : (old + 1));
+ int newCount = (old == null) ? 1 : (old + 1);
+ if (isShimple) {
+ // Need to count all definitions in a PhiExpr
+ Value rightOp = ds.getRightOp();
+ if (rightOp instanceof PhiExpr) {
+ PhiExpr pe = (PhiExpr) rightOp;
+ // NOTE: "-1" accounts for the "+1" already added in 'newCount'
+ newCount += pe.getArgCount() - 1;
+ }
+ }
+ localToDefCount.put(loc, newCount);
  }
  }
  }
@@ -141,10 +155,10 @@ protected void internalTransform(Body b, String phaseName, Map<String, String> o
  CPOptions options = new CPOptions(opts);
  // Perform a local propagation pass.
  for (Unit u : (new PseudoTopologicalOrderer<Unit>()).newList(graph, false)) {
- for (ValueBox useBox : u.getUseBoxes()) {
+ USE_LOOP: for (ValueBox useBox : u.getUseBoxes()) {
  Value value = useBox.getValue();
  if (value instanceof Local) {
- Local l = (Local) value;
+ final Local l = (Local) value;
 
  // We force propagating nulls. If a target can only be
  // null due to typing, we always inline that constant.
@@ -160,10 +174,36 @@ protected void internalTransform(Body b, String phaseName, Map<String, String> o
  // We can propagate the definition if we either only have one definition
  // or all definitions are side-effect free and equal. For starters, we
  // only support constants in the case of multiple definitions.
- List<Unit> defsOfUse = localDefs.getDefsOfAt(l, u);
- boolean propagateDef = defsOfUse.size() == 1;
- if (!propagateDef && defsOfUse.size() > 0) {
- boolean agrees = true;
+ boolean propagateDef;
+ final List<Unit> defsOfUse = localDefs.getDefsOfAt(l, u);
+ if (defsOfUse.isEmpty()) {
+ propagateDef = false;
+ } else if (defsOfUse.size() == 1) {
+ propagateDef = true;
+ if (isShimple) {
+ // If a single definition was found but the RHS is a PhiExpr
+ // that contains more than one distinct value, then it counts
+ // as multiple definitions so it must not be propagated.
+ Value rhs = ((DefinitionStmt) defsOfUse.get(0)).getRightOp();
+ if (rhs instanceof PhiExpr) {
+ PhiExpr pe = (PhiExpr) rhs;
+ List<Value> vals = pe.getValues();
+ if (!vals.isEmpty()) {
+ Iterator<Value> itr = vals.iterator();
+ final Value firstVal = itr.next();
+ while (itr.hasNext()) {
+ Value next = itr.next();
+ if (!firstVal.equivTo(next)) {
+ propagateDef = false;
+ break;
+ }
+ }
+ }
+ }
+ }
+ } else {
+ assert (defsOfUse.size() > 1);
+ propagateDef = true; // Can propagate if no disagreement is found
  Constant constVal = null;
  for (Unit defUnit : defsOfUse) {
  boolean defAgrees = false;
@@ -178,15 +218,16 @@ protected void internalTransform(Body b, String phaseName, Map<String, String> o
  }
  }
  }
- agrees &= defAgrees;
+ // Do not propagate if any disagreement is found
+ if (!defAgrees) {
+ propagateDef = false;
+ break;
+ }
  }
- propagateDef = agrees;
  }
-
  if (propagateDef) {
  final DefinitionStmt def = (DefinitionStmt) defsOfUse.get(0);
  final Value rightOp = def.getRightOp();
-
  if (rightOp instanceof Constant) {
  if (useBox.canContainValue(rightOp)) {
  useBox.setValue(rightOp);
@@ -214,52 +255,37 @@ protected void internalTransform(Body b, String phaseName, Map<String, String> o
  useBox.setValue(m);
  copyLineTags(useBox, def);
  fastCopyPropagationCount++;
- continue;
- }
-
- List<Unit> path = graph.getExtendedBasicBlockPathBetween(def, u);
- if (path == null) {
- // no path in the extended basic block
- continue;
- }
-
- {
- boolean isRedefined = false;
-
- Iterator<Unit> pathIt = path.iterator();
- // Skip first node
- pathIt.next();
- // Make sure that m is not redefined along path
- while (pathIt.hasNext()) {
- Stmt s = (Stmt) pathIt.next();
-
- if (u == s) {
- // Don't look at the last statement
- // since it is evaluated after the uses.
- break;
- }
- if (s instanceof DefinitionStmt) {
- if (((DefinitionStmt) s).getLeftOp() == m) {
- isRedefined = true;
+ } else {
+ List<Unit> path = graph.getExtendedBasicBlockPathBetween(def, u);
+ if (path != null) {
+ Iterator<Unit> pathIt = path.iterator();
+ // Skip first node
+ pathIt.next();
+ // Make sure that m is not redefined along path
+ while (pathIt.hasNext()) {
+ Stmt s = (Stmt) pathIt.next();
+
+ if (u == s) {
+ // Don't look at the last statement
+ // since it is evaluated after the uses.
  break;
  }
+ if (s instanceof DefinitionStmt) {
+ if (((DefinitionStmt) s).getLeftOp() == m) {
+ continue USE_LOOP;
+ }
+ }
  }
- }
-
- if (isRedefined) {
- continue;
+ useBox.setValue(m);
+ slowCopyPropagationCount++;
  }
  }
-
- useBox.setValue(m);
- slowCopyPropagationCount++;
  }
  }
  }
  }
  }
  }
-
  if (Options.v().verbose()) {
  logger.debug("[" + b.getMethod().getName() + "] Propagated: " + fastCopyPropagationCount + " fast copies "
  + slowCopyPropagationCount + " slow copies");

src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java

@@ -0,0 +1,154 @@
+package soot.jimple.toolkit.scalar;
+
+/*-
+ * #%L
+ * Soot - a J*va Optimization Framework
+ * %%
+ * Copyright (C) 2021 Timothy Hoffman
+ * %%
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation, either version 2.1 of the
+ * License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Lesser Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Lesser Public
+ * License along with this program. If not, see
+ * <http://www.gnu.org/licenses/lgpl-2.1.html>.
+ * #L%
+ */
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.powermock.core.classloader.annotations.PowerMockIgnore;
+
+import soot.Body;
+import soot.SootMethod;
+import soot.jimple.toolkits.scalar.CopyPropagator;
+import soot.jimple.toolkits.scalar.DeadAssignmentEliminator;
+import soot.options.Options;
+import soot.shimple.ShimpleBody;
+import soot.testing.framework.AbstractTestingFramework;
+
+@PowerMockIgnore({ "com.sun.org.apache.xerces.*", "javax.xml.*", "org.xml.*", "org.w3c.*" })
+public class CopyPropagatorTest extends AbstractTestingFramework {
+
+ private static final boolean DEBUG_PRINT = false;
+
+ private static final String TEST_TARGET_CLASS = "soot.jimple.toolkits.scalar.CopyPropagatorTestInput";
+ private static final String RUN_TEST_METHOD_NAME = "runTest";
+ private static final String RUN_TEST_EXPECT_FIELD_NAME = "EXPECT";
+
+ @Override
+ protected void setupSoot() {
+ final Options opts = Options.v();
+
+ // Ensure a ShimpleBody is formed
+ opts.set_via_shimple(true);
+ opts.set_output_format(Options.output_format_shimple);
+
+ // Skip optimizations after eliminating Phi nodes
+ opts.setPhaseOption("shimple", "node-elim-opt:false");
+ }
+
+ private static void transform(Body b) {
+ Map<String, String> options = new HashMap<>();
+ options.put("enabled", "true");
+ options.put("only-stack-locals", "false");
+ options.put("only-regular-locals", "false");
+
+ CopyPropagator.v().transform(b, "test-cp-1", options);
+ if (DEBUG_PRINT) {
+ System.out.println("[transform](after test-cp-1) " + b);
+ }
+ CopyPropagator.v().transform(b, "test-cp-2", options);
+ if (DEBUG_PRINT) {
+ System.out.println("[transform](after test-cp-2) " + b);
+ }
+ DeadAssignmentEliminator.v().transform(b, "test-dae", options);
+ }
+
+ @Test
+ public void test_cp_nonSSA() throws Exception {
+ SootMethod target =
+ prepareTarget(methodSigFromComponents(TEST_TARGET_CLASS, "void", "implCompressSimpl", "int[]"), TEST_TARGET_CLASS);
+
+ ShimpleBody body = (ShimpleBody) target.retrieveActiveBody();
+ if (DEBUG_PRINT) {
+ System.out.println("[test_cp_nonSSA](Initial) " + body);
+ }
+
+ // Eliminate PhiExpr and then rebuild SSA form to introduce
+ // the code that gives CopyPropagator a hard time.
+ body.eliminateNodes();
+ body.rebuild();
+ if (DEBUG_PRINT) {
+ System.out.println("[test_cp_nonSSA](elim+rebuild) " + body);
+ }
+
+ // In this case, again eliminate all PhiExpr to show that the
+ // CopyPropagator does not have the problem when there are none.
+ body.eliminateNodes();
+ if (DEBUG_PRINT) {
+ System.out.println("[test_cp_nonSSA](elim-2) " + body);
+ }
+
+ // Now, run CopyPropagator when there are no PhiExpr present.
+ transform(body);
+ if (DEBUG_PRINT) {
+ System.out.println("[test_cp_nonSSA](cp+dae) " + body);
+ }
+
+ // Convert the body to a runnable classfile, run, and test output.
+ Class<?> c = generateClass(target.getDeclaringClass());
+ int[] expect = (int[]) c.getField(RUN_TEST_EXPECT_FIELD_NAME).get(null);
+ int[] actual = (int[]) c.getMethod(RUN_TEST_METHOD_NAME).invoke(null);
+ if (DEBUG_PRINT) {
+ System.out.println("expect = " + Arrays.toString(expect));
+ System.out.println("actual = " + Arrays.toString(actual));
+ }
+ Assert.assertArrayEquals("failure in test_cp_nonSSA", expect, actual);
+ }
+
+ @Test
+ public void test_cp_withSSA() throws Exception {
+ SootMethod target =
+ prepareTarget(methodSigFromComponents(TEST_TARGET_CLASS, "void", "implCompressSimpl", "int[]"), TEST_TARGET_CLASS);
+
+ ShimpleBody body = (ShimpleBody) target.retrieveActiveBody();
+ if (DEBUG_PRINT) {
+ System.out.println("[test_cp_withSSA](Initial) " + body);
+ }
+
+ // Eliminate PhiExpr and then rebuild SSA form to introduce
+ // the code that gives CopyPropagator a hard time.
+ body.eliminateNodes();
+ body.rebuild();
+ if (DEBUG_PRINT) {
+ System.out.println("[test_cp_withSSA](elim+rebuild) " + body);
+ }
+
+ // Run CopyPropagator when there are PhiExpr present.
+ transform(body);
+ if (DEBUG_PRINT) {
+ System.out.println("[test_cp_withSSA](cp+dae) " + body);
+ }
+
+ // Convert the body to a runnable classfile, run, and test output.
+ Class<?> c = generateClass(target.getDeclaringClass());
+ int[] expect = (int[]) c.getField(RUN_TEST_EXPECT_FIELD_NAME).get(null);
+ int[] actual = (int[]) c.getMethod(RUN_TEST_METHOD_NAME).invoke(null);
+ if (DEBUG_PRINT) {
+ System.out.println("expect = " + Arrays.toString(expect));
+ System.out.println("actual = " + Arrays.toString(actual));
+ }
+ Assert.assertArrayEquals("failure in test_cp_withSSA", expect, actual);
+ }
+}

src/systemTest/targets/soot/jimple/toolkit/scalar/CopyPropagatorTestInput.java

@@ -0,0 +1,54 @@
+package soot.jimple.toolkits.scalar;
+
+/*-
+ * #%L
+ * Soot - a J*va Optimization Framework
+ * %%
+ * Copyright (C) 2021 Timothy Hoffman
+ * %%
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation, either version 2.1 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Lesser Public License for more details.
+ *
+ * You should have received a copy of the GNU General Lesser Public
+ * License along with this program. If not, see
+ * <http://www.gnu.org/licenses/lgpl-2.1.html>.
+ * #L%
+ */
+
+/**
+ * @author Timothy Hoffman
+ */
+public class CopyPropagatorTestInput {
+
+ public static final int[] EXPECT = { 0, 16585, 144402 };
+
+ public static int[] runTest() {
+ final int[] state = { -271733879, -1732584194, 271733878 };
+ new CopyPropagatorTestInput().implCompressSimpl(state);
+ return state;
+ }
+
+ public void implCompressSimpl(int[] state) {
+ int b = state[0];
+ int c = state[1];
+ int d = state[2];
+
+ for (int i = 0; i < 20; i++) {
+ int temp = (b & c) | (~b & d);
+ d = c;
+ c = (b >>> 2);
+ b = temp;
+ }
+
+ state[0] = b;
+ state[1] = c;
+ state[2] = d;
+ }
+}