Deob control flow housekeeping

deobfuscator/deobfuscator.gradle.kts

@@ -103,6 +103,7 @@ tasks {
 
         classpath = project.sourceSets.main.get().runtimeClasspath
         main = "net.runelite.deob.Deob"
+        args = listOf(tokens["vanilla.jar"], "$buildDir/libs/deobfuscated-$version.jar")
     }
 
     register<JavaExec>("UpdateMappings.main()") {

deobfuscator/src/main/java/net/runelite/asm/execution/Execution.java

@@ -204,6 +204,18 @@ public class Execution
 		this.addFrame(f);
 	}
 
+	public void addMethods(Iterable<Method> methods)
+	{
+		for (Method m : methods)
+		{
+			if (m.getCode() == null)
+				continue;
+			Frame f = new Frame(this, m);
+			f.initialize();
+			this.addFrame(f);
+		}
+	}
+
 	public void run()
 	{
 		assert !paused;

deobfuscator/src/main/java/net/runelite/deob/deobfuscators/cfg/Block.java

@@ -27,6 +27,7 @@ package net.runelite.deob.deobfuscators.cfg;
 import java.util.ArrayList;
 import java.util.List;
 import net.runelite.asm.attributes.code.Instruction;
+import net.runelite.asm.attributes.code.Label;
 
 public class Block
 {
@@ -36,22 +37,22 @@ public class Block
 	/**
 	 * blocks which jump here
 	 */
-	private final List<Block> prev = new ArrayList<>();
+	private final List<Block> pred = new ArrayList<>();
 
 	/**
 	 * blocks which this jumps to
 	 */
-	private final List<Block> next = new ArrayList<>();
+	private final List<Block> succ = new ArrayList<>();
 
 	/**
 	 * block which flows directly into this block
 	 */
-	private Block flowsFrom;
+	private Block from;
 
 	/**
 	 * block which this directly flows into
 	 */
-	private Block flowsInto;
+	private Block into;
 
 	/**
 	 * instructions in this block
@@ -83,17 +84,17 @@ public class Block
 	{
 		StringBuilder sb = new StringBuilder();
 		sb.append("Block ID ").append(id).append("\n");
-		if (flowsFrom != null)
+		if (from != null)
 		{
-			sb.append(" flows from ").append(flowsFrom.id).append("\n");
+			sb.append(" flows from ").append(from.id).append("\n");
 		}
 		for (Instruction i : instructions)
 		{
 			sb.append("  ").append(i.toString()).append("\n");
 		}
-		if (flowsInto != null)
+		if (into != null)
 		{
-			sb.append(" flows into ").append(flowsInto.id).append("\n");
+			sb.append(" flows into ").append(into.id).append("\n");
 		}
 		sb.append("\n");
 		return sb.toString();
@@ -118,47 +119,65 @@ public class Block
 
 	public void addPrev(Block block)
 	{
-		if (!prev.contains(block))
+		if (!pred.contains(block))
 		{
-			prev.add(block);
+			pred.add(block);
 		}
 	}
 
-	public List<Block> getPrev()
+	public List<Block> getPred()
 	{
-		return prev;
+		return pred;
 	}
 
 	public void addNext(Block block)
 	{
-		if (!next.contains(block))
+		if (!succ.contains(block))
 		{
-			next.add(block);
+			succ.add(block);
 		}
 	}
 
-	public List<Block> getNext()
+	public List<Block> getSucc()
 	{
-		return next;
+		return succ;
 	}
 
-	public Block getFlowsFrom()
+	public Block getFrom()
 	{
-		return flowsFrom;
+		return from;
 	}
 
-	public void setFlowsFrom(Block flowsFrom)
+	public void setFrom(Block from)
 	{
-		this.flowsFrom = flowsFrom;
+		this.from = from;
 	}
 
-	public Block getFlowsInto()
+	public Block getInto()
 	{
-		return flowsInto;
+		return into;
 	}
 
-	public void setFlowsInto(Block flowsInto)
+	public void setInto(Block into)
 	{
-		this.flowsInto = flowsInto;
+		this.into = into;
+	}
+
+	static int compare(Block a, Block b)
+	{
+		final int l1 = a.getLineNumber();
+		final int l2 = b.getLineNumber();
+		if (l1 == l2 || l1 == -1 || l2 == -1)
+			return 0;
+		return Integer.compare(l1, l2);
+	}
+
+	private int getLineNumber()
+	{
+		for (Instruction i : instructions)
+			if (i instanceof Label)
+				if (((Label) i).getLineNumber() != null)
+					return ((Label) i).getLineNumber();
+		return -1;
 	}
 }

deobfuscator/src/main/java/net/runelite/deob/deobfuscators/cfg/ControlFlowDeobfuscator.java

@@ -24,16 +24,11 @@
  */
 package net.runelite.deob.deobfuscators.cfg;
 
-import java.util.ArrayList;
 import java.util.List;
-import java.util.PriorityQueue;
-import java.util.Queue;
 import net.runelite.asm.ClassFile;
 import net.runelite.asm.ClassGroup;
 import net.runelite.asm.Method;
 import net.runelite.asm.attributes.Code;
-import net.runelite.asm.attributes.code.Exception;
-import net.runelite.asm.attributes.code.Exceptions;
 import net.runelite.asm.attributes.code.Instruction;
 import net.runelite.asm.attributes.code.Instructions;
 import net.runelite.asm.attributes.code.Label;
@@ -64,7 +59,6 @@ public class ControlFlowDeobfuscator implements Deobfuscator
 					continue;
 				}
 
-				split(code);
 				run(code);
 				runJumpLabel(code);
 			}
