From aad2d24c589c49749410315d859412f6a8c62b16 Mon Sep 17 00:00:00 2001 From: Skylot Date: Tue, 16 Jul 2019 19:44:48 +0300 Subject: [PATCH] fix: unbind unused ssa variable after ternary conversion (#708) --- .../core/dex/visitors/regions/TernaryMod.java | 3 + .../regions/variables/ProcessVariables.java | 5 +- .../visitors/shrink/CodeShrinkVisitor.java | 8 +- .../java/jadx/core/utils/InsnRemover.java | 35 ++++- .../conditions/TestConditions14.java | 4 +- .../integration/conditions/TestTernary4.java | 45 ++++++ .../integration/types/TestTypeResolver3.java | 10 -- .../test/smali/conditions/TestTernary4.smali | 144 ++++++++++++++++++ 8 files changed, 233 insertions(+), 21 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/conditions/TestTernary4.java create mode 100644 jadx-core/src/test/smali/conditions/TestTernary4.smali 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 8a830f83..6bc9e2a2 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 @@ -99,6 +99,7 @@ public class TernaryMod implements IRegionIterativeVisitor { RegisterArg resArg; if (thenPhi.getArgsCount() == 2) { resArg = thenPhi.getResult(); + InsnRemover.unbindResult(mth, thenInsn); } else { resArg = thenResArg; thenPhi.removeArg(elseResArg); @@ -107,6 +108,8 @@ public class TernaryMod implements IRegionIterativeVisitor { resArg, InsnArg.wrapArg(thenInsn), InsnArg.wrapArg(elseInsn)); ternInsn.setSourceLine(thenInsn.getSourceLine()); + InsnRemover.unbindResult(mth, elseInsn); + // remove 'if' instruction header.getInstructions().clear(); header.getInstructions().add(ternInsn); diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/variables/ProcessVariables.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/variables/ProcessVariables.java index 89424e09..a6deb182 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/variables/ProcessVariables.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/variables/ProcessVariables.java @@ -14,6 +14,7 @@ import org.slf4j.LoggerFactory; import jadx.core.dex.attributes.AFlag; import jadx.core.dex.attributes.AType; import jadx.core.dex.attributes.nodes.DeclareVariablesAttr; +import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.args.ArgType; import jadx.core.dex.instructions.args.CodeVar; import jadx.core.dex.instructions.args.RegisterArg; @@ -228,7 +229,9 @@ public class ProcessVariables extends AbstractVisitor { private static boolean checkDeclareAtAssign(SSAVar var) { RegisterArg arg = var.getAssign(); InsnNode parentInsn = arg.getParentInsn(); - if (parentInsn == null) { + if (parentInsn == null + || parentInsn.contains(AFlag.WRAPPED) + || parentInsn.getType() == InsnType.PHI) { return false; } if (!arg.equals(parentInsn.getResult())) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/shrink/CodeShrinkVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/shrink/CodeShrinkVisitor.java index c70c02f5..14738780 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/shrink/CodeShrinkVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/shrink/CodeShrinkVisitor.java @@ -20,6 +20,7 @@ import jadx.core.dex.visitors.JadxVisitor; import jadx.core.dex.visitors.ModVisitor; import jadx.core.utils.BlockUtils; import jadx.core.utils.InsnList; +import jadx.core.utils.InsnRemover; import jadx.core.utils.exceptions.JadxRuntimeException; @JadxVisitor( @@ -67,7 +68,7 @@ public class CodeShrinkVisitor extends AbstractVisitor { } if (!wrapList.isEmpty()) { for (WrapInfo wrapInfo : wrapList) { - inline(wrapInfo.getArg(), wrapInfo.getInsn(), block); + inline(mth, wrapInfo.getArg(), wrapInfo.getInsn(), block); } } } @@ -106,12 +107,12 @@ public class CodeShrinkVisitor extends AbstractVisitor { if (assignBlock != null && assignInsn != arg.getParentInsn() && canMoveBetweenBlocks(assignInsn, assignBlock, block, argsInfo.getInsn())) { - inline(arg, assignInsn, assignBlock); + inline(mth, arg, assignInsn, assignBlock); } } } - private static boolean inline(RegisterArg arg, InsnNode insn, BlockNode block) { + private static boolean inline(MethodNode mth, RegisterArg arg, InsnNode insn, BlockNode block) { InsnNode parentInsn = arg.getParentInsn(); if (parentInsn != null && parentInsn.getType() == InsnType.RETURN) { parentInsn.setSourceLine(insn.getSourceLine()); @@ -119,6 +120,7 @@ public class CodeShrinkVisitor extends AbstractVisitor { boolean replaced = arg.wrapInstruction(insn) != null; if (replaced) { InsnList.remove(block, insn); + InsnRemover.unbindResult(mth, insn); } return replaced; } 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 c66b637c..89af7f25 100644 --- a/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java +++ b/jadx-core/src/main/java/jadx/core/utils/InsnRemover.java @@ -8,6 +8,7 @@ import org.jetbrains.annotations.Nullable; 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; @@ -66,7 +67,7 @@ public class InsnRemover { toRemove.clear(); } - public static void unbindInsn(MethodNode mth, InsnNode insn) { + public static void unbindInsn(@Nullable MethodNode mth, InsnNode insn) { for (InsnArg arg : insn.getArguments()) { unbindArgUsage(mth, arg); } @@ -81,17 +82,41 @@ public class InsnRemover { insn.add(AFlag.REMOVE); } - public static void unbindResult(MethodNode mth, InsnNode insn) { + public static void unbindResult(@Nullable MethodNode mth, InsnNode insn) { RegisterArg r = insn.getResult(); if (r != null && r.getSVar() != null && mth != null) { SSAVar ssaVar = r.getSVar(); - if (ssaVar.getUseCount() == 0) { - mth.removeSVar(ssaVar); + removeSsaVar(mth, ssaVar); + } + } + + private static void removeSsaVar(MethodNode mth, SSAVar ssaVar) { + int useCount = ssaVar.getUseCount(); + if (useCount == 0) { + mth.removeSVar(ssaVar); + return; + } + // check if all usage only in PHI insns + boolean allPhis = true; + for (RegisterArg arg : ssaVar.getUseList()) { + InsnNode parentInsn = arg.getParentInsn(); + if (parentInsn == null || parentInsn.getType() != InsnType.PHI) { + allPhis = false; + break; + } + } + if (allPhis) { + for (RegisterArg arg : new ArrayList<>(ssaVar.getUseList())) { + InsnNode parentInsn = arg.getParentInsn(); + if (parentInsn != null) { + ((PhiInsn) parentInsn).removeArg(arg); + } } + mth.removeSVar(ssaVar); } } - public static void unbindArgUsage(MethodNode mth, InsnArg arg) { + public static void unbindArgUsage(@Nullable MethodNode mth, InsnArg arg) { if (arg instanceof RegisterArg) { RegisterArg reg = (RegisterArg) arg; SSAVar sVar = reg.getSVar(); diff --git a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions14.java b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions14.java index df524fd1..bbe07f11 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions14.java +++ b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions14.java @@ -17,7 +17,7 @@ public class TestConditions14 extends IntegrationTest { if (r) { return false; } - System.out.println("1"); + System.out.println("r=" + r); return true; } } @@ -29,6 +29,6 @@ public class TestConditions14 extends IntegrationTest { assertThat(code, containsOne("boolean r = a == null ? b != null : !a.equals(b);")); assertThat(code, containsOne("if (r) {")); - assertThat(code, containsOne("System.out.println(\"1\");")); + assertThat(code, containsOne("System.out.println(\"r=\" + r);")); } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestTernary4.java b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestTernary4.java new file mode 100644 index 00000000..55f949b7 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestTernary4.java @@ -0,0 +1,45 @@ +package jadx.tests.integration.conditions; + +import org.junit.jupiter.api.Test; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.SmaliTest; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; + +public class TestTernary4 extends SmaliTest { + + // @formatter:off + /* + private Set test(HashMap hashMap) { + boolean z; + HashSet hashSet = new HashSet(); + synchronized (this.defaultValuesByPath) { + for (String next : this.defaultValuesByPath.keySet()) { + Object obj = hashMap.get(next); + if (obj != null) { + z = !getValueObject(next).equals(obj); + } else { + z = this.valuesByPath.get(next) != null;; + } + if (z) { + hashSet.add(next); + } + } + } + return hashSet; + } + */ + // @formatter:on + + @Test + public void test() { + ClassNode cls = getClassNodeFromSmali(); + String code = cls.getCode().toString(); + + assertThat(code, not(containsString("r5"))); + assertThat(code, not(containsString("try"))); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/integration/types/TestTypeResolver3.java b/jadx-core/src/test/java/jadx/tests/integration/types/TestTypeResolver3.java index 7a6773f9..74ac3bf8 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/types/TestTypeResolver3.java +++ b/jadx-core/src/test/java/jadx/tests/integration/types/TestTypeResolver3.java @@ -2,7 +2,6 @@ package jadx.tests.integration.types; import org.junit.jupiter.api.Test; -import jadx.NotYetImplemented; import jadx.core.dex.nodes.ClassNode; import jadx.tests.api.IntegrationTest; @@ -27,15 +26,6 @@ public class TestTypeResolver3 extends IntegrationTest { ClassNode cls = getClassNode(TestCls.class); String code = cls.getCode().toString(); - assertThat(code, containsOne("s1.length() == s2.length() ? 0 : s1.length() < s2.length() ? -1 : 1;")); - } - - @Test - @NotYetImplemented - public void test3() { - ClassNode cls = getClassNode(TestCls.class); - String code = cls.getCode().toString(); - assertThat(code, containsOne("return s1.length() == s2.length() ? 0 : s1.length() < s2.length() ? -1 : 1;")); } diff --git a/jadx-core/src/test/smali/conditions/TestTernary4.smali b/jadx-core/src/test/smali/conditions/TestTernary4.smali new file mode 100644 index 00000000..f0f339cc --- /dev/null +++ b/jadx-core/src/test/smali/conditions/TestTernary4.smali @@ -0,0 +1,144 @@ +.class public final Lconditions/TestTernary4; +.super Ljava/lang/Object; + +.field private defaultValuesByPath:Ljava/util/Map; + .annotation system Ldalvik/annotation/Signature; + value = { + "Ljava/util/Map<", + "Ljava/lang/String;", + "Ljava/lang/Object;", + ">;" + } + .end annotation +.end field + +.field private valuesByPath:Ljava/util/Map; + .annotation system Ldalvik/annotation/Signature; + value = { + "Ljava/util/Map<", + "Ljava/lang/String;", + "Ljava/lang/Object;", + ">;" + } + .end annotation +.end field + +.method private test(Ljava/util/HashMap;)Ljava/util/Set; + .registers 10 + .annotation system Ldalvik/annotation/Signature; + value = { + "(", + "Ljava/util/HashMap<", + "Ljava/lang/String;", + "Ljava/lang/Object;", + ">;)", + "Ljava/util/Set;" + } + .end annotation + + .line 278 + new-instance v0, Ljava/util/HashSet; + + invoke-direct {v0}, Ljava/util/HashSet;->()V + + .line 280 + iget-object v1, p0, Lconditions/TestTernary4;->defaultValuesByPath:Ljava/util/Map; + + monitor-enter v1 + + .line 281 + :try_start_14 + iget-object v3, p0, Lconditions/TestTernary4;->defaultValuesByPath:Ljava/util/Map; + + invoke-interface {v3}, Ljava/util/Map;->keySet()Ljava/util/Set; + + move-result-object v3 + + invoke-interface {v3}, Ljava/util/Set;->iterator()Ljava/util/Iterator; + + move-result-object v3 + + :cond_1e + :goto_1e + invoke-interface {v3}, Ljava/util/Iterator;->hasNext()Z + + move-result v4 + + if-eqz v4, :cond_4c + + invoke-interface {v3}, Ljava/util/Iterator;->next()Ljava/lang/Object; + + move-result-object v4 + + check-cast v4, Ljava/lang/String; + + .line 286 + invoke-virtual {p1, v4}, Ljava/util/HashMap;->get(Ljava/lang/Object;)Ljava/lang/Object; + + move-result-object v5 + + const/4 v6, 0x1 + + if-eqz v5, :cond_3b + + .line 287 + invoke-direct {p0, v4}, Lconditions/TestTernary4;->getValueObject(Ljava/lang/String;)Ljava/lang/Object; + + move-result-object v7 + + invoke-virtual {v7, v5}, Ljava/lang/Object;->equals(Ljava/lang/Object;)Z + + move-result v5 + + xor-int/2addr v5, v6 + + goto :goto_46 + + .line 289 + :cond_3b + iget-object v5, p0, Lconditions/TestTernary4;->valuesByPath:Ljava/util/Map; + + invoke-interface {v5, v4}, Ljava/util/Map;->get(Ljava/lang/Object;)Ljava/lang/Object; + + move-result-object v5 + + if-eqz v5, :cond_45 + + const/4 v5, 0x1 + + goto :goto_46 + + :cond_45 + const/4 v5, 0x0 + + :goto_46 + if-eqz v5, :cond_1e + + .line 293 + invoke-interface {v0, v4}, Ljava/util/Set;->add(Ljava/lang/Object;)Z + + goto :goto_1e + + .line 296 + :cond_4c + monitor-exit v1 + + return-object v0 + + :catchall_4e + move-exception p1 + + monitor-exit v1 + :try_end_50 + .catchall {:try_start_14 .. :try_end_50} :catchall_4e + + throw p1 + + return-void +.end method + +.method private getValueObject(Ljava/lang/String;)Ljava/lang/Object; + .locals 1 + const/4 v0, 0x0 + return-object v0 +.end method -- GitLab