Fix branch removal in compiler generated code

Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking

This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.

To make the code simple I added a hashset to prevent multiple optimization runs on the same method. Technically this can be guaranteed by the caller, but it's really complex and very fragile.

Added tests.

Note that there's still a whole in analysis of compiler generated code around state machines, see dotnet#3087

src/linker/Linker.Steps/MarkStep.cs

@@ -3543,6 +3543,10 @@ bool MarkAndCheckRequiresReflectionMethodBodyScanner (MethodBody body)
 			if (_compilerGeneratedMethodRequiresScanner.TryGetValue (body, out bool requiresReflectionMethodBodyScanner))
 				return requiresReflectionMethodBodyScanner;
 
+			// Make sure the method's body is processed, for compiler generated code we can get here before ProcessMethod is called
+			// and thus we need to make sure we operate on the optimized method body (to avoid marking code which is otherwise optimized away).
+			UnreachableBlocksOptimizer.ProcessMethod (body.Method);
+
 			foreach (VariableDefinition var in body.Variables)
 				MarkType (var.VariableType, new DependencyInfo (DependencyKind.VariableType, body.Method));
 

src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

@@ -24,6 +24,7 @@ public class UnreachableBlocksOptimizer
 		readonly LinkContext _context;
 		readonly Dictionary<MethodDefinition, MethodResult?> _cache_method_results = new (2048);
 		readonly Stack<MethodDefinition> _resursion_guard = new ();
+		readonly HashSet<MethodDefinition> _processed_methods = new (2048);
 
 		MethodDefinition? IntPtrSize, UIntPtrSize;
 
