[release/7.0] Fix branch removal in compiler generated code

src/linker/BannedSymbols.txt

@@ -1,2 +1,6 @@
 T:Mono.Cecil.Cil.ILProcessor;Use LinkerILProcessor instead
 M:Mono.Cecil.TypeReference.Resolve();Use LinkContext.Resolve and LinkContext.TryResolve helpers instead
+P:Mono.Cecil.Cil.MethodBody.Instructions;Use LinkContext.MethodBodyInstructionProvider instead
+P:Mono.Cecil.Cil.MethodBody.ExceptionHandlers;Use LinkContext.MethodBodyInstructionProvider instead
+P:Mono.Cecil.Cil.MethodBody.Variables;Use LinkContext.MethodBodyInstructionProvider instead
+M:Mono.Linker.Steps.ILProvider/MethodIL.Create;Use ILProvider GetMethodIL instead

src/linker/Linker.Dataflow/CompilerGeneratedState.cs

@@ -139,7 +139,7 @@ void ProcessMethod (MethodDefinition method)
 				// Discover calls or references to lambdas or local functions. This includes
 				// calls to local functions, and lambda assignments (which use ldftn).
 				if (method.Body != null) {
-					foreach (var instruction in method.Body.Instructions) {
+					foreach (var instruction in _context.GetMethodIL (method).Instructions) {
 						switch (instruction.OpCode.OperandType) {
 						case OperandType.InlineMethod: {
 								MethodDefinition? referencedMethod = _context.TryResolve ((MethodReference) instruction.Operand);
@@ -348,7 +348,7 @@ void MapGeneratedTypeTypeParameters (TypeDefinition generatedType)
 
 			GenericInstanceType? ScanForInit (TypeDefinition compilerGeneratedType, MethodBody body)
 			{
-				foreach (var instr in body.Instructions) {
+				foreach (var instr in _context.GetMethodIL (body).Instructions) {
 					bool handled = false;
 					switch (instr.OpCode.Code) {
 					case Code.Initobj:

src/linker/Linker.Dataflow/FlowAnnotations.cs

@@ -422,7 +422,7 @@ bool ScanMethodBodyForFieldAccess (MethodBody body, bool write, out FieldDefinit
 
 			FieldReference? foundReference = null;
 
-			foreach (Instruction instruction in body.Instructions) {
+			foreach (Instruction instruction in _context.GetMethodIL (body).Instructions) {
 				switch (instruction.OpCode.Code) {
 				case Code.Ldsfld when !write:
 				case Code.Ldfld when !write:

src/linker/Linker.Dataflow/InterproceduralState.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using ILLink.Shared;
 using ILLink.Shared.DataFlow;
 using Mono.Cecil;
 using Mono.Cecil.Cil;
@@ -14,23 +15,25 @@
 
 namespace Mono.Linker.Dataflow
 {
-	// Wrapper that implements IEquatable for MethodBody.
-	readonly record struct MethodBodyValue (MethodBody MethodBody);
-
 	// Tracks the set of methods which get analyzer together during interprocedural analysis,
 	// and the possible states of hoisted locals in state machine methods and lambdas/local functions.
 	struct InterproceduralState : IEquatable<InterproceduralState>
 	{
-		public ValueSet<MethodBodyValue> MethodBodies;
+		public ValueSet<MethodIL> MethodBodies;
 		public HoistedLocalState HoistedLocals;
 		readonly InterproceduralStateLattice lattice;
 
-		public InterproceduralState (ValueSet<MethodBodyValue> methodBodies, HoistedLocalState hoistedLocals, InterproceduralStateLattice lattice)
+		public InterproceduralState (ValueSet<MethodIL> methodBodies, HoistedLocalState hoistedLocals, InterproceduralStateLattice lattice)
 			=> (MethodBodies, HoistedLocals, this.lattice) = (methodBodies, hoistedLocals, lattice);
 
 		public bool Equals (InterproceduralState other)
 			=> MethodBodies.Equals (other.MethodBodies) && HoistedLocals.Equals (other.HoistedLocals);
 
+		public override bool Equals (object? obj)
+			=> obj is InterproceduralState state && Equals (state);
+
+		public override int GetHashCode () => HashUtils.Combine(MethodBodies.GetHashCode (), HoistedLocals.GetHashCode ());
+
 		public InterproceduralState Clone ()
 			=> new (MethodBodies.Clone (), HoistedLocals.Clone (), lattice);
 
@@ -43,23 +46,28 @@ public void TrackMethod (MethodDefinition method)
 		}
 
 		public void TrackMethod (MethodBody methodBody)
+		{
+			TrackMethod (lattice.Context.GetMethodIL (methodBody));
+		}
+
+		public void TrackMethod (MethodIL methodIL)
 		{
 			// Work around the fact that ValueSet is readonly
-			var methodsList = new List<MethodBodyValue> (MethodBodies);
-			methodsList.Add (new MethodBodyValue (methodBody));
+			var methodsList = new List<MethodIL> (MethodBodies);
+			methodsList.Add (methodIL);
 
 			// For state machine methods, also scan the state machine members.
 			// Simplification: assume that all generated methods of the state machine type are
 			// reached at the point where the state machine method is reached.
-			if (CompilerGeneratedState.TryGetStateMachineType (methodBody.Method, out TypeDefinition? stateMachineType)) {
+			if (CompilerGeneratedState.TryGetStateMachineType (methodIL.Method, out TypeDefinition? stateMachineType)) {
 				foreach (var stateMachineMethod in stateMachineType.Methods) {
 					Debug.Assert (!CompilerGeneratedNames.IsLambdaOrLocalFunction (stateMachineMethod.Name));
 					if (stateMachineMethod.Body is MethodBody stateMachineMethodBody)
-						methodsList.Add (new MethodBodyValue (stateMachineMethodBody));
+						methodsList.Add (lattice.Context.GetMethodIL (stateMachineMethodBody));
 				}
 			}
 
-			MethodBodies = new ValueSet<MethodBodyValue> (methodsList);
+			MethodBodies = new ValueSet<MethodIL> (methodsList);
 		}
 
 		public void SetHoistedLocal (HoistedLocalKey key, MultiValue value)
@@ -76,15 +84,17 @@ public MultiValue GetHoistedLocal (HoistedLocalKey key)
 			=> HoistedLocals.Get (key);
 	}
 
-	struct InterproceduralStateLattice : ILattice<InterproceduralState>
+	readonly struct InterproceduralStateLattice : ILattice<InterproceduralState>
 	{
-		public readonly ValueSetLattice<MethodBodyValue> MethodBodyLattice;
+		public readonly ValueSetLattice<MethodIL> MethodBodyLattice;
 		public readonly DictionaryLattice<HoistedLocalKey, MultiValue, ValueSetLattice<SingleValue>> HoistedLocalsLattice;
+		public readonly LinkContext Context;
 
 		public InterproceduralStateLattice (
-			ValueSetLattice<MethodBodyValue> methodBodyLattice,
-			DictionaryLattice<HoistedLocalKey, MultiValue, ValueSetLattice<SingleValue>> hoistedLocalsLattice)
-			=> (MethodBodyLattice, HoistedLocalsLattice) = (methodBodyLattice, hoistedLocalsLattice);
+			ValueSetLattice<MethodIL> methodBodyLattice,
+			DictionaryLattice<HoistedLocalKey, MultiValue, ValueSetLattice<SingleValue>> hoistedLocalsLattice,
+			LinkContext context)
+			=> (MethodBodyLattice, HoistedLocalsLattice, Context) = (methodBodyLattice, hoistedLocalsLattice, context);
 
 		public InterproceduralState Top => new InterproceduralState (MethodBodyLattice.Top, HoistedLocalsLattice.Top, this);
 

src/linker/Linker.Dataflow/MethodBodyScanner.cs

@@ -51,7 +51,7 @@ abstract partial class MethodBodyScanner
 		protected MethodBodyScanner (LinkContext context)
 		{
 			this._context = context;
-			this.InterproceduralStateLattice = default;
+			this.InterproceduralStateLattice = new InterproceduralStateLattice (default, default, context);
 		}
 
 		internal MultiValue ReturnValue { private set; get; }
@@ -151,9 +151,9 @@ private struct BasicBlockIterator
 			int _currentBlockIndex;
 			bool _foundEndOfPrevBlock;
 
-			public BasicBlockIterator (MethodBody methodBody)
+			public BasicBlockIterator (MethodIL methodIL)
 			{
-				_methodBranchTargets = methodBody.ComputeBranchTargets ();
+				_methodBranchTargets = methodIL.ComputeBranchTargets ();
 				_currentBlockIndex = -1;
 				_foundEndOfPrevBlock = true;
 			}
@@ -226,25 +226,25 @@ protected static void StoreMethodLocalValue<KeyType> (
 
 		// Scans the method as well as any nested functions (local functions or lambdas) and state machines
 		// reachable from it.
-		public virtual void InterproceduralScan (MethodBody startingMethodBody)
+		public virtual void InterproceduralScan (MethodIL startingMethodIL)
 		{
-			MethodDefinition startingMethod = startingMethodBody.Method;
+			MethodDefinition startingMethod = startingMethodIL.Method;
 
 			// Note that the default value of a hoisted local will be MultiValueLattice.Top, not UnknownValue.Instance.
 			// This ensures that there are no warnings for the "unassigned state" of a parameter.
 			// Definite assignment should ensure that there is no way for this to be an analysis hole.
 			var interproceduralState = InterproceduralStateLattice.Top;
 
 			var oldInterproceduralState = interproceduralState.Clone ();
-			interproceduralState.TrackMethod (startingMethodBody);
+			interproceduralState.TrackMethod (startingMethodIL);
 
 			while (!interproceduralState.Equals (oldInterproceduralState)) {
 				oldInterproceduralState = interproceduralState.Clone ();
 
 				// Flow state through all methods encountered so far, as long as there
 				// are changes discovered in the hoisted local state on entry to any method.
-				foreach (var methodBodyValue in oldInterproceduralState.MethodBodies)
-					Scan (methodBodyValue.MethodBody, ref interproceduralState);
+				foreach (var methodIL in oldInterproceduralState.MethodBodies)
+					Scan (methodIL, ref interproceduralState);
 			}
 
 #if DEBUG
@@ -274,22 +274,23 @@ void TrackNestedFunctionReference (MethodReference referencedMethod, ref Interpr
 			interproceduralState.TrackMethod (method);
 		}
 
-		protected virtual void Scan (MethodBody methodBody, ref InterproceduralState interproceduralState)
+		protected virtual void Scan (MethodIL methodIL, ref InterproceduralState interproceduralState)
 		{
+			MethodBody methodBody = methodIL.Body;
 			MethodDefinition thisMethod = methodBody.Method;
 
-			LocalVariableStore locals = new (methodBody.Variables.Count);
+			LocalVariableStore locals = new (methodIL.Variables.Count);
 
 			Dictionary<int, Stack<StackSlot>> knownStacks = new Dictionary<int, Stack<StackSlot>> ();
 			Stack<StackSlot>? currentStack = new Stack<StackSlot> (methodBody.MaxStackSize);
 
-			ScanExceptionInformation (knownStacks, methodBody);
+			ScanExceptionInformation (knownStacks, methodIL);
 
-			BasicBlockIterator blockIterator = new BasicBlockIterator (methodBody);
+			BasicBlockIterator blockIterator = new BasicBlockIterator (methodIL);
 
 			ReturnValue = new ();
-			foreach (Instruction operation in methodBody.Instructions) {
-				ValidateNoReferenceToReference (locals, methodBody.Method, operation.Offset);
+			foreach (Instruction operation in methodIL.Instructions) {
+				ValidateNoReferenceToReference (locals, methodBody.Method, operation.Offset); 
 				int curBasicBlock = blockIterator.MoveNext (operation);
 
 				if (knownStacks.ContainsKey (operation.Offset)) {
@@ -403,7 +404,7 @@ protected virtual void Scan (MethodBody methodBody, ref InterproceduralState int
 				case Code.Ldarg_S:
 				case Code.Ldarga:
 				case Code.Ldarga_S:
-					ScanLdarg (operation, currentStack, thisMethod, methodBody);
+					ScanLdarg (operation, currentStack, thisMethod, methodIL);
 					break;
 
 				case Code.Ldloc:
@@ -414,7 +415,7 @@ protected virtual void Scan (MethodBody methodBody, ref InterproceduralState int
 				case Code.Ldloc_S:
 				case Code.Ldloca:
 				case Code.Ldloca_S:
-					ScanLdloc (operation, currentStack, methodBody, locals);
+					ScanLdloc (operation, currentStack, methodIL, locals);
 					break;
 
 				case Code.Ldstr: {
@@ -577,7 +578,7 @@ protected virtual void Scan (MethodBody methodBody, ref InterproceduralState int
 				case Code.Stloc_1:
 				case Code.Stloc_2:
 				case Code.Stloc_3:
-					ScanStloc (operation, currentStack, methodBody, locals, curBasicBlock);
+					ScanStloc (operation, currentStack, methodIL, locals, curBasicBlock);
 					break;
 
 				case Code.Constrained:
@@ -697,9 +698,9 @@ protected virtual void Scan (MethodBody methodBody, ref InterproceduralState int
 			}
 		}
 
-		private static void ScanExceptionInformation (Dictionary<int, Stack<StackSlot>> knownStacks, MethodBody methodBody)
+		private static void ScanExceptionInformation (Dictionary<int, Stack<StackSlot>> knownStacks, MethodIL methodIL)
 		{
-			foreach (ExceptionHandler exceptionClause in methodBody.ExceptionHandlers) {
+			foreach (ExceptionHandler exceptionClause in methodIL.ExceptionHandlers) {
 				Stack<StackSlot> catchStack = new Stack<StackSlot> (1);
 				catchStack.Push (new StackSlot ());
 
@@ -715,7 +716,7 @@ private static void ScanExceptionInformation (Dictionary<int, Stack<StackSlot>>
 
 		protected abstract SingleValue GetMethodParameterValue (MethodDefinition method, int parameterIndex);
 
-		private void ScanLdarg (Instruction operation, Stack<StackSlot> currentStack, MethodDefinition thisMethod, MethodBody methodBody)
+		private void ScanLdarg (Instruction operation, Stack<StackSlot> currentStack, MethodDefinition thisMethod, MethodIL methodBody)
 		{
 			Code code = operation.OpCode.Code;
 
@@ -741,7 +742,7 @@ private void ScanLdarg (Instruction operation, Stack<StackSlot> currentStack, Me
 			} else {
 				var paramDefinition = (ParameterDefinition) operation.Operand;
 				if (thisMethod.HasImplicitThis ()) {
-					if (paramDefinition == methodBody.ThisParameter) {
+					if (paramDefinition == methodBody.Body.ThisParameter) {
 						paramNum = 0;
 					} else {
 						paramNum = paramDefinition.Index + 1;
@@ -781,12 +782,12 @@ private void ScanStarg (
 		private void ScanLdloc (
 			Instruction operation,
 			Stack<StackSlot> currentStack,
-			MethodBody methodBody,
+			MethodIL methodIL,
 			LocalVariableStore locals)
 		{
-			VariableDefinition localDef = GetLocalDef (operation, methodBody.Variables);
+			VariableDefinition localDef = GetLocalDef (operation, methodIL.Variables);
 			if (localDef == null) {
-				PushUnknownAndWarnAboutInvalidIL (currentStack, methodBody, operation.Offset);
+				PushUnknownAndWarnAboutInvalidIL (currentStack, methodIL.Body, operation.Offset);
 				return;
 			}
 
@@ -844,14 +845,14 @@ void ScanLdtoken (Instruction operation, Stack<StackSlot> currentStack)
 		private void ScanStloc (
 			Instruction operation,
 			Stack<StackSlot> currentStack,
-			MethodBody methodBody,
+			MethodIL methodIL,
 			LocalVariableStore locals,
 			int curBasicBlock)
 		{
-			StackSlot valueToStore = PopUnknown (currentStack, 1, methodBody, operation.Offset);
-			VariableDefinition localDef = GetLocalDef (operation, methodBody.Variables);
+			StackSlot valueToStore = PopUnknown (currentStack, 1, methodIL.Body, operation.Offset);
+			VariableDefinition localDef = GetLocalDef (operation, methodIL.Variables);
 			if (localDef == null) {
-				WarnAboutInvalidILInMethod (methodBody, operation.Offset);
+				WarnAboutInvalidILInMethod (methodIL.Body, operation.Offset);
 				return;
 			}
 

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

@@ -61,21 +61,21 @@ public ReflectionMethodBodyScanner (LinkContext context, MarkStep parent, Messag
 			TrimAnalysisPatterns = new TrimAnalysisPatternStore (MultiValueLattice, context);
 		}
 
-		public override void InterproceduralScan (MethodBody methodBody)
+		public override void InterproceduralScan (MethodIL methodIL)
 		{
-			base.InterproceduralScan (methodBody);
+			base.InterproceduralScan (methodIL);
 
 			var reflectionMarker = new ReflectionMarker (_context, _markStep, enabled: true);
 			TrimAnalysisPatterns.MarkAndProduceDiagnostics (reflectionMarker, _markStep);
 		}
 
-		protected override void Scan (MethodBody methodBody, ref InterproceduralState interproceduralState)
+		protected override void Scan (MethodIL methodIL, ref InterproceduralState interproceduralState)
 		{
-			_origin = new MessageOrigin (methodBody.Method);
-			base.Scan (methodBody, ref interproceduralState);
+			_origin = new MessageOrigin (methodIL.Method);
+			base.Scan (methodIL, ref interproceduralState);
 
-			if (!methodBody.Method.ReturnsVoid ()) {
-				var method = methodBody.Method;
+			if (!methodIL.Method.ReturnsVoid ()) {
+				var method = methodIL.Method;
 				var methodReturnValue = _annotations.GetMethodReturnValue (method);
 				if (methodReturnValue.DynamicallyAccessedMemberTypes != 0)
 					HandleAssignmentPattern (_origin, ReturnValue, methodReturnValue);

src/linker/Linker.Dataflow/ScannerExtensions.cs

@@ -16,10 +16,10 @@ public static bool IsControlFlowInstruction (in this OpCode opcode)
 				|| (opcode.FlowControl == FlowControl.Return && opcode.Code != Code.Ret);
 		}
 
-		public static HashSet<int> ComputeBranchTargets (this MethodBody methodBody)
+		public static HashSet<int> ComputeBranchTargets (this MethodIL methodIL)
 		{
 			HashSet<int> branchTargets = new HashSet<int> ();
-			foreach (Instruction operation in methodBody.Instructions) {
+			foreach (Instruction operation in methodIL.Instructions) {
 				if (!operation.OpCode.IsControlFlowInstruction ())
 					continue;
 				Object value = operation.Operand;
@@ -31,7 +31,7 @@ public static HashSet<int> ComputeBranchTargets (this MethodBody methodBody)
 					}
 				}
 			}
-			foreach (ExceptionHandler einfo in methodBody.ExceptionHandlers) {
+			foreach (ExceptionHandler einfo in methodIL.ExceptionHandlers) {
 				if (einfo.HandlerType == ExceptionHandlerType.Filter) {
 					branchTargets.Add (einfo.FilterStart.Offset);
 				}

src/linker/Linker.Steps/AddBypassNGenStep.cs

@@ -94,7 +94,9 @@ private void EnsureBypassNGenAttribute (ModuleDefinition targetModule)
 
 				const MethodAttributes ctorAttributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.SpecialName | MethodAttributes.RTSpecialName;
 				bypassNGenAttributeDefaultConstructor = new MethodDefinition (".ctor", ctorAttributes, coreLibAssembly.MainModule.TypeSystem.Void);
+#pragma warning disable RS0030 // Anything after MarkStep should use Cecil directly as all method bodies should be processed by this point
 				var instructions = bypassNGenAttributeDefaultConstructor.Body.Instructions;
+#pragma warning restore RS0030
 				instructions.Add (Instruction.Create (OpCodes.Ldarg_0));
 				instructions.Add (Instruction.Create (OpCodes.Call, systemAttributeDefaultConstructor));
 				instructions.Add (Instruction.Create (OpCodes.Ret));

src/linker/Linker.Steps/CodeRewriterStep.cs

@@ -64,12 +64,15 @@ void AddFieldsInitializations (TypeDefinition type)
 				ret = Instruction.Create (OpCodes.Ret);
 				processor.Append (ret);
 			} else {
-				ret = cctor.Body.Instructions.Last (l => l.OpCode.Code == Code.Ret);
 				var body = cctor.Body;
-				processor = cctor.Body.GetLinkerILProcessor ();
-
-				for (int i = 0; i < body.Instructions.Count; ++i) {
-					var instr = body.Instructions[i];
+#pragma warning disable RS0030 // After MarkStep all methods should be processed and thus accessing Cecil directly is the right approach
+				var instructions = body.Instructions;
+#pragma warning restore RS0030
+				ret = instructions.Last (l => l.OpCode.Code == Code.Ret);
+				processor = body.GetLinkerILProcessor ();
+
+				for (int i = 0; i < instructions.Count; ++i) {
+					var instr = instructions[i];
 					if (instr.OpCode.Code != Code.Stsfld)
 						continue;
 
@@ -199,7 +202,9 @@ static void StubComplexBody (MethodDefinition method, MethodBody body, LinkerILP
 			case MetadataType.MVar:
 			case MetadataType.ValueType:
 				var vd = new VariableDefinition (method.ReturnType);
+#pragma warning disable RS0030 // Anything after MarkStep should not use ILProvider since all methods are guaranteed processed
 				body.Variables.Add (vd);
+#pragma warning restore RS0030
 				body.InitLocals = true;
 
 				il.Emit (OpCodes.Ldloca_S, vd);