From 8a464e827486bf9b96821c323c016ae62651ec56 Mon Sep 17 00:00:00 2001 From: Skylot Date: Sun, 23 Mar 2014 23:00:25 +0400 Subject: [PATCH] core: fix condition processing errors --- jadx-core/src/main/java/jadx/core/Jadx.java | 4 +- .../dex/instructions/mods/TernaryInsn.java | 15 ++- .../java/jadx/core/dex/nodes/BlockNode.java | 17 ++- .../java/jadx/core/dex/nodes/IRegion.java | 1 + .../jadx/core/dex/regions/AbstractRegion.java | 7 + .../java/jadx/core/dex/regions/Region.java | 18 ++- .../core/dex/visitors/BlockMakerVisitor.java | 53 ++++++++ .../core/dex/visitors/DotGraphVisitor.java | 29 +++- .../dex/visitors/MethodInlineVisitor.java | 3 +- .../dex/visitors/regions/CheckRegions.java | 4 +- .../dex/visitors/regions/CleanRegions.java | 2 +- .../regions/DepthRegionTraversal.java | 71 ++++++++++ .../regions/DepthRegionTraverser.java | 31 ----- .../regions/IRegionIterativeVisitor.java | 13 ++ .../dex/visitors/regions/IfRegionVisitor.java | 127 ++++++++++++++++++ .../visitors/regions/ProcessReturnInsns.java | 1 + .../visitors/regions/ProcessVariables.java | 2 +- .../dex/visitors/regions/RegionMaker.java | 21 ++- .../visitors/regions/RegionMakerVisitor.java | 39 +----- .../{TernaryVisitor.java => TernaryMod.java} | 44 +----- .../main/java/jadx/core/utils/BlockUtils.java | 29 +++- .../java/jadx/core/utils/EmptyBitSet.java | 85 ++++++++++++ .../internal/conditions/TestConditions.java | 5 +- .../internal/conditions/TestConditions3.java | 72 ++++++++++ .../internal/conditions/TestConditions4.java | 31 +++++ .../internal/conditions/TestConditions5.java | 37 +++++ .../tests/internal/inner/TestInnerClass3.java | 40 ++++++ .../java/jadx/samples/TestConditions.java | 3 - 28 files changed, 656 insertions(+), 148 deletions(-) create mode 100644 jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraversal.java delete mode 100644 jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraverser.java create mode 100644 jadx-core/src/main/java/jadx/core/dex/visitors/regions/IRegionIterativeVisitor.java create mode 100644 jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java rename jadx-core/src/main/java/jadx/core/dex/visitors/regions/{TernaryVisitor.java => TernaryMod.java} (68%) create mode 100644 jadx-core/src/main/java/jadx/core/utils/EmptyBitSet.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions3.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions4.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions5.java create mode 100644 jadx-core/src/test/java/jadx/tests/internal/inner/TestInnerClass3.java diff --git a/jadx-core/src/main/java/jadx/core/Jadx.java b/jadx-core/src/main/java/jadx/core/Jadx.java index 1d344b2d6..d1b3ce496 100644 --- a/jadx-core/src/main/java/jadx/core/Jadx.java +++ b/jadx-core/src/main/java/jadx/core/Jadx.java @@ -15,9 +15,9 @@ import jadx.core.dex.visitors.ModVisitor; import jadx.core.dex.visitors.PrepareForCodeGen; import jadx.core.dex.visitors.SimplifyVisitor; import jadx.core.dex.visitors.regions.CheckRegions; +import jadx.core.dex.visitors.regions.IfRegionVisitor; import jadx.core.dex.visitors.regions.ProcessVariables; import jadx.core.dex.visitors.regions.RegionMakerVisitor; -import jadx.core.dex.visitors.regions.TernaryVisitor; import jadx.core.dex.visitors.typeresolver.FinishTypeResolver; import jadx.core.dex.visitors.typeresolver.TypeResolver; import jadx.core.utils.Utils; @@ -70,7 +70,7 @@ public class Jadx { passes.add(new CodeShrinker()); passes.add(new RegionMakerVisitor()); - passes.add(new TernaryVisitor()); + passes.add(new IfRegionVisitor()); passes.add(new CodeShrinker()); passes.add(new SimplifyVisitor()); diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/mods/TernaryInsn.java b/jadx-core/src/main/java/jadx/core/dex/instructions/mods/TernaryInsn.java index e23819ecc..f408f8009 100644 --- a/jadx-core/src/main/java/jadx/core/dex/instructions/mods/TernaryInsn.java +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/mods/TernaryInsn.java @@ -2,6 +2,7 @@ package jadx.core.dex.instructions.mods; import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.args.InsnArg; +import jadx.core.dex.instructions.args.LiteralArg; import jadx.core.dex.instructions.args.RegisterArg; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.regions.IfCondition; @@ -14,10 +15,18 @@ public class TernaryInsn extends InsnNode { public TernaryInsn(IfCondition condition, RegisterArg result, InsnArg th, InsnArg els) { super(InsnType.TERNARY, 2); - this.condition = condition; setResult(result); - addArg(th); - addArg(els); + + if (th.equals(LiteralArg.FALSE) && els.equals(LiteralArg.TRUE)) { + // inverted + this.condition = IfCondition.invert(condition); + addArg(els); + addArg(th); + } else { + this.condition = condition; + addArg(th); + addArg(els); + } } public IfCondition getCondition() { diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java index 0dc25a49e..214868fbb 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java @@ -22,8 +22,13 @@ public class BlockNode extends AttrNode implements IBlock { private List successors = new ArrayList(1); private List cleanSuccessors; - private BitSet doms; // all dominators - private BlockNode idom; // immediate dominator + // all dominators + private BitSet doms; + // dominance frontier + private BitSet domFrontier; + // immediate dominator + private BlockNode idom; + // blocks on which dominates this block private final List dominatesOn = new ArrayList(1); private BlockRegState startState; @@ -118,6 +123,14 @@ public class BlockNode extends AttrNode implements IBlock { this.doms = doms; } + public BitSet getDomFrontier() { + return domFrontier; + } + + public void setDomFrontier(BitSet domFrontier) { + this.domFrontier = domFrontier; + } + /** * Immediate dominator */ diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java b/jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java index 91969e5cd..720e32baf 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/IRegion.java @@ -8,4 +8,5 @@ public interface IRegion extends IContainer { List getSubBlocks(); + boolean replaceSubBlock(IContainer oldBlock, IContainer newBlock); } diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java b/jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java index 75ba981dc..3f4728546 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/AbstractRegion.java @@ -1,6 +1,7 @@ package jadx.core.dex.regions; import jadx.core.dex.attributes.AttrNode; +import jadx.core.dex.nodes.IContainer; import jadx.core.dex.nodes.IRegion; public abstract class AbstractRegion extends AttrNode implements IRegion { @@ -19,4 +20,10 @@ public abstract class AbstractRegion extends AttrNode implements IRegion { public void setParent(IRegion parent) { this.parent = parent; } + + @Override + public boolean replaceSubBlock(IContainer oldBlock, IContainer newBlock) { + // TODO: implement for others regions + return false; + } } diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/Region.java b/jadx-core/src/main/java/jadx/core/dex/regions/Region.java index 421edae3b..116d6bd48 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/Region.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/Region.java @@ -21,6 +21,23 @@ public final class Region extends AbstractRegion { return blocks; } + public void add(IContainer region) { + if (region instanceof AbstractRegion) { + ((AbstractRegion) region).setParent(this); + } + blocks.add(region); + } + + @Override + public boolean replaceSubBlock(IContainer oldBlock, IContainer newBlock) { + int i = blocks.indexOf(oldBlock); + if (i != -1) { + blocks.set(i, newBlock); + return true; + } + return false; + } + @Override public String toString() { StringBuilder sb = new StringBuilder(); @@ -35,5 +52,4 @@ public final class Region extends AbstractRegion { } return sb.toString(); } - } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/BlockMakerVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/BlockMakerVisitor.java index 2a5a6c27b..1449cb691 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/BlockMakerVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/BlockMakerVisitor.java @@ -19,6 +19,7 @@ import jadx.core.dex.trycatch.CatchAttr; import jadx.core.dex.trycatch.ExceptionHandler; import jadx.core.dex.trycatch.SplitterBlockAttr; import jadx.core.utils.BlockUtils; +import jadx.core.utils.EmptyBitSet; import jadx.core.utils.exceptions.JadxRuntimeException; import java.util.ArrayList; @@ -41,6 +42,8 @@ public class BlockMakerVisitor extends AbstractVisitor { InsnType.MONITOR_ENTER, InsnType.MONITOR_EXIT); + private static final BitSet EMPTY_BITSET = new EmptyBitSet(); + private static int nextBlockId; @Override @@ -298,6 +301,56 @@ public class BlockMakerVisitor extends AbstractVisitor { } } } + + computeDominanceFrontier(mth); + } + + private static void computeDominanceFrontier(MethodNode mth) { + for (BlockNode exit : mth.getExitBlocks()) { + exit.setDomFrontier(EMPTY_BITSET); + } + for (BlockNode block : mth.getBasicBlocks()) { + computeBlockDF(block); + } + } + + private static void computeBlockDF(BlockNode block) { + if (block.getDomFrontier() != null) { + return; + } + BitSet domFrontier = null; + BitSet doms = block.getDoms(); + int id = block.getId(); + + for (BlockNode s : block.getSuccessors()) { + if (s.getIDom() != block) { + if (domFrontier == null) { + domFrontier = new BitSet(); + } + domFrontier.set(s.getId()); + } + } + for (BlockNode node : block.getDominatesOn()) { + if (node.getIDom() == block) { + BitSet frontier = node.getDomFrontier(); + if (frontier == null) { + computeBlockDF(node); + frontier = node.getDomFrontier(); + } + for (int w = frontier.nextSetBit(0); w >= 0; w = frontier.nextSetBit(w + 1)) { + if (id == w || !doms.get(w)) { + if (domFrontier == null) { + domFrontier = new BitSet(); + } + domFrontier.set(w); + } + } + } + } + if (domFrontier == null || domFrontier.cardinality() == 0) { + domFrontier = EMPTY_BITSET; + } + block.setDomFrontier(domFrontier); } private static void markReturnBlocks(MethodNode mth) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/DotGraphVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/DotGraphVisitor.java index b90521ea1..db7dfd0aa 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/DotGraphVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/DotGraphVisitor.java @@ -11,6 +11,7 @@ import jadx.core.dex.nodes.IRegion; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; import jadx.core.dex.trycatch.ExceptionHandler; +import jadx.core.utils.BlockUtils; import jadx.core.utils.InsnUtils; import jadx.core.utils.RegionUtils; import jadx.core.utils.Utils; @@ -23,6 +24,7 @@ import java.util.Set; public class DotGraphVisitor extends AbstractVisitor { private static final String NL = "\\l"; + private static final boolean PRINT_DOMINATORS = false; private final File dir; private final boolean useRegions; @@ -161,12 +163,29 @@ public class DotGraphVisitor extends AbstractVisitor { falsePath = ((IfNode) list.get(0)).getElseBlock(); } for (BlockNode next : block.getSuccessors()) { - conn.startLine(makeName(block)).add(" -> ").add(makeName(next)); - if (next == falsePath) { - conn.add("[style=dotted]"); - } - conn.add(';'); + String style = next == falsePath ? "[style=dashed]" : ""; + addEdge(block, next, style); } + + if (PRINT_DOMINATORS) { + for (BlockNode dom : BlockUtils.bitSetToBlocks(mth, block.getDoms())) { + String style = "[color=green]"; + if (dom == block.getIDom()) { + style = "[style=dashed, color=green]"; + } + addEdge(block, dom, style); + } + + for (BlockNode dom : BlockUtils.bitSetToBlocks(mth, block.getDomFrontier())) { + addEdge(block, dom, "[color=blue]"); + } + } + } + + private void addEdge(BlockNode from, BlockNode to, String style) { + conn.startLine(makeName(from)).add(" -> ").add(makeName(to)); + conn.add(style); + conn.add(';'); } private String attributesString(IAttributeNode block) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/MethodInlineVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/MethodInlineVisitor.java index 90e2e6f0d..7e0b6abc5 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/MethodInlineVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/MethodInlineVisitor.java @@ -22,7 +22,8 @@ public class MethodInlineVisitor extends AbstractVisitor { && accessFlags.isStatic() && mth.getBasicBlocks().size() == 2) { BlockNode block = mth.getBasicBlocks().get(1); - if (block.getAttributes().contains(AttributeFlag.RETURN)) { + if (block.getInstructions().isEmpty() + || block.getAttributes().contains(AttributeFlag.RETURN)) { inlineMth(mth); } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CheckRegions.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CheckRegions.java index d3f48f711..8519438e4 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CheckRegions.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CheckRegions.java @@ -38,7 +38,7 @@ public class CheckRegions extends AbstractVisitor { } } }; - DepthRegionTraverser.traverseAll(mth, collectBlocks); + DepthRegionTraversal.traverseAll(mth, collectBlocks); if (mth.getBasicBlocks().size() != blocksInRegions.size()) { for (BlockNode block : mth.getBasicBlocks()) { @@ -65,6 +65,6 @@ public class CheckRegions extends AbstractVisitor { } } }; - DepthRegionTraverser.traverseAll(mth, checkLoops); + DepthRegionTraversal.traverseAll(mth, checkLoops); } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CleanRegions.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CleanRegions.java index 75334a2e9..139c3e1c1 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CleanRegions.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/CleanRegions.java @@ -44,6 +44,6 @@ public class CleanRegions { } } }; - DepthRegionTraverser.traverseAll(mth, removeEmptyBlocks); + DepthRegionTraversal.traverseAll(mth, removeEmptyBlocks); } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraversal.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraversal.java new file mode 100644 index 000000000..d605ea8a7 --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraversal.java @@ -0,0 +1,71 @@ +package jadx.core.dex.visitors.regions; + +import jadx.core.dex.nodes.IBlock; +import jadx.core.dex.nodes.IContainer; +import jadx.core.dex.nodes.IRegion; +import jadx.core.dex.nodes.MethodNode; +import jadx.core.dex.trycatch.ExceptionHandler; + +public class DepthRegionTraversal { + + private DepthRegionTraversal() { + } + + public static void traverse(MethodNode mth, IRegionVisitor visitor) { + traverseInternal(mth, visitor, mth.getRegion()); + } + + public static void traverseAll(MethodNode mth, IRegionVisitor visitor) { + traverse(mth, visitor); + for (ExceptionHandler h : mth.getExceptionHandlers()) { + traverseInternal(mth, visitor, h.getHandlerRegion()); + } + } + + public static void traverseAllIterative(MethodNode mth, IRegionIterativeVisitor visitor) { + boolean repeat; + do { + repeat = traverseAllIterativeIntern(mth, visitor); + } while (repeat); + } + + private static void traverseInternal(MethodNode mth, IRegionVisitor visitor, IContainer container) { + if (container instanceof IBlock) { + visitor.processBlock(mth, (IBlock) container); + } else if (container instanceof IRegion) { + IRegion region = (IRegion) container; + visitor.enterRegion(mth, region); + for (IContainer subCont : region.getSubBlocks()) { + traverseInternal(mth, visitor, subCont); + } + visitor.leaveRegion(mth, region); + } + } + + private static boolean traverseAllIterativeIntern(MethodNode mth, IRegionIterativeVisitor visitor) { + if (traverseIterativeInternal(mth, visitor, mth.getRegion())) { + return true; + } + for (ExceptionHandler h : mth.getExceptionHandlers()) { + if (traverseIterativeInternal(mth, visitor, h.getHandlerRegion())) { + return true; + } + } + return false; + } + + public static boolean traverseIterativeInternal(MethodNode mth, IRegionIterativeVisitor visitor, IContainer container) { + if (container instanceof IRegion) { + IRegion region = (IRegion) container; + if (visitor.visitRegion(mth, region)) { + return true; + } + for (IContainer subCont : region.getSubBlocks()) { + if (traverseIterativeInternal(mth, visitor, subCont)) { + return true; + } + } + } + return false; + } +} diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraverser.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraverser.java deleted file mode 100644 index 2d773a4d3..000000000 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/DepthRegionTraverser.java +++ /dev/null @@ -1,31 +0,0 @@ -package jadx.core.dex.visitors.regions; - -import jadx.core.dex.nodes.IBlock; -import jadx.core.dex.nodes.IContainer; -import jadx.core.dex.nodes.IRegion; -import jadx.core.dex.nodes.MethodNode; -import jadx.core.dex.trycatch.ExceptionHandler; - -public class DepthRegionTraverser { - - public static void traverse(MethodNode mth, IRegionVisitor visitor, IContainer container) { - if (container instanceof IBlock) { - visitor.processBlock(mth, (IBlock) container); - } else if (container instanceof IRegion) { - IRegion region = (IRegion) container; - visitor.enterRegion(mth, region); - for (IContainer subCont : region.getSubBlocks()) { - traverse(mth, visitor, subCont); - } - visitor.leaveRegion(mth, region); - } - } - - public static void traverseAll(MethodNode mth, IRegionVisitor visitor) { - traverse(mth, visitor, mth.getRegion()); - - for (ExceptionHandler h : mth.getExceptionHandlers()) { - traverse(mth, visitor, h.getHandlerRegion()); - } - } -} diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IRegionIterativeVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IRegionIterativeVisitor.java new file mode 100644 index 000000000..1785525ae --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IRegionIterativeVisitor.java @@ -0,0 +1,13 @@ +package jadx.core.dex.visitors.regions; + +import jadx.core.dex.nodes.IRegion; +import jadx.core.dex.nodes.MethodNode; + +public interface IRegionIterativeVisitor { + + /** + * If return 'true' traversal will be restarted. + */ + boolean visitRegion(MethodNode mth, IRegion region); + +} diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java new file mode 100644 index 000000000..833f8e574 --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java @@ -0,0 +1,127 @@ +package jadx.core.dex.visitors.regions; + +import jadx.core.dex.attributes.AttributeFlag; +import jadx.core.dex.instructions.args.ArgType; +import jadx.core.dex.nodes.IBlock; +import jadx.core.dex.nodes.IContainer; +import jadx.core.dex.nodes.IRegion; +import jadx.core.dex.nodes.MethodNode; +import jadx.core.dex.regions.IfCondition; +import jadx.core.dex.regions.IfRegion; +import jadx.core.dex.regions.Region; +import jadx.core.dex.visitors.AbstractVisitor; +import jadx.core.utils.RegionUtils; + +import java.util.List; + +public class IfRegionVisitor extends AbstractVisitor implements IRegionVisitor, IRegionIterativeVisitor { + + @Override + public void visit(MethodNode mth) { + DepthRegionTraversal.traverseAll(mth, this); + DepthRegionTraversal.traverseAllIterative(mth, this); + } + + @Override + public void enterRegion(MethodNode mth, IRegion region) { + if (region instanceof IfRegion) { + processIfRegion(mth, (IfRegion) region); + } + } + + @Override + public boolean visitRegion(MethodNode mth, IRegion region) { + if (region instanceof IfRegion) { + return removeRedundantElseBlock((IfRegion) region); + } + return false; + } + + @Override + public void processBlock(MethodNode mth, IBlock container) { + } + + @Override + public void leaveRegion(MethodNode mth, IRegion region) { + } + + private static void processIfRegion(MethodNode mth, IfRegion ifRegion) { + simplifyIfCondition(ifRegion); + moveReturnToThenBlock(mth, ifRegion); + markElseIfChains(ifRegion); + + TernaryMod.makeTernaryInsn(mth, ifRegion); + } + + private static void simplifyIfCondition(IfRegion ifRegion) { + if (ifRegion.simplifyCondition()) { + IfCondition condition = ifRegion.getCondition(); + if (condition.getMode() == IfCondition.Mode.NOT) { + tryInvertIfRegion(ifRegion); + } + } + } + + private static void moveReturnToThenBlock(MethodNode mth, IfRegion ifRegion) { + if (!mth.getReturnType().equals(ArgType.VOID) + && hasSimpleReturnBlock(ifRegion.getElseRegion()) + && !hasSimpleReturnBlock(ifRegion.getThenRegion())) { + tryInvertIfRegion(ifRegion); + } + } + + private static boolean removeRedundantElseBlock(IfRegion ifRegion) { + if (ifRegion.getElseRegion() != null && hasSimpleReturnBlock(ifRegion.getThenRegion())) { + IRegion parent = ifRegion.getParent(); + Region newRegion = new Region(parent); + if (parent.replaceSubBlock(ifRegion, newRegion)) { + newRegion.add(ifRegion); + newRegion.add(ifRegion.getElseRegion()); + ifRegion.setElseRegion(null); + return true; + } + } + return false; + } + + /** + * Mark if-else-if chains + */ + private static void markElseIfChains(IfRegion ifRegion) { + IContainer elsRegion = ifRegion.getElseRegion(); + if (elsRegion != null) { + if (elsRegion instanceof IfRegion) { + elsRegion.getAttributes().add(AttributeFlag.ELSE_IF_CHAIN); + } else if (elsRegion instanceof Region) { + List subBlocks = ((Region) elsRegion).getSubBlocks(); + if (subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion) { + subBlocks.get(0).getAttributes().add(AttributeFlag.ELSE_IF_CHAIN); + } + } + } + } + + private static void tryInvertIfRegion(IfRegion ifRegion) { + IContainer elseRegion = ifRegion.getElseRegion(); + if (elseRegion != null && RegionUtils.notEmpty(elseRegion)) { + ifRegion.invert(); + } + } + + private static boolean hasSimpleReturnBlock(IContainer region) { + if (region == null) { + return false; + } + if (region.getAttributes().contains(AttributeFlag.RETURN)) { + return true; + } + if (region instanceof IRegion) { + List subBlocks = ((IRegion) region).getSubBlocks(); + if (subBlocks.size() == 1 + && subBlocks.get(0).getAttributes().contains(AttributeFlag.RETURN)) { + return true; + } + } + return false; + } +} diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessReturnInsns.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessReturnInsns.java index 830dd7074..db65e0678 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessReturnInsns.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessReturnInsns.java @@ -32,6 +32,7 @@ public class ProcessReturnInsns extends TracedRegionVisitor { && blockNotInLoop(mth, block) && noTrailInstructions(block)) { insns.remove(insns.size() - 1); + block.getAttributes().remove(AttributeFlag.RETURN); } } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java index 13dfccfa8..685431b27 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java @@ -104,7 +104,7 @@ public class ProcessVariables extends AbstractVisitor { } } }; - DepthRegionTraverser.traverseAll(mth, collect); + DepthRegionTraversal.traverseAll(mth, collect); // reduce assigns map List mthArgs = mth.getArguments(true); diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java index 39bdc6141..56c7a83bd 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java @@ -412,27 +412,22 @@ public class RegionMaker { BlockNode thenBlock = null; BlockNode elseBlock = null; - for (BlockNode d : block.getDominatesOn()) { - if (d != bThen && d != bElse) { - out = d; - break; - } - } - IfRegion ifRegion = new IfRegion(currentRegion, block); currentRegion.getSubBlocks().add(ifRegion); IfInfo mergedIf = mergeNestedIfNodes(block, bThen, bElse, null); if (mergedIf != null) { - block = mergedIf.getIfnode(); ifRegion.setCondition(mergedIf.getCondition()); thenBlock = mergedIf.getThenBlock(); elseBlock = mergedIf.getElseBlock(); - bThen = thenBlock; - bElse = elseBlock; - } - - if (thenBlock == null) { + out = BlockUtils.getPathCrossBlockFor(mth, thenBlock, elseBlock); + } else { + for (BlockNode d : block.getDominatesOn()) { + if (d != bThen && d != bElse) { + out = d; + break; + } + } // invert condition (compiler often do it) ifnode.invertCondition(); BlockNode tmp = bThen; diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMakerVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMakerVisitor.java index a91aaf4c4..91c234340 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMakerVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMakerVisitor.java @@ -1,12 +1,10 @@ package jadx.core.dex.visitors.regions; -import jadx.core.dex.attributes.AttributeFlag; import jadx.core.dex.instructions.args.ArgType; import jadx.core.dex.nodes.IContainer; import jadx.core.dex.nodes.IRegion; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; -import jadx.core.dex.regions.IfRegion; import jadx.core.dex.regions.LoopRegion; import jadx.core.dex.regions.Region; import jadx.core.dex.regions.SynchronizedRegion; @@ -49,11 +47,11 @@ public class RegionMakerVisitor extends AbstractVisitor { private static void postProcessRegions(MethodNode mth) { // make try-catch regions - DepthRegionTraverser.traverse(mth, new ProcessTryCatchRegions(mth), mth.getRegion()); + DepthRegionTraversal.traverse(mth, new ProcessTryCatchRegions(mth)); // merge conditions in loops if (mth.getLoopsCount() != 0) { - DepthRegionTraverser.traverseAll(mth, new AbstractRegionVisitor() { + DepthRegionTraversal.traverseAll(mth, new AbstractRegionVisitor() { @Override public void enterRegion(MethodNode mth, IRegion region) { if (region instanceof LoopRegion) { @@ -66,45 +64,14 @@ public class RegionMakerVisitor extends AbstractVisitor { CleanRegions.process(mth); - DepthRegionTraverser.traverseAll(mth, new AbstractRegionVisitor() { - @Override - public void leaveRegion(MethodNode mth, IRegion region) { - if (region instanceof IfRegion) { - processIfRegion((IfRegion) region); - - } - } - }); - // remove useless returns in void methods if (mth.getReturnType().equals(ArgType.VOID)) { - DepthRegionTraverser.traverseAll(mth, new ProcessReturnInsns()); + DepthRegionTraversal.traverseAll(mth, new ProcessReturnInsns()); } if (mth.getAccessFlags().isSynchronized()) { removeSynchronized(mth); } - - } - - private static void processIfRegion(IfRegion ifRegion) { - if (ifRegion.simplifyCondition()) { -// IfCondition condition = ifRegion.getCondition(); -// if (condition.getMode() == IfCondition.Mode.NOT) { -// ifRegion.invert(); -// } - } - - // mark if-else-if chains - IContainer elsRegion = ifRegion.getElseRegion(); - if (elsRegion instanceof IfRegion) { - elsRegion.getAttributes().add(AttributeFlag.ELSE_IF_CHAIN); - } else if (elsRegion instanceof Region) { - List subBlocks = ((Region) elsRegion).getSubBlocks(); - if (subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion) { - subBlocks.get(0).getAttributes().add(AttributeFlag.ELSE_IF_CHAIN); - } - } } private static void removeSynchronized(MethodNode mth) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java similarity index 68% rename from jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryVisitor.java rename to jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java index c41c72218..a2fcc3bff 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java @@ -4,49 +4,28 @@ import jadx.core.dex.attributes.AttributeFlag; import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.args.ArgType; import jadx.core.dex.instructions.args.InsnArg; -import jadx.core.dex.instructions.args.LiteralArg; import jadx.core.dex.instructions.mods.TernaryInsn; import jadx.core.dex.nodes.BlockNode; -import jadx.core.dex.nodes.ClassNode; import jadx.core.dex.nodes.IContainer; -import jadx.core.dex.nodes.IRegion; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; -import jadx.core.dex.regions.IfCondition; import jadx.core.dex.regions.IfRegion; import jadx.core.dex.regions.Region; import jadx.core.dex.regions.TernaryRegion; import jadx.core.dex.visitors.CodeShrinker; -import jadx.core.dex.visitors.IDexTreeVisitor; import jadx.core.utils.InsnList; -import jadx.core.utils.exceptions.JadxException; import java.util.List; -public class TernaryVisitor extends AbstractRegionVisitor implements IDexTreeVisitor { +public class TernaryMod { - private static final LiteralArg FALSE_ARG = InsnArg.lit(0, ArgType.BOOLEAN); - private static final LiteralArg TRUE_ARG = InsnArg.lit(1, ArgType.BOOLEAN); - - @Override - public boolean visit(ClassNode cls) throws JadxException { - return true; + private TernaryMod() { } - @Override - public void visit(MethodNode mth) { - DepthRegionTraverser.traverseAll(mth, this); - } - - @Override - public void enterRegion(MethodNode mth, IRegion region) { - if (!(region instanceof IfRegion)) { + static void makeTernaryInsn(MethodNode mth, IfRegion ifRegion) { + if (ifRegion.getAttributes().contains(AttributeFlag.ELSE_IF_CHAIN)) { return; } - if (region.getAttributes().contains(AttributeFlag.ELSE_IF_CHAIN)) { - return; - } - IfRegion ifRegion = (IfRegion) region; IContainer thenRegion = ifRegion.getThenRegion(); IContainer elseRegion = ifRegion.getElseRegion(); if (thenRegion == null || elseRegion == null) { @@ -89,25 +68,12 @@ public class TernaryVisitor extends AbstractRegionVisitor implements IDexTreeVis if (!mth.getReturnType().equals(ArgType.VOID) && t.getType() == InsnType.RETURN && e.getType() == InsnType.RETURN) { - boolean inverted = false; - InsnArg thenArg = t.getArg(0); - InsnArg elseArg = e.getArg(0); - if (thenArg.equals(FALSE_ARG) && elseArg.equals(TRUE_ARG)) { - inverted = true; - } InsnList.remove(tb, t); InsnList.remove(eb, e); tb.getAttributes().remove(AttributeFlag.RETURN); eb.getAttributes().remove(AttributeFlag.RETURN); - IfCondition condition = ifRegion.getCondition(); - if (inverted) { - condition = IfCondition.invert(condition); - InsnArg tmp = thenArg; - thenArg = elseArg; - elseArg = tmp; - } - TernaryInsn ternInsn = new TernaryInsn(condition, null, thenArg, elseArg); + TernaryInsn ternInsn = new TernaryInsn(ifRegion.getCondition(), null, t.getArg(0), e.getArg(0)); InsnNode retInsn = new InsnNode(InsnType.RETURN, 1); retInsn.addArg(InsnArg.wrapArg(ternInsn)); diff --git a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java index 5dfa538b9..ded1a1bd6 100644 --- a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java @@ -169,17 +169,18 @@ public class BlockUtils { } } - private static boolean traverseSuccessorsUntil(BlockNode from, BlockNode until, Set checked) { + private static boolean traverseSuccessorsUntil(BlockNode from, BlockNode until, BitSet visited) { for (BlockNode s : from.getCleanSuccessors()) { if (s == until) { return true; } - if (!checked.contains(s)) { - checked.add(s); + int id = s.getId(); + if (!visited.get(id)) { + visited.set(id); if (until.isDominator(s)) { return true; } - if (traverseSuccessorsUntil(s, until, checked)) { + if (traverseSuccessorsUntil(s, until, visited)) { return true; } } @@ -194,7 +195,7 @@ public class BlockUtils { if (end.isDominator(start)) { return true; } - return traverseSuccessorsUntil(start, end, new HashSet()); + return traverseSuccessorsUntil(start, end, new BitSet()); } public static boolean isOnlyOnePathExists(BlockNode start, BlockNode end) { @@ -230,6 +231,24 @@ public class BlockUtils { return null; } + public static BlockNode getPathCrossBlockFor(MethodNode mth, BlockNode b1, BlockNode b2) { + if (b1 == null || b2 == null) { + return null; + } + BitSet b = new BitSet(); + b.or(b1.getDomFrontier()); + b.or(b2.getDomFrontier()); + b.clear(b1.getId()); + b.clear(b2.getId()); + if (b.cardinality() == 1) { + BlockNode end = mth.getBasicBlocks().get(b.nextSetBit(0)); + if (isPathExists(b1, end) && isPathExists(b2, end)) { + return end; + } + } + return null; + } + /** * Collect all block dominated by 'dominator', starting from 'start' */ diff --git a/jadx-core/src/main/java/jadx/core/utils/EmptyBitSet.java b/jadx-core/src/main/java/jadx/core/utils/EmptyBitSet.java new file mode 100644 index 000000000..8e478eaaa --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/utils/EmptyBitSet.java @@ -0,0 +1,85 @@ +package jadx.core.utils; + +import java.util.BitSet; + +public class EmptyBitSet extends BitSet { + + public EmptyBitSet() { + super(0); + } + + @Override + public int cardinality() { + return 0; + } + + @Override + public boolean isEmpty() { + return true; + } + + @Override + public int nextSetBit(int fromIndex) { + return -1; + } + + @Override + public int length() { + return 0; + } + + @Override + public int size() { + return 0; + } + + @Override + public void set(int bitIndex) { + throw new UnsupportedOperationException(); + } + + @Override + public void set(int bitIndex, boolean value) { + throw new UnsupportedOperationException(); + } + + @Override + public void set(int fromIndex, int toIndex) { + throw new UnsupportedOperationException(); + } + + @Override + public void set(int fromIndex, int toIndex, boolean value) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean get(int bitIndex) { + return false; + } + + @Override + public BitSet get(int fromIndex, int toIndex) { + throw new UnsupportedOperationException(); + } + + @Override + public void and(BitSet set) { + throw new UnsupportedOperationException(); + } + + @Override + public void or(BitSet set) { + throw new UnsupportedOperationException(); + } + + @Override + public void xor(BitSet set) { + throw new UnsupportedOperationException(); + } + + @Override + public void andNot(BitSet set) { + throw new UnsupportedOperationException(); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions.java index d1833a40e..0729c7326 100644 --- a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions.java +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions.java @@ -12,7 +12,7 @@ import static org.junit.Assert.assertThat; public class TestConditions extends InternalJadxTest { public static class TestCls { - private boolean f1(boolean a, boolean b, boolean c) { + private boolean test(boolean a, boolean b, boolean c) { return (a && b) || c; } } @@ -24,7 +24,6 @@ public class TestConditions extends InternalJadxTest { System.out.println(code); assertThat(code, not(containsString("(!a || !b) && !c"))); - assertThat(code, containsString("(a && b) || c")); -// assertThat(code, containsString("return (a && b) || c;")); + assertThat(code, containsString("return (a && b) || c;")); } } diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions3.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions3.java new file mode 100644 index 000000000..ea48bc63d --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions3.java @@ -0,0 +1,72 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import java.util.List; +import java.util.regex.Pattern; + +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestConditions3 extends InternalJadxTest { + + public static class TestCls { + private static final Pattern PATTERN = Pattern.compile("[a-f0-9]{20}"); + + public static Object test(final A a) { + List list = a.getList(); + if (list == null) { + return null; + } + if (list.size() != 1) { + return null; + } + String s = list.get(0); + if (isEmpty(s)) { + return null; + } + if (isDigitsOnly(s)) { + return new A().set(s); + } + if (PATTERN.matcher(s).matches()) { + return new A().set(s); + } + return null; + } + + private static boolean isDigitsOnly(String s) { + return false; + } + + private static boolean isEmpty(String s) { + return false; + } + + private static class A { + public Object set(String s) { + return null; + } + + public List getList() { + return null; + } + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsString("return null;")); + assertThat(code, not(containsString("else"))); + + // TODO: fix constant inline +// assertThat(code, not(containsString("AnonymousClass_1"))); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions4.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions4.java new file mode 100644 index 000000000..4f187f6ec --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions4.java @@ -0,0 +1,31 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestConditions4 extends InternalJadxTest { + + public static class TestCls { + public int test(int num) { + boolean inRange = (num >= 59 && num <= 66); + return inRange ? num + 1 : num; + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsString("num >= 59 && num <= 66")); + assertThat(code, containsString("return inRange ? num + 1 : num;")); + assertThat(code, not(containsString("else"))); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions5.java b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions5.java new file mode 100644 index 000000000..083f6cd89 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/conditions/TestConditions5.java @@ -0,0 +1,37 @@ +package jadx.tests.internal.conditions; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestConditions5 extends InternalJadxTest { + + public static class TestCls { + public static void assertEquals(Object a1, Object a2) { + if (a1 == null) { + if (a2 != null) + throw new AssertionError(a1 + " != " + a2); + } else if (!a1.equals(a2)) { + throw new AssertionError(a1 + " != " + a2); + } + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, containsString("if (a1 == null) {")); + assertThat(code, containsString("if (a2 != null) {")); + assertThat(code, containsString("throw new AssertionError(a1 + \" != \" + a2);")); + assertThat(code, not(containsString("if (a1.equals(a2)) {"))); + assertThat(code, containsString("} else if (!a1.equals(a2)) {")); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/internal/inner/TestInnerClass3.java b/jadx-core/src/test/java/jadx/tests/internal/inner/TestInnerClass3.java new file mode 100644 index 000000000..b7ad2dcb5 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/internal/inner/TestInnerClass3.java @@ -0,0 +1,40 @@ +package jadx.tests.internal.inner; + +import jadx.api.InternalJadxTest; +import jadx.core.dex.nodes.ClassNode; + +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +public class TestInnerClass3 extends InternalJadxTest { + + public static class TestCls { + private String c; + + private void setC(String c) { + this.c = c; + } + + public class C { + public String c() { + setC("c"); + return c; + } + } + } + + @Test + public void test() { + ClassNode cls = getClassNode(TestCls.class); + String code = cls.getCode().toString(); + System.out.println(code); + + assertThat(code, not(containsString("synthetic"))); + assertThat(code, not(containsString("access$"))); + assertThat(code, not(containsString("x0"))); + assertThat(code, containsString("setC(\"c\");")); + } +} diff --git a/jadx-samples/src/main/java/jadx/samples/TestConditions.java b/jadx-samples/src/main/java/jadx/samples/TestConditions.java index cab348196..7a7e97705 100644 --- a/jadx-samples/src/main/java/jadx/samples/TestConditions.java +++ b/jadx-samples/src/main/java/jadx/samples/TestConditions.java @@ -1,8 +1,5 @@ package jadx.samples; -/** - * Failed tests for current jadx version - */ public class TestConditions extends AbstractTest { public int test1(int num) {