@@ -39,6 +40,9 @@ public UnreachableBlocksOptimizer (LinkContext context)
 		/// <param name="method">The method to process</param>
 		public void ProcessMethod (MethodDefinition method)
 		{
+			if (!_processed_methods.Add (method))
+				return;
+
 			if (!IsMethodSupported (method))
 				return;
 

test/Mono.Linker.Tests.Cases/UnreachableBlock/CompilerGeneratedCodeSubstitutions.cs

@@ -0,0 +1,246 @@
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
+using System.Threading.Tasks;
+using Mono.Linker.Tests.Cases.Expectations.Assertions;
+using Mono.Linker.Tests.Cases.Expectations.Metadata;
+
+namespace Mono.Linker.Tests.Cases.UnreachableBlock
+{
+	[SetupLinkerArgument ("--enable-opt", "ipconstprop")]
+
+	// Using Kept validation on compiler generated code is tricky as we would have to describe
+	// all of the compiler generated classes and members which are expected to be kept.
+	// So not using that here (at least until we come up with a better way to do this).
+	// Instead this test relies on RUC and warnings to detect "kept" and "removed" calls.
+	[SkipKeptItemsValidation]
+	[ExpectedNoWarnings]
+	class CompilerGeneratedCodeSubstitutions
+	{
+		static void Main ()
+		{
+			Lambda.Test ();
+			LocalFunction.Test ();
+			Iterator.Test ();
+			Async.Test ();
+		}
+
+		class Lambda
+		{
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchInLambda ()
+			{
+				var a = () => {
+					if (AlwaysFalse) {
+						RemovedMethod ();
+					} else {
+						UsedMethod ();
+					}
+				};
+
+				a ();
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchAroundLambda ()
+			{
+				Action a;
+				if (AlwaysFalse) {
+					a = () => RemovedMethod ();
+				}
+				else {
+					a = () => UsedMethod ();
+				}
+
+				a ();
+			}
+
+			public static void Test ()
+			{
+				TestBranchInLambda ();
+				TestBranchAroundLambda ();
+			}
+		}
+
+		class LocalFunction
+		{
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchInLocalFunction ()
+			{
+				void LocalFunction ()
+				{
+					if (AlwaysFalse) {
+						RemovedMethod ();
+					} else {
+						UsedMethod ();
+					}
+				}
+
+				LocalFunction ();
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchAroundLocalFunction ()
+			{
+				Action a;
+				if (AlwaysFalse) {
+					void RemovedLocalFunction ()
+					{
+						RemovedMethod ();
+					}
+
+					RemovedLocalFunction ();
+				} else {
+					void UsedLocalFunction ()
+					{
+						UsedMethod ();
+					}
+
+					UsedLocalFunction ();
+				}
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static void TestBranchAroundUsageOfLocalFunction ()
+			{
+				Action a;
+				if (AlwaysFalse) {
+					RemovedLocalFunction ();
+				} else {
+					UsedLocalFunction ();
+				}
+
+				void RemovedLocalFunction ()
+				{
+					RemovedMethod ();
+				}
+
+				void UsedLocalFunction ()
+				{
+					UsedMethod ();
+				}
+			}
+
+			public static void Test ()
+			{
+				TestBranchInLocalFunction ();
+				TestBranchAroundLocalFunction ();
+				TestBranchAroundUsageOfLocalFunction ();
+			}
+		}
+
+		class Iterator
+		{
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static IEnumerable<int> TestBranchWithNormalCall ()
+			{
+				if (AlwaysFalse) {
+					RemovedMethod ();
+				} else {
+					UsedMethod ();
+				}
+
+				yield return 1;
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static IEnumerable<int> TestBranchWithYieldAfter ()
+			{
+				if (AlwaysFalse) {
+					RemovedMethod ();
+					yield return 1;
+				} else {
+					UsedMethod ();
+					yield return 1;
+				}
+
+				yield return 1;
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			// https://github.com/dotnet/linker/issues/3087
+			[ExpectedWarning ("IL2026", "--RemovedMethod--", CompilerGeneratedCode = true)]
+			static IEnumerable<int> TestBranchWithYieldBefore ()
+			{
+				if (AlwaysFalse) {
+					yield return 1;
+					RemovedMethod ();
+				} else {
+					yield return 1;
+					UsedMethod ();
+				}
+
+				yield return 1;
+			}
+
+			public static void Test ()
+			{
+				TestBranchWithNormalCall ();
+				TestBranchWithYieldAfter ();
+				TestBranchWithYieldBefore ();
+			}
+		}
+
+		class Async
+		{
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			static async Task TestBranchWithNormalCall ()
+			{
+				if (AlwaysFalse) {
+					RemovedMethod ();
+				} else {
+					UsedMethod ();
+				}
+
+				await Task.FromResult (0);
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedMethod--", CompilerGeneratedCode = true)]
+			// https://github.com/dotnet/linker/issues/3087
+			[ExpectedWarning ("IL2026", "--RemovedMethod--", CompilerGeneratedCode = true)]
+			static async Task TestBranchWithNormalCallAfterWAwait ()
+			{
+				if (AlwaysFalse) {
+					await Task.FromResult (0);
+					RemovedMethod ();
+				} else {
+					await Task.FromResult (0);
+					UsedMethod ();
+				}
+
+				await Task.FromResult (0);
+			}
+
+			[ExpectedWarning ("IL2026", "--UsedAsyncMethod--", CompilerGeneratedCode = true)]
+			static async Task TestBranchWithAsyncCall ()
+			{
+				if (AlwaysFalse) {
+					await RemovedAsyncMethod ();
+				} else {
+					await UsedAsyncMethod ();
+				}
+			}
+
+			public static void Test ()
+			{
+				TestBranchWithNormalCall ().RunSynchronously (); ;
+				TestBranchWithNormalCallAfterWAwait ().RunSynchronously ();
+				TestBranchWithAsyncCall ().RunSynchronously ();
+			}
+		}
+
+		static bool AlwaysFalse => false;
+
+		[RequiresUnreferencedCode ("--UsedAsyncMethod--")]
+		static async Task UsedAsyncMethod () => await Task.FromResult (0);
+
+		[RequiresUnreferencedCode ("--RemovedAsyncMethod--")]
+		static async Task RemovedAsyncMethod () => await Task.FromResult (-1);
+
+		[RequiresUnreferencedCode ("--UsedMethod--")]
+		static void UsedMethod () { }
+
+		[RequiresUnreferencedCode ("--RemovedMethod--")]
+		static void RemovedMethod () { }
+	}
+}