Fix #1242: Discard unreachable code.

Unreachable code is not part of the dominator tree, which most of our transforms are based on.

In particular, dominance-based loop detection runs into the problem where unreachable code might have jumps into two independent loops. In that case, it's impossible to place the unreachable code in a way that avoids assertions / generating invalid C#.

We establish the invariant that all blocks in a BlockContainer must be statically reachable from the entry point (-> every block is part of the dominator tree). This means transforms no longer have to deal with special cases for unreachable code.

The "Remove dead and side effect free code" option still has an effect on dead stores, but unreachable code is now always removed (previously this also was dependent on this option).

ICSharpCode.Decompiler.Tests/TestCases/ILPretty/Issue1047.cs

@@ -6,12 +6,8 @@
 
 		private void ProblemMethod()
 		{
-			IL_0000:
 			while (!dummy) {
 			}
-			return;
-			IL_0014:
-			goto IL_0000;
 		}
 	}
 }

ICSharpCode.Decompiler/IL/ControlFlow/ControlFlowSimplification.cs

@@ -158,9 +158,6 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				}
 				// Remove return blocks that are no longer reachable:
 				container.Blocks.RemoveAll(b => b.IncomingEdgeCount == 0 && b.Instructions.Count == 0);
-				if (context.Settings.RemoveDeadCode) {
-					container.SortBlocks(deleteUnreachableBlocks: true);
-				}
 			}
 		}
 

ICSharpCode.Decompiler/IL/ControlFlow/LoopDetection.cs

@@ -107,7 +107,6 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				IncludeNestedContainers(loop);
 				// Try to extend the loop to reduce the number of exit points:
 				ExtendLoop(h, loop, out var exitPoint);
-				IncludeUnreachablePredecessors(loop);
 
 				// Sort blocks in the loop in reverse post-order to make the output look a bit nicer.
 				// (if the loop doesn't contain nested loops, this is a topological sort)
@@ -115,7 +114,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				Debug.Assert(loop[0] == h);
 				foreach (var node in loop) {
 					node.Visited = false; // reset visited flag so that we can find outer loops
-					Debug.Assert(h.Dominates(node) || !node.IsReachable, "The loop body must be dominated by the loop head");
+					Debug.Assert(h.Dominates(node), "The loop body must be dominated by the loop head");
 				}
 				ConstructLoop(loop, exitPoint);
 			}
@@ -150,7 +149,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 					// (the entry-point itself doesn't have a CFG node, because it's newly created by this transform)
 					for (int i = 1; i < nestedContainer.Blocks.Count; i++) {
 						var node = context.ControlFlowGraph.GetNode(nestedContainer.Blocks[i]);
-						Debug.Assert(loop[0].Dominates(node) || !node.IsReachable);
+						Debug.Assert(loop[0].Dominates(node));
 						if (!node.Visited) {
 							node.Visited = true;
 							loop.Add(node);
@@ -602,30 +601,6 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 		}
 		#endregion
 
-		/// <summary>
-		/// While our normal dominance logic ensures the loop has just a single reachable entry point,
-		/// it's possible that there are unreachable code blocks that have jumps into the loop.
-		/// We'll also include those into the loop.
-		/// 
-		/// Requires and maintains the invariant that a node is marked as visited iff it is contained in the loop.
-		/// </summary>
-		private void IncludeUnreachablePredecessors(List<ControlFlowNode> loop)
-		{
-			for (int i = 1; i < loop.Count; i++) {
-				Debug.Assert(loop[i].Visited);
-				foreach (var pred in loop[i].Predecessors) {
-					if (!pred.Visited) {
-						if (pred.IsReachable) {
-							Debug.Fail("All jumps into the loop body should go through the entry point");
-						} else {
-							pred.Visited = true;
-							loop.Add(pred);
-						}
-					}
-				}
-			}
-		}
-
 		/// <summary>
 		/// Move the blocks associated with the loop into a new block container.
 		/// </summary>
@@ -708,7 +683,6 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 				}
 				exitPoint = null;
 			}
-			IncludeUnreachablePredecessors(nodesInSwitch);
 
 			context.Step("Create BlockContainer for switch", switchInst);
 			// Sort blocks in the loop in reverse post-order to make the output look a bit nicer.
@@ -717,7 +691,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 			Debug.Assert(nodesInSwitch[0] == h);
 			foreach (var node in nodesInSwitch) {
 				node.Visited = false; // reset visited flag so that we can find outer loops
-				Debug.Assert(h.Dominates(node) || !node.IsReachable, "The switch body must be dominated by the switch head");
+				Debug.Assert(h.Dominates(node), "The switch body must be dominated by the switch head");
 			}
 
 			BlockContainer switchContainer = new BlockContainer(ContainerKind.Switch);

ICSharpCode.Decompiler/IL/ControlFlow/YieldReturnDecompiler.cs

@@ -142,7 +142,6 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 			function.Body = newBody;
 			// register any locals used in newBody
 			function.Variables.AddRange(newBody.Descendants.OfType<IInstructionWithVariableOperand>().Select(inst => inst.Variable).Distinct());
-			function.CheckInvariant(ILPhase.Normal);
 
 			PrintFinallyMethodStateRanges(newBody);
 
@@ -164,6 +163,7 @@ namespace ICSharpCode.Decompiler.IL.ControlFlow
 			// Note: because this only deletes blocks outright, the 'stateChanges' entries remain valid
 			// (though some may point to now-deleted blocks)
 			newBody.SortBlocks(deleteUnreachableBlocks: true);