@@ -74,167 +68,49 @@ public class ControlFlowDeobfuscator implements Deobfuscator
 			insertedJump, placedBlocks, removedJumps, insertedJump - removedJumps);
 	}
 
-	/**
-	 * Add gotos at the end of blocks without terminal instructions
-	 *
-	 * @param code
-	 */
-	private void split(Code code)
-	{
-		Instructions ins = code.getInstructions();
-		Exceptions exceptions = code.getExceptions();
-
-		ControlFlowGraph graph = new ControlFlowGraph.Builder().build(code);
-
-		List<Exception> exc = new ArrayList<>(exceptions.getExceptions());
-
-		exceptions.clear(); // Must clear this before ins.clear() runs
-		ins.clear();
-
-		// insert jumps where blocks flow into others
-		for (Block block : graph.getBlocks())
-		{
-			if (block.getFlowsInto() == null)
-			{
-				continue;
-			}
-
-			Block into = block.getFlowsInto();
-			assert into.getFlowsFrom() == block;
-
-			Instruction first = into.getInstructions().get(0);
-			Label label;
-			if (!(first instanceof Label))
-			{
-				label = new Label(null);
-				into.addInstruction(0, label);
-			}
-			else
-			{
-				label = (Label) first;
-			}
-
-			Goto g = new Goto(null, label);
-			block.addInstruction(g);
-
-			block.setFlowsInto(null);
-			into.setFlowsFrom(null);
-
-			++insertedJump;
-		}
-
-		// Readd instructions from modified blocks
-		for (Block block : graph.getBlocks())
-		{
-			for (Instruction i : block.getInstructions())
-			{
-				assert i.getInstructions() == null;
-				i.setInstructions(ins); // I shouldn't have to do this here
-				ins.addInstruction(i);
-			}
-		}
-
-		// Readd exceptions
-		for (Exception ex : exc)
-		{
-			exceptions.add(ex);
-		}
-	}
-
-	private int compareBlock(Block o1, Block o2)
-	{
-		// higher numbers have the lowest priority
-		if (o1.isJumptarget() && !o2.isJumptarget())
-		{
-			return -1;
-		}
-		if (o2.isJumptarget() && !o1.isJumptarget())
-		{
-			return 1;
-		}
-
-		return 0;
-	}
-
 	private void run(Code code)
 	{
 		Instructions ins = code.getInstructions();
-		Exceptions exceptions = code.getExceptions();
 
-		ControlFlowGraph graph = new ControlFlowGraph.Builder().build(code);
+		ControlFlowGraph graph = new ControlFlowGraph(code);
-
-		for (Block block : graph.getBlocks())
-		{
-			assert block.getFlowsFrom() == null;
-			assert block.getFlowsInto() == null;
-		}
 
 		if (logger.isDebugEnabled()) // graph.toString() is expensive
 		{
 			logger.debug(graph.toString());
 		}
 
-		List<Exception> originalExceptions = new ArrayList<>(exceptions.getExceptions());
+		// Clear existing instructions as we are going to rebuild them
-
-		// Clear existing exceptions and instructions as we are going to
-		// rebuild them
-		exceptions.clear();
 		ins.clear();
-
+		final List<Block> sorted = graph.topologicalSort();
-		List<Block> done = new ArrayList<>();
+		for (Block b : sorted)
-		Queue<Block> queue = new PriorityQueue<>(this::compareBlock);
-
-		// add initial code block
-		queue.add(graph.getHead());
-
-		while (!queue.isEmpty())
 		{
-			Block block = queue.remove();
-
-			if (done.contains(block))
-			{
-				continue;
-			}
-
-			done.add(block);
 			++placedBlocks;
-
+			for (Instruction i : b.getInstructions())
-			logger.debug("Placed block {}", block.getId());
-
-			List<Block> next = block.getNext();
-
-			if (next.isEmpty() == false)
 			{
-				// jumps are added in order their instructions are reached by ControlFlowGraph,
-				// so the last jump is the goto.
-				//
-				// removing this line causes the priority queue (due to implementation detail on how
-				// it handles objects with equal priority) to try to optimize for block closeness
-				// (how close blocks which are neighbors are to each other in bytecode).
-				// I get a jump delta of ~+14k with this on 143, vs ~-47k when priotiziing optimizing
-				// out jumps. I can't tell which is better.
-				next.get(next.size() - 1).setJumptarget(true);
-			}
-
-			// add next reachable blocks
-			for (Block bl : next)
-			{
-				queue.add(bl);
-			}
-
-			for (Instruction i : block.getInstructions())
-			{
-				assert i.getInstructions() == null;
-				i.setInstructions(ins); // I shouldn't have to do this here
 				ins.addInstruction(i);
+				i.setInstructions(ins);
+			}
+			if (b.getInto() != null && b.getInstructions().size() > 0)
+			{
+				final var i = b.getInstructions().get(b.getInstructions().size() - 1);
+				if (!i.isTerminal())
+				{
+					final var next = b.getInto();
+					var maybeLabel = next.getInstructions().get(0);
+					if (!(maybeLabel instanceof Label))
+					{
+						maybeLabel = new Label(ins);
+						next.getInstructions().add(0, maybeLabel);
+					}
+					ins.addInstruction(new Goto(ins, (Label) maybeLabel));
+					++insertedJump;
+				}
 			}
 		}
 	}
 
 	/**
 	 * remove jumps followed immediately by the label they are jumping to
-	 *
-	 * @param code
 	 */
 	private void runJumpLabel(Code code)
 	{

deobfuscator/src/main/java/net/runelite/deob/deobfuscators/cfg/ControlFlowGraph.java

@@ -26,9 +26,12 @@ package net.runelite.deob.deobfuscators.cfg;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import net.runelite.asm.attributes.Code;
 import net.runelite.asm.attributes.code.Instruction;
 import net.runelite.asm.attributes.code.Label;
@@ -36,10 +39,90 @@ import net.runelite.asm.attributes.code.instruction.types.JumpingInstruction;
 
 public class ControlFlowGraph
 {
-	private Map<Label, Block> blocks = new HashMap<>();
+	private final Map<Label, Block> blocks = new HashMap<>();
-	private List<Block> allBlocks = new ArrayList<>();
+	private final List<Block> allBlocks = new ArrayList<>();
 	private final Block head;
 
+	public ControlFlowGraph(Code code)
+	{
+		int id = 0;
+		this.head = new Block();
+		for (Instruction i : code.getInstructions())
+			if (i instanceof Label)
+				blocks.computeIfAbsent((Label) i, lbl -> {
+					var b = new Block();
+					allBlocks.add(b);
+					return b;
+				});
+
+		allBlocks.add(0, head);
+		Block cur = head;
+		for (Instruction i : code.getInstructions())
+		{
+			if (i instanceof Label)
+			{
+				Block next = blocks.get(i);
+				if (next.getId() == -1)
+				{
+					next.setId(id++);
+				}
+				if (next != cur)
+				{
+					Instruction last = cur.getInstructions().isEmpty()
+						? null
+						: cur.getInstructions().get(cur.getInstructions().size() - 1);
+					if (last == null || !last.isTerminal())
+					{
+						assert next.getFrom() == null;
+						assert cur.getInto() == null;
+						// previous block flows directly into next
+						next.setFrom(cur);
+						cur.setInto(next);
+					}
+					cur = next;
+				}
+			}
+			cur.addInstruction(i);
+			if (i instanceof JumpingInstruction)
+			{
+				JumpingInstruction ji = (JumpingInstruction) i;
+				for (Label l : ji.getJumps())
+				{
+					Block next = blocks.get(l);
+					if (next.getId() == -1)
+					{
+						next.setId(id++);
+					}
+					cur.addNext(next);
+					next.addPrev(cur);
+				}
+			}
+		}
+
+		assert head.getFrom() == null;
+	}
+
+	List<Block> topologicalSort()
+	{
+		final List<Block> ret = new ArrayList<>();
+		walk(head, ret, new HashSet<>());
+		Collections.reverse(ret);
+		return ret;
+	}
+
+	private static void walk(Block cur, List<Block> order, Set<Block> done)
+	{
+		Block dirsucc = cur.getInto();
+		if (dirsucc != null && done.add(dirsucc))
+			walk(cur.getInto(), order, done);
+		List<Block> succs = cur.getSucc();
+		succs.sort(Block::compare);
+		for (Block succ : succs)
+			if (done.add(succ))
+				walk(succ, order, done);
+		order.add(cur);
+	}
+
 	public ControlFlowGraph(Block head)
 	{
 		this.head = head;
@@ -70,91 +153,4 @@ public class ControlFlowGraph
 	{
 		return head;
 	}
-
-	public static class Builder
-	{
-		private final Map<Label, Block> blocks = new HashMap<>();
-		private final List<Block> allBlocks = new ArrayList<>();
-
-		public ControlFlowGraph build(Code code)
-		{
-			int id = 0;
-
-			Block head = new Block(),
-				cur = head;
-			allBlocks.add(head);
-
-			for (Instruction i : code.getInstructions().getInstructions())
-			{
-				if (i instanceof Label)
-				{
-					// blocks always begin at labels, so create initial blocks
-					Block block = new Block();
-					blocks.put((Label) i, block);
-					allBlocks.add(block);
-				}
-			}
-
-			for (Instruction i : code.getInstructions().getInstructions())
-			{
-				if (i instanceof Label)
-				{
-					Block next = blocks.get((Label) i);
-					assert next != null;
-
-					if (next.getId() == -1)
-					{
-						next.setId(id++);
-					}
-
-					if (next != cur)
-					{
-						Instruction last = cur.getInstructions().isEmpty()
-							? null
-							: cur.getInstructions().get(cur.getInstructions().size() - 1);
-
-						if (last == null || !last.isTerminal())
-						{
-							assert next.getFlowsFrom() == null;
-							assert cur.getFlowsInto() == null;
-
-							// previous block flows directly into next
-							next.setFlowsFrom(cur);
-							cur.setFlowsInto(next);
-						}
-
-						cur = next;
-					}
-				}
-
-				cur.addInstruction(i);
-
-				if (i instanceof JumpingInstruction)
-				{
-					JumpingInstruction ji = (JumpingInstruction) i;
-
-					for (Label l : ji.getJumps())
-					{
-						Block next = blocks.get(l);
-
-						if (next.getId() == -1)
-						{
-							next.setId(id++);
-						}
-
-						cur.addNext(next);
-						next.addPrev(cur);
-					}
-				}
-			}
-
-			assert head != null : "no instructions in code";
-			assert head.getFlowsFrom() == null;
-
-			ControlFlowGraph cfg = new ControlFlowGraph(head);
-			cfg.blocks = blocks;
-			cfg.allBlocks = allBlocks;
-			return cfg;
-		}
-	}
 }