From b60422b75157140e974ded993d04bfb0956f00aa Mon Sep 17 00:00:00 2001 From: Timothy Hoffman <4001421+tim-hoffman@users.noreply.github.com> Date: Tue, 15 Jun 2021 16:09:15 -0500 Subject: [PATCH 1/3] Fix bug where CopyPropagator did not correctly handle PhiExpr in ShimpleBody --- .../toolkits/scalar/CopyPropagator.java | 122 ++++++----- .../toolkit/scalar/CopyPropagatorTest.java | 197 ++++++++++++++++++ .../scalar/CopyPropagatorTestInput.java | 54 +++++ 3 files changed, 325 insertions(+), 48 deletions(-) create mode 100644 src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java create mode 100644 src/systemTest/targets/soot/jimple/toolkit/scalar/CopyPropagatorTestInput.java diff --git a/src/main/java/soot/jimple/toolkits/scalar/CopyPropagator.java b/src/main/java/soot/jimple/toolkits/scalar/CopyPropagator.java index 2b067a2494c..62e8758ae46 100644 --- a/src/main/java/soot/jimple/toolkits/scalar/CopyPropagator.java +++ b/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 o Timers.v().propagatorTimer.start(); } + final boolean isShimple = b instanceof ShimpleBody; + // Count number of definitions for each local. Map localToDefCount = new HashMap(); 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 o CPOptions options = new CPOptions(opts); // Perform a local propagation pass. for (Unit u : (new PseudoTopologicalOrderer()).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 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 defsOfUse = localDefs.getDefsOfAt(l, u); - boolean propagateDef = defsOfUse.size() == 1; - if (!propagateDef && defsOfUse.size() > 0) { - boolean agrees = true; + boolean propagateDef; + final List 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 vals = pe.getValues(); + if (!vals.isEmpty()) { + Iterator 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 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 o useBox.setValue(m); copyLineTags(useBox, def); fastCopyPropagationCount++; - continue; - } - - List path = graph.getExtendedBasicBlockPathBetween(def, u); - if (path == null) { - // no path in the extended basic block - continue; - } - - { - boolean isRedefined = false; - - Iterator 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 path = graph.getExtendedBasicBlockPathBetween(def, u); + if (path != null) { + Iterator 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"); diff --git a/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java b/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java new file mode 100644 index 00000000000..2e3e3720564 --- /dev/null +++ b/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java @@ -0,0 +1,197 @@ +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 + * . + * #L% + */ +import java.io.ByteArrayOutputStream; +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.PackManager; +import soot.SootClass; +import soot.SootMethod; +import soot.baf.Baf; +import soot.baf.BafASMBackend; +import soot.baf.BafBody; +import soot.jimple.JimpleBody; +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"); + + // TODO: maybe don't need to keep these (I'll see) + opts.setPhaseOption("jb", "use-original-names:true"); + opts.setPhaseOption("jb.sils", "enabled:false"); + } + + private static void transform(Body b) { + Map 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); + } + + private static Class generateClass(SootClass sc) { + // First, convert all method Body instances to BafBody + for (SootMethod m : sc.getMethods()) { + convertBodyToBaf(m); + } + // Then use ASM to generate a byte[] for the classfile + ByteArrayOutputStream os = new ByteArrayOutputStream(); + new BafASMBackend(sc, Options.v().java_version()).generateClassFile(os); + final byte[] classBytes = os.toByteArray(); + + // NOTE: "new ClassLoader" ensures every class loaded is distinct. + return new ClassLoader() { + @Override + public Class findClass(String name) { + return defineClass(name, classBytes, 0, classBytes.length); + } + }.findClass(TEST_TARGET_CLASS); + } + + private static void convertBodyToBaf(SootMethod m) { + Body b = m.retrieveActiveBody(); + if (b instanceof ShimpleBody) { + b = ((ShimpleBody) b).toJimpleBody(); + } + Assert.assertTrue(b instanceof JimpleBody); + // Convert body to Jimple, then to Baf + BafBody bafBody = Baf.v().newBody((JimpleBody) b); + PackManager.v().getPack("bop").apply(bafBody); + PackManager.v().getPack("tag").apply(bafBody); + m.setActiveBody(bafBody); + } + + @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); + } +} diff --git a/src/systemTest/targets/soot/jimple/toolkit/scalar/CopyPropagatorTestInput.java b/src/systemTest/targets/soot/jimple/toolkit/scalar/CopyPropagatorTestInput.java new file mode 100644 index 00000000000..5c9faf62a5f --- /dev/null +++ b/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 + * . + * #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; + } +} From a57223b314d50043a01c62071e0f319ef0c1505e Mon Sep 17 00:00:00 2001 From: Timothy Hoffman <4001421+tim-hoffman@users.noreply.github.com> Date: Fri, 18 Jun 2021 12:04:16 -0500 Subject: [PATCH 2/3] remove unnecessary lines from CopyProp test --- .../java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java b/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java index 2e3e3720564..60a98548b35 100644 --- a/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java +++ b/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java @@ -63,10 +63,6 @@ protected void setupSoot() { // Skip optimizations after eliminating Phi nodes opts.setPhaseOption("shimple", "node-elim-opt:false"); - - // TODO: maybe don't need to keep these (I'll see) - opts.setPhaseOption("jb", "use-original-names:true"); - opts.setPhaseOption("jb.sils", "enabled:false"); } private static void transform(Body b) { From f842d4d2ee2ecf46946823d884b788859b58b15a Mon Sep 17 00:00:00 2001 From: Timothy Hoffman <4001421+tim-hoffman@users.noreply.github.com> Date: Fri, 18 Jun 2021 15:00:05 -0500 Subject: [PATCH 3/3] remove redundant methods from copy propagator test --- .../toolkit/scalar/CopyPropagatorTest.java | 39 ------------------- 1 file changed, 39 deletions(-) diff --git a/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java b/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java index 60a98548b35..3ff770de771 100644 --- a/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java +++ b/src/systemTest/java/soot/jimple/toolkit/scalar/CopyPropagatorTest.java @@ -21,7 +21,6 @@ * . * #L% */ -import java.io.ByteArrayOutputStream; import java.util.Arrays; import java.util.HashMap; import java.util.Map; @@ -31,13 +30,7 @@ import org.powermock.core.classloader.annotations.PowerMockIgnore; import soot.Body; -import soot.PackManager; -import soot.SootClass; import soot.SootMethod; -import soot.baf.Baf; -import soot.baf.BafASMBackend; -import soot.baf.BafBody; -import soot.jimple.JimpleBody; import soot.jimple.toolkits.scalar.CopyPropagator; import soot.jimple.toolkits.scalar.DeadAssignmentEliminator; import soot.options.Options; @@ -82,38 +75,6 @@ private static void transform(Body b) { DeadAssignmentEliminator.v().transform(b, "test-dae", options); } - private static Class generateClass(SootClass sc) { - // First, convert all method Body instances to BafBody - for (SootMethod m : sc.getMethods()) { - convertBodyToBaf(m); - } - // Then use ASM to generate a byte[] for the classfile - ByteArrayOutputStream os = new ByteArrayOutputStream(); - new BafASMBackend(sc, Options.v().java_version()).generateClassFile(os); - final byte[] classBytes = os.toByteArray(); - - // NOTE: "new ClassLoader" ensures every class loaded is distinct. - return new ClassLoader() { - @Override - public Class findClass(String name) { - return defineClass(name, classBytes, 0, classBytes.length); - } - }.findClass(TEST_TARGET_CLASS); - } - - private static void convertBodyToBaf(SootMethod m) { - Body b = m.retrieveActiveBody(); - if (b instanceof ShimpleBody) { - b = ((ShimpleBody) b).toJimpleBody(); - } - Assert.assertTrue(b instanceof JimpleBody); - // Convert body to Jimple, then to Baf - BafBody bafBody = Baf.v().newBody((JimpleBody) b); - PackManager.v().getPack("bop").apply(bafBody); - PackManager.v().getPack("tag").apply(bafBody); - m.setActiveBody(bafBody); - } - @Test public void test_cp_nonSSA() throws Exception { SootMethod target =