+			function.CheckInvariant(ILPhase.Normal);
 
 			if (!isCompiledWithMono) {
 				DecompileFinallyBlocks();

ICSharpCode.Decompiler/IL/ILReader.cs

@@ -493,8 +493,18 @@ namespace ICSharpCode.Decompiler.IL
 			CollectionExtensions.AddRange(function.Variables, stackVariables);
 			CollectionExtensions.AddRange(function.Variables, variableByExceptionHandler.Values);
 			function.AddRef(); // mark the root node
+			var removedBlocks = new List<Block>();
 			foreach (var c in function.Descendants.OfType<BlockContainer>()) {
-				c.SortBlocks();
+				var newOrder = c.TopologicalSort(deleteUnreachableBlocks: true);
+				if (newOrder.Count < c.Blocks.Count) {
+					removedBlocks.AddRange(c.Blocks.Except(newOrder));
+				}
+				c.Blocks.ReplaceList(newOrder);
+			}
+			if (removedBlocks.Count > 0) {
+				removedBlocks.SortBy(b => b.StartILOffset);
+				function.Warnings.Add("Discarded unreachable code: "
+							+ string.Join(", ", removedBlocks.Select(b => $"IL_{b.StartILOffset:x4}")));
 			}
 			function.Warnings.AddRange(Warnings);
 			return function;

ICSharpCode.Decompiler/IL/Instructions/BlockContainer.cs

@@ -184,6 +184,7 @@ namespace ICSharpCode.Decompiler.IL
 			Debug.Assert(EntryPoint == null || Parent is ILFunction || !HasILRange);
 			Debug.Assert(Blocks.All(b => b.HasFlag(InstructionFlags.EndPointUnreachable)));
 			Debug.Assert(Blocks.All(b => b.Kind == BlockKind.ControlFlow)); // this also implies that the blocks don't use FinalInstruction
+			Debug.Assert(TopologicalSort(deleteUnreachableBlocks: true).Count == Blocks.Count, "Container should not have any unreachable blocks");
 			Block bodyStartBlock;
 			switch (Kind) {
 				case ContainerKind.Normal:
@@ -237,45 +238,56 @@ namespace ICSharpCode.Decompiler.IL
 				return InstructionFlags.ControlFlow;
 			}
 		}
-		
+
 		/// <summary>
-		/// Sort the blocks in reverse post-order over the control flow graph between the blocks.
+		/// Topologically sort the blocks.
+		/// The new order is returned without modifying the BlockContainer.
 		/// </summary>
-		public void SortBlocks(bool deleteUnreachableBlocks = false)
+		/// <param name="deleteUnreachableBlocks">If true, unreachable blocks are not included in the new order.</param>
+		public List<Block> TopologicalSort(bool deleteUnreachableBlocks = false)
 		{
-			if (Blocks.Count < 2)
-				return;
-
 			// Visit blocks in post-order
 			BitSet visited = new BitSet(Blocks.Count);
 			List<Block> postOrder = new List<Block>();
-			
-			Action<Block> visit = null;
-			visit = delegate(Block block) {
+			Visit(EntryPoint);
+			postOrder.Reverse();
+			if (!deleteUnreachableBlocks) {
+				for (int i = 0; i < Blocks.Count; i++) {
+					if (!visited[i])
+						postOrder.Add(Blocks[i]);
+				}
+			}
+			return postOrder;
+
+			void Visit(Block block)
+			{
 				Debug.Assert(block.Parent == this);
 				if (!visited[block.ChildIndex]) {
 					visited[block.ChildIndex] = true;
 
 					foreach (var branch in block.Descendants.OfType<Branch>()) {
 						if (branch.TargetBlock.Parent == this) {
-							visit(branch.TargetBlock);
+							Visit(branch.TargetBlock);
 						}
 					}
 
 					postOrder.Add(block);
 				}
 			};
-			visit(EntryPoint);
-			
-			postOrder.Reverse();
-			if (!deleteUnreachableBlocks) {
-				for (int i = 0; i < Blocks.Count; i++) {
-					if (!visited[i])
-						postOrder.Add(Blocks[i]);
-				}
-			}
-			Debug.Assert(postOrder[0] == Blocks[0]);
-			Blocks.ReplaceList(postOrder);
+		}
+
+		/// <summary>
+		/// Topologically sort the blocks.
+		/// </summary>
+		/// <param name="deleteUnreachableBlocks">If true, delete unreachable blocks.</param>
+		public void SortBlocks(bool deleteUnreachableBlocks = false)
+		{
+			if (Blocks.Count < 2)
+				return;
+
+			var newOrder = TopologicalSort(deleteUnreachableBlocks);
+			Debug.Assert(newOrder[0] == Blocks[0]);
+			Blocks.ReplaceList(newOrder);
 		}
 
 		public static BlockContainer FindClosestContainer(ILInstruction inst)

ICSharpCode.Decompiler/IL/Transforms/BlockTransform.cs

@@ -85,7 +85,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 					context.CancellationToken.ThrowIfCancellationRequested();
 					blockContext.ControlFlowGraph = new ControlFlowGraph(container, context.CancellationToken);
 					VisitBlock(blockContext.ControlFlowGraph.GetNode(container.EntryPoint), blockContext);
-					// TODO: handle unreachable code?
 				}
 			} finally {
 				running = false;