From 6c61ce52a3010ab09c78087f571f69cc96ced2ec Mon Sep 17 00:00:00 2001 From: Skylot Date: Thu, 23 May 2019 22:42:25 +0300 Subject: [PATCH] fix: handle cases with SSA variable used in several PHI's (#667) --- .../jadx/core/dex/instructions/PhiInsn.java | 5 +- .../core/dex/instructions/args/SSAVar.java | 53 ++++++++++++++++--- .../core/dex/visitors/InitCodeVariables.java | 31 ++++++----- .../debuginfo/DebugInfoApplyVisitor.java | 3 +- .../visitors/regions/LoopRegionVisitor.java | 9 ++-- .../core/dex/visitors/regions/TernaryMod.java | 8 +-- .../core/dex/visitors/ssa/SSATransform.java | 8 +-- .../typeinference/TypeInferenceVisitor.java | 15 ++++-- .../main/java/jadx/core/utils/DebugUtils.java | 3 +- .../java/jadx/core/utils/InsnRemover.java | 16 +----- .../variables/TestVariablesDefinitions2.java | 42 +++++++++++++++ 11 files changed, 132 insertions(+), 61 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesDefinitions2.java diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/PhiInsn.java b/jadx-core/src/main/java/jadx/core/dex/instructions/PhiInsn.java index e66398e4d..36a7f8364 100644 --- a/jadx-core/src/main/java/jadx/core/dex/instructions/PhiInsn.java +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/PhiInsn.java @@ -12,7 +12,6 @@ import jadx.core.dex.instructions.args.InsnArg; import jadx.core.dex.instructions.args.RegisterArg; import jadx.core.dex.nodes.BlockNode; import jadx.core.dex.nodes.InsnNode; -import jadx.core.utils.InsnRemover; import jadx.core.utils.Utils; import jadx.core.utils.exceptions.JadxRuntimeException; @@ -76,7 +75,7 @@ public final class PhiInsn extends InsnNode { protected RegisterArg removeArg(int index) { RegisterArg reg = (RegisterArg) super.removeArg(index); blockBinds.remove(index); - InsnRemover.fixUsedInPhiFlag(reg); + reg.getSVar().updateUsedInPhiList(); return reg; } @@ -98,7 +97,7 @@ public final class PhiInsn extends InsnNode { RegisterArg reg = (RegisterArg) to; bindArg(reg, pred); - reg.getSVar().setUsedInPhi(this); + reg.getSVar().addUsedInPhi(this); return true; } diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/args/SSAVar.java b/jadx-core/src/main/java/jadx/core/dex/instructions/args/SSAVar.java index e8612e3d5..b2ee73dc6 100644 --- a/jadx-core/src/main/java/jadx/core/dex/instructions/args/SSAVar.java +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/args/SSAVar.java @@ -12,7 +12,9 @@ import org.jetbrains.annotations.Nullable; import jadx.core.dex.attributes.AType; import jadx.core.dex.attributes.AttrNode; import jadx.core.dex.attributes.nodes.RegDebugInfoAttr; +import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.PhiInsn; +import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; import jadx.core.dex.visitors.typeinference.TypeInfo; import jadx.core.utils.StringUtils; @@ -24,8 +26,7 @@ public class SSAVar extends AttrNode { private RegisterArg assign; private final List useList = new ArrayList<>(2); - @Nullable - private PhiInsn usedInPhi; + private List usedInPhi = null; private TypeInfo typeInfo = new TypeInfo(); @@ -85,24 +86,60 @@ public class SSAVar extends AttrNode { useList.removeIf(registerArg -> registerArg == arg); } - public void setUsedInPhi(@Nullable PhiInsn usedInPhi) { - this.usedInPhi = usedInPhi; + public void addUsedInPhi(PhiInsn phiInsn) { + if (usedInPhi == null) { + usedInPhi = new ArrayList<>(1); + } + usedInPhi.add(phiInsn); + } + + public void removeUsedInPhi(PhiInsn phiInsn) { + if (usedInPhi != null) { + usedInPhi.removeIf(insn -> insn == phiInsn); + if (usedInPhi.isEmpty()) { + usedInPhi = null; + } + } + } + + public void updateUsedInPhiList() { + this.usedInPhi = null; + for (RegisterArg reg : useList) { + InsnNode parentInsn = reg.getParentInsn(); + if (parentInsn != null && parentInsn.getType() == InsnType.PHI) { + addUsedInPhi((PhiInsn) parentInsn); + } + } } @Nullable - public PhiInsn getUsedInPhi() { + public PhiInsn getOnlyOneUseInPhi() { + if (usedInPhi != null && usedInPhi.size() == 1) { + return usedInPhi.get(0); + } + return null; + } + + public List getUsedInPhi() { + if (usedInPhi == null) { + return Collections.emptyList(); + } return usedInPhi; } public boolean isUsedInPhi() { - return usedInPhi != null; + return usedInPhi != null && !usedInPhi.isEmpty(); } public int getVariableUseCount() { + int count = useList.size(); if (usedInPhi == null) { - return useList.size(); + return count; } - return useList.size() + usedInPhi.getResult().getSVar().getUseCount(); + for (PhiInsn phiInsn : usedInPhi) { + count += phiInsn.getResult().getSVar().getUseCount(); + } + return count; } public void setName(String name) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/InitCodeVariables.java b/jadx-core/src/main/java/jadx/core/dex/visitors/InitCodeVariables.java index 298a39fde..10a185e21 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/InitCodeVariables.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/InitCodeVariables.java @@ -58,11 +58,11 @@ public class InitCodeVariables extends AbstractVisitor { } private static void setCodeVar(SSAVar ssaVar, CodeVar codeVar) { - PhiInsn usedInPhi = ssaVar.getUsedInPhi(); - if (usedInPhi != null) { + List usedInPhiList = ssaVar.getUsedInPhi(); + if (!usedInPhiList.isEmpty()) { Set vars = new LinkedHashSet<>(); vars.add(ssaVar); - collectConnectedVars(usedInPhi, vars); + collectConnectedVars(usedInPhiList, vars); setCodeVarType(codeVar, vars); vars.forEach(var -> { if (var.isCodeVarSet()) { @@ -92,19 +92,18 @@ public class InitCodeVariables extends AbstractVisitor { } } - private static void collectConnectedVars(PhiInsn phiInsn, Set vars) { - if (phiInsn == null) { - return; - } - SSAVar resultVar = phiInsn.getResult().getSVar(); - if (vars.add(resultVar)) { - collectConnectedVars(resultVar.getUsedInPhi(), vars); - } - phiInsn.getArguments().forEach(arg -> { - SSAVar sVar = ((RegisterArg) arg).getSVar(); - if (vars.add(sVar)) { - collectConnectedVars(sVar.getUsedInPhi(), vars); + private static void collectConnectedVars(List phiInsnList, Set vars) { + for (PhiInsn phiInsn : phiInsnList) { + SSAVar resultVar = phiInsn.getResult().getSVar(); + if (vars.add(resultVar)) { + collectConnectedVars(resultVar.getUsedInPhi(), vars); } - }); + phiInsn.getArguments().forEach(arg -> { + SSAVar sVar = ((RegisterArg) arg).getSVar(); + if (vars.add(sVar)) { + collectConnectedVars(sVar.getUsedInPhi(), vars); + } + }); + } } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/debuginfo/DebugInfoApplyVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/debuginfo/DebugInfoApplyVisitor.java index 515e6d4f3..d1c7f101c 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/debuginfo/DebugInfoApplyVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/debuginfo/DebugInfoApplyVisitor.java @@ -203,8 +203,7 @@ public class DebugInfoApplyVisitor extends AbstractVisitor { private static void fixNamesForPhiInsns(MethodNode mth) { mth.getSVars().forEach(ssaVar -> { - PhiInsn phiInsn = ssaVar.getUsedInPhi(); - if (phiInsn != null) { + for (PhiInsn phiInsn : ssaVar.getUsedInPhi()) { Set names = new HashSet<>(1 + phiInsn.getArgsCount()); addArgName(phiInsn.getResult(), names); phiInsn.getArguments().forEach(arg -> addArgName(arg, names)); diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/LoopRegionVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/LoopRegionVisitor.java index fbe6f45b7..a4a6335b0 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/LoopRegionVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/LoopRegionVisitor.java @@ -89,9 +89,12 @@ public class LoopRegionVisitor extends AbstractVisitor implements IRegionVisitor || !incrArg.getSVar().isUsedInPhi()) { return false; } - PhiInsn phiInsn = incrArg.getSVar().getUsedInPhi(); - if (phiInsn == null - || phiInsn.getArgsCount() != 2 + List phiInsnList = incrArg.getSVar().getUsedInPhi(); + if (phiInsnList.size() != 1) { + return false; + } + PhiInsn phiInsn = phiInsnList.get(0); + if (phiInsn.getArgsCount() != 2 || !phiInsn.containsArg(incrArg) || incrArg.getSVar().getUseCount() != 1) { return false; diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java index 2fdc9b0f3..6755ade97 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java @@ -62,8 +62,8 @@ public class TernaryMod { RegisterArg thenResArg = thenInsn.getResult(); RegisterArg elseResArg = elseInsn.getResult(); if (thenResArg != null && elseResArg != null) { - PhiInsn thenPhi = thenResArg.getSVar().getUsedInPhi(); - PhiInsn elsePhi = elseResArg.getSVar().getUsedInPhi(); + PhiInsn thenPhi = thenResArg.getSVar().getOnlyOneUseInPhi(); + PhiInsn elsePhi = elseResArg.getSVar().getOnlyOneUseInPhi(); if (thenPhi == null || thenPhi != elsePhi) { return false; } @@ -165,8 +165,8 @@ public class TernaryMod { if (t.getResult() == null || e.getResult() == null) { return false; } - PhiInsn tPhi = t.getResult().getSVar().getUsedInPhi(); - PhiInsn ePhi = e.getResult().getSVar().getUsedInPhi(); + PhiInsn tPhi = t.getResult().getSVar().getOnlyOneUseInPhi(); + PhiInsn ePhi = e.getResult().getSVar().getOnlyOneUseInPhi(); if (ePhi == null || tPhi != ePhi) { return false; } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/ssa/SSATransform.java b/jadx-core/src/main/java/jadx/core/dex/visitors/ssa/SSATransform.java index b4ce39fd3..081b5848f 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/ssa/SSATransform.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/ssa/SSATransform.java @@ -187,7 +187,7 @@ public class SSATransform extends AbstractVisitor { } RegisterArg arg = phiInsn.bindArg(state.getBlock()); var.use(arg); - var.setUsedInPhi(phiInsn); + var.addUsedInPhi(phiInsn); } /** @@ -323,7 +323,7 @@ public class SSATransform extends AbstractVisitor { } SSAVar sVar = ((RegisterArg) arg).getSVar(); if (sVar != null) { - sVar.setUsedInPhi(null); + sVar.removeUsedInPhi(phiInsn); } } InsnRemover.remove(mth, block, phiInsn); @@ -347,13 +347,13 @@ public class SSATransform extends AbstractVisitor { SSAVar argVar = arg.getSVar(); if (argVar != null) { argVar.removeUse(arg); - argVar.setUsedInPhi(null); + argVar.removeUsedInPhi(phi); } // try inline if (inlinePhiInsn(mth, block, phi)) { insns.remove(phiIndex); } else { - assign.setUsedInPhi(null); + assign.removeUsedInPhi(phi); InsnNode m = new InsnNode(InsnType.MOVE, 1); m.add(AFlag.SYNTHETIC); diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeInferenceVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeInferenceVisitor.java index 868b86653..37f331298 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeInferenceVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/typeinference/TypeInferenceVisitor.java @@ -196,8 +196,7 @@ public final class TypeInferenceVisitor extends AbstractVisitor { } private void mergePhiBounds(SSAVar ssaVar) { - PhiInsn usedInPhi = ssaVar.getUsedInPhi(); - if (usedInPhi != null) { + for (PhiInsn usedInPhi : ssaVar.getUsedInPhi()) { Set bounds = ssaVar.getTypeInfo().getBounds(); bounds.addAll(usedInPhi.getResult().getSVar().getTypeInfo().getBounds()); for (InsnArg arg : usedInPhi.getArguments()) { @@ -307,8 +306,8 @@ public final class TypeInferenceVisitor extends AbstractVisitor { if (var.getTypeInfo().getType().isTypeKnown()) { return false; } - PhiInsn phiInsn = var.getUsedInPhi(); - if (phiInsn == null) { + List usedInPhiList = var.getUsedInPhi(); + if (usedInPhiList.isEmpty()) { return false; } if (var.getUseCount() == 1) { @@ -317,7 +316,15 @@ public final class TypeInferenceVisitor extends AbstractVisitor { return false; } } + for (PhiInsn phiInsn : usedInPhiList) { + if (!insertMoveForPhi(mth, phiInsn, var)) { + return false; + } + } + return true; + } + private boolean insertMoveForPhi(MethodNode mth, PhiInsn phiInsn, SSAVar var) { int argsCount = phiInsn.getArgsCount(); for (int argIndex = 0; argIndex < argsCount; argIndex++) { RegisterArg reg = phiInsn.getArg(argIndex); diff --git a/jadx-core/src/main/java/jadx/core/utils/DebugUtils.java b/jadx-core/src/main/java/jadx/core/utils/DebugUtils.java index b775fc6bf..c94c190b5 100644 --- a/jadx-core/src/main/java/jadx/core/utils/DebugUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/DebugUtils.java @@ -193,8 +193,7 @@ public class DebugUtils { } } for (SSAVar ssaVar : mth.getSVars()) { - PhiInsn usedInPhi = ssaVar.getUsedInPhi(); - if (usedInPhi != null) { + for (PhiInsn usedInPhi : ssaVar.getUsedInPhi()) { boolean found = false; for (RegisterArg useArg : ssaVar.getUseList()) { InsnNode parentInsn = useArg.getParentInsn(); diff --git a/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java b/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java index 9bb7a8fdd..0b2c661ef 100644 --- a/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java +++ b/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java @@ -6,7 +6,6 @@ import java.util.List; import jadx.core.dex.attributes.AFlag; import jadx.core.dex.instructions.InsnType; -import jadx.core.dex.instructions.PhiInsn; import jadx.core.dex.instructions.args.InsnArg; import jadx.core.dex.instructions.args.InsnWrapArg; import jadx.core.dex.instructions.args.RegisterArg; @@ -65,7 +64,7 @@ public class InsnRemover { if (insn.getType() == InsnType.PHI) { for (InsnArg arg : insn.getArguments()) { if (arg instanceof RegisterArg) { - fixUsedInPhiFlag((RegisterArg) arg); + ((RegisterArg) arg).getSVar().updateUsedInPhiList(); } } } @@ -73,19 +72,6 @@ public class InsnRemover { insn.add(AFlag.REMOVE); } - public static void fixUsedInPhiFlag(RegisterArg useReg) { - PhiInsn usedIn = null; - for (RegisterArg reg : useReg.getSVar().getUseList()) { - InsnNode parentInsn = reg.getParentInsn(); - if (parentInsn != null - && parentInsn.getType() == InsnType.PHI - && parentInsn.containsArg(useReg)) { - usedIn = (PhiInsn) parentInsn; - } - } - useReg.getSVar().setUsedInPhi(usedIn); - } - public static void unbindResult(MethodNode mth, InsnNode insn) { RegisterArg r = insn.getResult(); if (r != null && r.getSVar() != null && mth != null) { diff --git a/jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesDefinitions2.java b/jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesDefinitions2.java new file mode 100644 index 000000000..edee0ceb7 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/variables/TestVariablesDefinitions2.java @@ -0,0 +1,42 @@ +package jadx.tests.integration.variables; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.IntegrationTest; + +import static jadx.tests.api.utils.JadxMatchers.containsOne; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; + +public class TestVariablesDefinitions2 extends IntegrationTest { + + public static class TestCls { + + public static int test(List list) { + int i = 0; + if (list != null) { + for (String str : list) { + if (str.isEmpty()) { + i++; + } + } + } + return i; + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + + assertThat(code, containsOne("int i = 0;")); + assertThat(code, containsOne("i++;")); + assertThat(code, containsOne("return i;")); + assertThat(code, not(containsString("i2;"))); + } +}