core: fix 'break' detection in loops

jadx-core/src/main/java/jadx/core/codegen/RegionGen.java

@@ -93,14 +93,10 @@ public class RegionGen extends InsnGen {
 		for (InsnNode insn : block.getInstructions()) {
 			makeInsn(insn, code);
 		}
-		if (block.getAttributes().contains(AttributeFlag.BREAK)) {
-			code.startLine("break;");
-		} else {
-			IAttribute attr;
-			if ((attr = block.getAttributes().get(AttributeType.FORCE_RETURN)) != null) {
-				ForceReturnAttr retAttr = (ForceReturnAttr) attr;
-				makeInsn(retAttr.getReturnInsn(), code);
-			}
+		IAttribute attr;
+		if ((attr = block.getAttributes().get(AttributeType.FORCE_RETURN)) != null) {
+			ForceReturnAttr retAttr = (ForceReturnAttr) attr;
+			makeInsn(retAttr.getReturnInsn(), code);
 		}
 	}
 

jadx-core/src/main/java/jadx/core/dex/attributes/AttributeFlag.java

@@ -9,7 +9,6 @@ public enum AttributeFlag {
 
 	SYNTHETIC,
 
-	BREAK,
 	RETURN, // block contains only return instruction
 
 	DECLARE_VAR,

jadx-core/src/main/java/jadx/core/dex/attributes/LoopAttr.java

 package jadx.core.dex.attributes;
 
 import jadx.core.dex.nodes.BlockNode;
+import jadx.core.dex.nodes.Edge;
 import jadx.core.utils.BlockUtils;
 
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.LinkedList;
+import java.util.List;
 import java.util.Set;
 
 public class LoopAttr implements IAttribute {
@@ -37,16 +40,16 @@ public class LoopAttr implements IAttribute {
 	}
 
 	/**
-	 * Return block nodes with exit edges from loop <br>
+	 * Return source blocks of exit edges. <br>
 	 * Exit nodes belongs to loop (contains in {@code loopBlocks})
 	 */
 	public Set<BlockNode> getExitNodes() {
 		Set<BlockNode> nodes = new HashSet<BlockNode>();
-		Set<BlockNode> inloop = getLoopBlocks();
-		for (BlockNode block : inloop) {
+		Set<BlockNode> blocks = getLoopBlocks();
+		for (BlockNode block : blocks) {
 			// exit: successor node not from this loop, (don't change to getCleanSuccessors)
 			for (BlockNode s : block.getSuccessors()) {
-				if (!inloop.contains(s) && !s.getAttributes().contains(AttributeType.EXC_HANDLER)) {
+				if (!blocks.contains(s) && !s.getAttributes().contains(AttributeType.EXC_HANDLER)) {
 					nodes.add(block);
 				}
 			}
@@ -54,6 +57,22 @@ public class LoopAttr implements IAttribute {
 		return nodes;
 	}
 
+	/**
+	 * Return loop exit edges.
+	 */
+	public List<Edge> getExitEdges() {
+		List<Edge> edges = new LinkedList<Edge>();
+		Set<BlockNode> blocks = getLoopBlocks();
+		for (BlockNode block : blocks) {
+			for (BlockNode s : block.getSuccessors()) {
+				if (!blocks.contains(s) && !s.getAttributes().contains(AttributeType.EXC_HANDLER)) {
+					edges.add(new Edge(block, s));
+				}
+			}
+		}
+		return edges;
+	}
+
 	@Override
 	public String toString() {
 		return "LOOP: " + start + "->" + end;

jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java

 package jadx.core.dex.nodes;
 
 import jadx.core.dex.attributes.AttrNode;
+import jadx.core.dex.attributes.AttributeFlag;
 import jadx.core.dex.attributes.AttributeType;
 import jadx.core.dex.attributes.BlockRegState;
 import jadx.core.dex.attributes.LoopAttr;
@@ -148,6 +149,14 @@ public class BlockNode extends AttrNode implements IBlock {
 		this.endState = endState;
 	}
 
+	public boolean isSynthetic() {
+		return getAttributes().contains(AttributeFlag.SYNTHETIC);
+	}
+
+	public boolean isReturnBlock() {
+		return getAttributes().contains(AttributeFlag.RETURN);
+	}
+
 	@Override
 	public int hashCode() {
 		return id; // TODO id can change during reindex

jadx-core/src/main/java/jadx/core/dex/nodes/Edge.java

+package jadx.core.dex.nodes;
+
+public class Edge {
+	private final BlockNode source;
+	private final BlockNode target;
+
+	public Edge(BlockNode source, BlockNode target) {
+		this.source = source;
+		this.target = target;
+	}
+
+	public BlockNode getSource() {
+		return source;
+	}
+
+	public BlockNode getTarget() {
+		return target;
+	}
+
+	@Override
+	public boolean equals(Object o) {
+		if (this == o) {
+			return true;
+		}
+		if (o == null || getClass() != o.getClass()) {
+			return false;
+		}
+		Edge edge = (Edge) o;
+		return source.equals(edge.source) && target.equals(edge.target);
+
+	}
+
+	@Override
+	public int hashCode() {
+		return source.hashCode() + 31 * target.hashCode();
+	}
+
+	@Override
+	public String toString() {
+		return "Edge: " + source + " -> " + target;
+	}
+}

jadx-core/src/main/java/jadx/core/dex/visitors/BlockMakerVisitor.java

@@ -12,6 +12,7 @@ import jadx.core.dex.instructions.args.ArgType;
 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.Edge;
 import jadx.core.dex.nodes.InsnNode;
 import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.trycatch.CatchAttr;
@@ -299,6 +300,16 @@ public class BlockMakerVisitor extends AbstractVisitor {
 		}
 	}
 
+	private static void markReturnBlocks(MethodNode mth) {
+		mth.getExitBlocks().clear();
+		for (BlockNode block : mth.getBasicBlocks()) {
+			if (BlockUtils.lastInsnType(block, InsnType.RETURN)) {
+				block.getAttributes().add(AttributeFlag.RETURN);
+				mth.getExitBlocks().add(block);
+			}
+		}
+	}
+
 	private static void markLoops(MethodNode mth) {
 		for (BlockNode block : mth.getBasicBlocks()) {
 			for (BlockNode succ : block.getSuccessors()) {
@@ -316,16 +327,6 @@ public class BlockMakerVisitor extends AbstractVisitor {
 		}
 	}
 
-	private static void markReturnBlocks(MethodNode mth) {
-		mth.getExitBlocks().clear();
-		for (BlockNode block : mth.getBasicBlocks()) {
-			if (BlockUtils.lastInsnType(block, InsnType.RETURN)) {
-				block.getAttributes().add(AttributeFlag.RETURN);
-				mth.getExitBlocks().add(block);
-			}
-		}
-	}
-
 	private static void registerLoops(MethodNode mth) {
 		for (BlockNode block : mth.getBasicBlocks()) {
 			AttributesList attributes = block.getAttributes();
@@ -356,6 +357,7 @@ public class BlockMakerVisitor extends AbstractVisitor {
 				if (oneHeader) {
 					// several back edges connected to one loop header => make additional block
 					BlockNode newLoopHeader = startNewBlock(mth, block.getStartOffset());
+					newLoopHeader.getAttributes().add(AttributeFlag.SYNTHETIC);
 					connect(newLoopHeader, block);
 					for (IAttribute a : loops) {
 						LoopAttr la = (LoopAttr) a;
@@ -366,6 +368,24 @@ public class BlockMakerVisitor extends AbstractVisitor {
 					return true;
 				}
 			}
+			// insert additional blocks if loop has several exits
+			if (loops.size() == 1) {
+				LoopAttr loop = (LoopAttr) loops.get(0);
+				List<Edge> edges = loop.getExitEdges();
+				if (edges.size() > 1) {
+					boolean change = false;
+					for (Edge edge : edges) {
+						BlockNode target = edge.getTarget();
+						if (!target.getAttributes().contains(AttributeFlag.SYNTHETIC)) {
+							insertBlockBetween(mth, edge.getSource(), target);
+							change = true;
+						}
+					}
+					if (change) {
+						return true;
+					}
+				}
+			}
 		}
 		if (splitReturn(mth)) {
 			return true;
@@ -376,6 +396,15 @@ public class BlockMakerVisitor extends AbstractVisitor {
 		return false;
 	}
 
+	private static BlockNode insertBlockBetween(MethodNode mth, BlockNode source, BlockNode target) {
+		BlockNode newBlock = startNewBlock(mth, target.getStartOffset());
+		newBlock.getAttributes().add(AttributeFlag.SYNTHETIC);
+		removeConnection(source, target);
+		connect(source, newBlock);
+		connect(newBlock, target);
+		return newBlock;
+	}
+
 	/**
 	 * Merge return blocks for void methods
 	 */
@@ -414,7 +443,6 @@ public class BlockMakerVisitor extends AbstractVisitor {
 		if (mth.getExitBlocks().size() != 1) {
 			return false;
 		}
-		boolean split = false;
 		BlockNode exitBlock = mth.getExitBlocks().get(0);
 		if (exitBlock.getPredecessors().size() > 1
 				&& exitBlock.getInstructions().size() == 1
@@ -422,10 +450,9 @@ public class BlockMakerVisitor extends AbstractVisitor {
 				&& !exitBlock.getAttributes().contains(AttributeFlag.SYNTHETIC)) {
 			InsnNode returnInsn = exitBlock.getInstructions().get(0);
 			List<BlockNode> preds = new ArrayList<BlockNode>(exitBlock.getPredecessors());
-			if (returnInsn.getArgsCount() != 0 && !isReturnArgAssignInPred(mth, preds, returnInsn)) {
+			if (returnInsn.getArgsCount() != 0 && !isReturnArgAssignInPred(preds, returnInsn)) {
 				return false;
 			}
-			split = true;
 			for (BlockNode pred : preds) {
 				BlockNode newRetBlock = startNewBlock(mth, exitBlock.getStartOffset());
 				newRetBlock.getAttributes().add(AttributeFlag.SYNTHETIC);
@@ -433,14 +460,13 @@ public class BlockMakerVisitor extends AbstractVisitor {
 				removeConnection(pred, exitBlock);
 				connect(pred, newRetBlock);
 			}
-		}
-		if (split) {
 			cleanExitNodes(mth);
+			return true;
 		}
-		return split;
+		return false;
 	}
 
-	private static boolean isReturnArgAssignInPred(MethodNode mth, List<BlockNode> preds, InsnNode returnInsn) {
+	private static boolean isReturnArgAssignInPred(List<BlockNode> preds, InsnNode returnInsn) {
 		RegisterArg arg = (RegisterArg) returnInsn.getArg(0);
 		int regNum = arg.getRegNum();
 		for (BlockNode pred : preds) {

jadx-core/src/main/java/jadx/core/dex/visitors/ConstInlinerVisitor.java

@@ -46,39 +46,37 @@ public class ConstInlinerVisitor extends AbstractVisitor {
 				long lit = ((LiteralArg) arg).getLiteral();
 				return replaceConst(mth, block, insn, lit);
 			}
-			// TODO process string and class const
 		}
 		return false;
 	}
 
 	private static boolean replaceConst(MethodNode mth, BlockNode block, InsnNode insn, long literal) {
 		List<InsnArg> use = insn.getResult().getTypedVar().getUseList();
-		int replace = 0;
+		int replaceCount = 0;
 		for (InsnArg arg : use) {
 			InsnNode useInsn = arg.getParentInsn();
-			if (useInsn == null) {
+			if (arg == insn.getResult() || useInsn == null) {
 				continue;
 			}
 			BlockNode useBlock = BlockUtils.getBlockByInsn(mth, useInsn);
-			if (useBlock == block || useBlock.isDominator(block)) {
-				if (arg != insn.getResult() && !registerReassignOnPath(block, useBlock, insn)) {
-					LiteralArg litArg;
-					if (use.size() == 2) {
-						// arg used only in one place
-						litArg = InsnArg.lit(literal, arg.getType());
-					} else {
-						// in most cases type not equal arg.getType()
-						// just set unknown type and run type fixer
-						litArg = InsnArg.lit(literal, ArgType.UNKNOWN);
-					}
-					if (useInsn.replaceArg(arg, litArg)) {
-						fixTypes(mth, useInsn, litArg);
-						replace++;
-					}
+			boolean isDominator = useBlock == block || useBlock.isDominator(block);
+			if (isDominator && !registerReassignOnPath(block, useBlock, insn)) {
+				LiteralArg litArg;
+				if (use.size() == 2) {
+					// arg used only in one place
+					litArg = InsnArg.lit(literal, arg.getType());
+				} else {
+					// in most cases type not equal arg.getType()
+					// just set unknown type and run type fixer
+					litArg = InsnArg.lit(literal, ArgType.UNKNOWN);
+				}
+				if (useInsn.replaceArg(arg, litArg)) {
+					fixTypes(mth, useInsn, litArg);
+					replaceCount++;
 				}
 			}
 		}
-		return (replace + 1) == use.size();
+		return replaceCount == use.size() - 1;
 	}
 
 	private static boolean registerReassignOnPath(BlockNode block, BlockNode useBlock, InsnNode assignInsn) {
@@ -86,7 +84,6 @@ public class ConstInlinerVisitor extends AbstractVisitor {
 			return false;
 		}
 		Set<BlockNode> blocks = BlockUtils.getAllPathsBlocks(block, useBlock);
-		// TODO store list of assign insn for each register
 		int regNum = assignInsn.getResult().getRegNum();
 		for (BlockNode b : blocks) {
 			for (InsnNode insn : b.getInstructions()) {

jadx-core/src/main/java/jadx/core/dex/visitors/typeresolver/TypeResolver.java

@@ -10,7 +10,11 @@ import jadx.core.dex.visitors.AbstractVisitor;
 
 import java.util.List;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 public class TypeResolver extends AbstractVisitor {
+	private static final Logger LOG = LoggerFactory.getLogger(TypeResolver.class);
 
 	@Override
 	public void visit(MethodNode mth) {
@@ -67,11 +71,22 @@ public class TypeResolver extends AbstractVisitor {
 			}
 		} while (changed);
 
-		for (BlockNode block : mth.getBasicBlocks()) {
-			for (BlockNode dest : block.getSuccessors()) {
-				connectEdges(mth, block, dest, false);
+		int i = 0;
+		do {
+			changed = false;
+			for (BlockNode block : mth.getBasicBlocks()) {
+				for (BlockNode dest : block.getSuccessors()) {
+					if (connectEdges(mth, block, dest, false)) {
+						changed = true;
+					}
+				}
 			}
-		}
+			i++;
+			if (i > 10) {
+				LOG.warn("Can't resolve types (forward connectEdges pass) in method: {}", mth);
+				break;
+			}
+		} while (changed && i < 10);
 	}
 
 	private static boolean connectEdges(MethodNode mth, BlockNode from, BlockNode to, boolean back) {

jadx-core/src/test/java/jadx/tests/internal/TestReturnWrapping.java

@@ -10,16 +10,15 @@ import static org.junit.Assert.assertThat;
 
 public class TestReturnWrapping extends InternalJadxTest {
 	public static class TestCls {
-		/**/
+
 		public static int f1(int arg0) {
 			switch (arg0) {
 				case 1:
 					return 255;
 			}
 			return arg0 + 1;
-		}/**/
+		}
 
-		/**/
 		public static Object f2(Object arg0, int arg1) {
 			Object ret = null;
 			int i = arg1;
@@ -34,9 +33,8 @@ public class TestReturnWrapping extends InternalJadxTest {
 				}
 				return i > 128 ? arg0.toString() + ret.toString() : i;
 			}
-		}/**/
+		}
 
-		/**/
 		public static int f3(int arg0) {
 			while (arg0 > 10) {
 				int abc = 951;
@@ -46,7 +44,7 @@ public class TestReturnWrapping extends InternalJadxTest {
 				arg0 -= abc;
 			}
 			return arg0;
-		}/**/
+		}
 	}
 
 	@Test
@@ -56,7 +54,6 @@ public class TestReturnWrapping extends InternalJadxTest {
 
 		assertThat(code, containsString("return 255;"));
 		assertThat(code, containsString("return arg0 + 1;"));
-		//assertThat(code, containsString("return Integer.toHexString(i);"));
 		assertThat(code, containsString("return i > 128 ? arg0.toString() + ret.toString() : Integer.valueOf(i);"));
 		assertThat(code, containsString("return arg0 + 2;"));
 		assertThat(code, containsString("arg0 -= 951;"));

jadx-core/src/test/java/jadx/tests/internal/loops/TestBreakInLoop.java

+package jadx.tests.internal.loops;
+
+import jadx.api.InternalJadxTest;
+import jadx.core.dex.nodes.ClassNode;
+
+import org.junit.Test;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+public class TestBreakInLoop extends InternalJadxTest {
+
+	public static class TestCls {
+		private int f;
+
+		private void test(int[] a, int b) {
+			int i = 0;
+			while (i < a.length) {
+				a[i]++;
+				if (i < b) {
+					break;
+				}
+				i++;
+			}
+			this.f++;
+		}
+	}
+
+	@Test
+	public void test() {
+		ClassNode cls = getClassNode(TestCls.class);
+		String code = cls.getCode().toString();
+		System.out.println(code);
+
+		assertEquals(count(code, "this.f++;"), 1);
+		assertThat(code, containsString("if (i < b) {"));
+		assertThat(code, containsString("break;"));
+	}
+}

jadx-core/src/test/java/jadx/tests/internal/loops/TestLoopCondition.java

@@ -10,8 +10,7 @@ import static org.junit.Assert.assertThat;
 
 public class TestLoopCondition extends InternalJadxTest {
 
-	@SuppressWarnings("serial")
-	public static class TestCls extends Exception {
+	public static class TestCls {
 		public String f;
 
 		private void setEnabled(boolean r1z) {
@@ -32,14 +31,6 @@ public class TestLoopCondition extends InternalJadxTest {
 			setEnabled(false);
 		}
 
-		public int testComplexIfInLoop(boolean a) {
-			int i = 0;
-			while (a && i < 10) {
-				i++;
-			}
-			return i;
-		}
-
 		private void testMoreComplexIfInLoop(java.util.ArrayList<String> list) throws Exception {
 			for (int i = 0; i != 16 && i < 255; i++) {
 				list.set(i, "ABC");
@@ -55,9 +46,9 @@ public class TestLoopCondition extends InternalJadxTest {
 	public void test() {
 		ClassNode cls = getClassNode(TestCls.class);
 		String code = cls.getCode().toString();
+		System.out.println(code);
 
 		assertThat(code, containsString("i < this.f.length()"));
-		assertThat(code, containsString("while (a && i < 10) {"));
 		assertThat(code, containsString("list.set(i, \"ABC\")"));
 		assertThat(code, containsString("list.set(i, \"DEF\")"));
 	}

jadx-core/src/test/java/jadx/tests/internal/loops/TestLoopCondition2.java

+package jadx.tests.internal.loops;
+
+import jadx.api.InternalJadxTest;
+import jadx.core.dex.nodes.ClassNode;
+
+import org.junit.Test;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertThat;
+
+public class TestLoopCondition2 extends InternalJadxTest {
+
+	public static class TestCls {
+
+		public int test(boolean a) {
+			int i = 0;
+			while (a && i < 10) {
+				i++;
+			}
+			return i;
+		}
+	}
+
+	@Test
+	public void test() {
+		ClassNode cls = getClassNode(TestCls.class);
+		String code = cls.getCode().toString();
+		System.out.println(code);
+
+		assertThat(code, containsString("while (a && i < 10) {"));
+	}
+}

jadx-samples/src/main/java/jadx/samples/TestCF3.java

@@ -222,15 +222,13 @@ public class TestCF3 extends AbstractTest {
 				new ArrayList<String>(Arrays.asList("a1", "b2"))));
 
 		List<String> list1 = Arrays.asList(null, "a", "b");
-
-		// TODO this line required to omit generic information because it create List<Object>
-		// List<String> list2 = Arrays.asList(null, null, null);
-
 		assertEquals(testReturnInLoop(list1), "a");
 		assertEquals(testReturnInLoop2(list1), "a");
 
-		// assertEquals(testReturnInLoop(list2), "error");
-		// assertEquals(testReturnInLoop2(list2), "error");
+		// TODO this line required to omit generic information because it create List<Object>
+//		List<String> list2 = Arrays.asList(null, null, null);
+//		assertEquals(testReturnInLoop(list2), "error");
+//		assertEquals(testReturnInLoop2(list2), "error");
 
 		// assertTrue(testLabeledBreakContinue());