Fix broken try/catch/filter offsets after isinst optimization (#2205)

The isinst optimization replaces that instruction with a pop, ldnull when the type is not instantiated. This changes the instruction and also changes the length of the instruction in that position. Cecil unfortunately doesn't update try/catch/filter references and they keep pointing to the old isinst instruction which not part of the method body anymore. When saving the assembly the offsets stores in the try/catch/filter records end up effective random and corrupted.

This is a short-term fix to unblock failures in runtime due to this problem.

Medium term fix would be to carefully handle all IL replacements in the linker with regard to try/catch/filter records.

Ideally the long term fix would be to do this in Cecil in such a way that IL replacements would be correctly handled on their own.

This fixes the Http3RequestStream failures mentioned in #2181, but I was not able to confirm if this fixes the CoreLib ArrayPool issues as well (I think it will not).

src/linker/Linker.Steps/MarkStep.cs

@@ -511,6 +511,19 @@ static void UpdateBranchTarget (MethodBody body, Instruction oldTarget, Instruct
 						break;
 					}
 				}
+
+				foreach (var handler in body.ExceptionHandlers) {
+					if (handler.TryStart == oldTarget)
+						handler.TryStart = newTarget;
+					if (handler.TryEnd == oldTarget)
+						handler.TryEnd = newTarget;
+					if (handler.HandlerStart == oldTarget)
+						handler.HandlerStart = newTarget;
+					if (handler.HandlerEnd == oldTarget)
+						handler.HandlerEnd = newTarget;
+					if (handler.FilterStart == oldTarget)
+						handler.FilterStart = newTarget;
+				}
 			}
 		}
 

test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs

@@ -20,6 +20,8 @@ public static void Main ()
 			TestTypeCheckKept_2<string> (null);
 			TestTypeCheckKept_3 ();
 			TestTypeCheckKept_4 (null);
+
+			TypeCheckRemovalInExceptionFilter.Test ();
 		}
 
 		[Kept]
@@ -230,5 +232,116 @@ class T8
 			[Kept]
 			public object Instance;
 		}
+
+		[Kept]
+		class TypeCheckRemovalInExceptionFilter
+		{
+			[Kept]
+			[KeptBaseType (typeof (Exception))]
+			class TypeToCheckException : Exception
+			{
+				[Kept]
+				public int Value;
+			}
+
+			[Kept]
+			[ExpectedInstructionSequence (new string[] {
+				".try",
+				"ldarg.0",
+				"pop",
+				"ldnull",
+				"pop",
+				"leave.s il_1f",
+				".endtry",
+				".filter",
+				"pop",
+				"ldnull",
+				"dup",
+				"brtrue.s il_f",
+				"pop",
+				"ldc.i4.0",
+				"br.s il_1a",
+				"ldfld",
+				"ldc.i4.0",
+				"ceq",
+				"ldc.i4.0",
+				"cgt.un",
+				"endfilter",
+				".catch",
+				"pop",
+				"leave.s il_1f",
+				".endcatch",
+				"ret"
+			})]
+			static void MethodWithFilterRemovalInTry (object o)
+			{
+				try {
+					if (o is TypeToCheckException) {
+					}
+				} catch (TypeToCheckException ex) when (ex.Value == 0) {
+				}
+			}
+
+			[Kept]
+			[ExpectedInstructionSequence (new string[] {
+				".try",
+				"newobj",
+				"pop",
+				"leave.s il_3a",
+				".endtry",
+				".filter",
+				"pop",
+				"ldnull",
+				"dup",
+				"brtrue.s il_11",
+				"pop",
+				"ldc.i4.0",
+				"br.s il_1c",
+				"ldfld",
+				"ldc.i4.0",
+				"ceq",
+				"ldc.i4.0",
+				"cgt.un",
+				"endfilter",
+				".catch",
+				"pop",
+				"leave.s il_3a",
+				".endcatch",
+				".filter",
+				"pop",
+				"ldnull",
+				"dup",
+				"brtrue.s il_2a",
+				"pop",
+				"ldc.i4.0",
+				"br.s il_35",
+				"ldfld",
+				"ldc.i4.1",
+				"ceq",
+				"ldc.i4.0",
+				"cgt.un",
+				"endfilter",
+				".catch",
+				"pop",
+				"leave.s il_3a",
+				".endcatch",
+				"ret",
+			})]
+			static void MethodWithTwoFilters ()
+			{
+				try {
+					new object ();
+				} catch (TypeToCheckException ex) when (ex.Value == 0) {
+				} catch (TypeToCheckException ex) when (ex.Value == 1) {
+				}
+			}
+
+			[Kept]
+			public static void Test ()
+			{
+				MethodWithFilterRemovalInTry (null);
+				MethodWithTwoFilters ();
+			}
+		}
 	}
 }

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

@@ -133,16 +133,20 @@ static TestEnum Test4 ()
 
 		[Kept]
 		[ExpectedInstructionSequence (new[] {
+			".try",
 			"call",
 			"pop",
 			"call",
 			"leave.s il_16",
+			".endtry",
+			".catch",
 			"pop",
 			"call",
 			"leave.s il_15",
+			".endcatch",
 			"ret",
-			"ret"
-			})]
+			"ret",
+		})]
 		static void Test5 ()
 		{
 			try {
@@ -162,21 +166,25 @@ static void Test5 ()
 
 		[Kept]
 		[ExpectedInstructionSequence (new[] {
+			".try",
 			"call",
 			"pop",
 			"call",
 			"ldc.i4.1",
 			"conv.i8",
 			"stloc.0",
 			"leave.s il_16",
+			".endtry",
+			".catch",
 			"pop",
 			"ldc.i4.2",
 			"conv.i8",
 			"stloc.0",
 			"leave.s il_16",
+			".endcatch",
 			"ldloc.0",
-			"ret"
-			})]
+			"ret",
+		})]
 		static long Test6 ()
 		{
 			try {
@@ -195,21 +203,25 @@ static long Test6 ()
 		[ExpectedInstructionSequence (new[] {
 			"ldc.i4.0",
 			"stloc.0",
+			".try",
 			"call",
 			"pop",
 			"call",
 			"ldc.i4.1",
 			"stloc.1",
 			"leave.s il_1c",
+			".endtry",
+			".catch",
 			"pop",
 			"ldloc.0",
 			"call",
 			"leave.s il_1a",
+			".endcatch",
 			"ldc.i4.3",
 			"ret",
 			"ldloc.1",
-			"ret"
-			})]
+			"ret",
+		})]
 		static byte Test7 ()
 		{
 			int i = 0;
@@ -251,13 +263,17 @@ static void Test8 ()
 
 		[Kept]
 		[ExpectedInstructionSequence (new[] {
+			".try",
 			"call",
 			"pop",
 			"call",
 			"leave.s il_10",
+			".endtry",
+			".catch",
 			"pop",
 			"leave.s il_10",
-			"ret"
+			".endcatch",
+			"ret",
 		})]
 		static void Test9 ()
 		{

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

@@ -16,19 +16,24 @@ public static void Main ()
 
 		[Kept]
 		[ExpectedInstructionSequence (new[] {
+			".try",
 			"call",
 			"brfalse.s il_7",
 			"call",
 			"leave.s il_1c",
+			".endtry",
+			".filter",
 			"pop",
 			"call",
 			"ldc.i4.0",
 			"cgt.un",
 			"endfilter",
+			".catch",
 			"pop",
 			"leave.s il_1c",
+			".endcatch",
 			"ldc.i4.2",
-			"ret"
+			"ret",
 		})]
 		[ExpectedExceptionHandlerSequence (new string[] { "filter" })]
 		static int TestUnreachableInsideTry ()
@@ -46,19 +51,24 @@ static int TestUnreachableInsideTry ()
 
 		[Kept]
 		[ExpectedInstructionSequence (new[] {
+			".try",
 			"call",
 			"leave.s il_18",
+			".endtry",
+			".filter",
 			"pop",
 			"call",
 			"brfalse.s il_f",
 			"ldc.i4.0",
 			"ldc.i4.0",
 			"cgt.un",
 			"endfilter",
+			".catch",
 			"pop",
 			"leave.s il_18",
+			".endcatch",
 			"ldc.i4.3",
-			"ret"
+			"ret",
 		})]
 		[ExpectedExceptionHandlerSequence (new string[] { "filter" })]
 		static int TestUnreachableInsideFilterCondition ()

test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs

@@ -390,11 +390,42 @@ protected static void VerifyInstructions (MethodDefinition src, MethodDefinition
 				nameof (ExpectedInstructionSequenceAttribute),
 				nameof (ExpectBodyModifiedAttribute),
 				"instructions",
-				m => m.Body.Instructions.Select (ins => FormatInstruction (ins).ToLower ()).ToArray (),
+				m => FormatMethodBody (m.Body),
 				attr => GetStringArrayAttributeValue (attr).Select (v => v.ToLower ()).ToArray ());
 		}
 
-		public static string FormatInstruction (Instruction instr)
+		public static string[] FormatMethodBody (MethodBody body)
+		{
+			List<(Instruction, string)> result = new List<(Instruction, string)> (body.Instructions.Count);
+			for (int index = 0; index < body.Instructions.Count; index++) {
+				var instruction = body.Instructions[index];
+				result.Add ((instruction, FormatInstruction (instruction).ToLowerInvariant ()));
+			}
+
+			HashSet<(Instruction, Instruction)> existingTryBlocks = new HashSet<(Instruction, Instruction)> ();
+			foreach (var exHandler in body.ExceptionHandlers) {
+				if (existingTryBlocks.Add ((exHandler.TryStart, exHandler.TryEnd))) {
+					InsertBeforeInstruction (exHandler.TryStart, ".try");
+					InsertBeforeInstruction (exHandler.TryEnd, ".endtry");
+				}
+
+				if (exHandler.HandlerStart != null)
+					InsertBeforeInstruction (exHandler.HandlerStart, ".catch");
+
+				if (exHandler.HandlerEnd != null)
+					InsertBeforeInstruction (exHandler.HandlerEnd, ".endcatch");
+
+				if (exHandler.FilterStart != null)
+					InsertBeforeInstruction (exHandler.FilterStart, ".filter");
+			}
+
+			return result.Select (i => i.Item2).ToArray ();
+
+			void InsertBeforeInstruction (Instruction instruction, string text) =>
+				result.Insert (result.FindIndex (i => i.Item1 == instruction), (null, text));
+		}
+
+		static string FormatInstruction (Instruction instr)
 		{
 			switch (instr.OpCode.FlowControl) {
 			case FlowControl.Branch:
@@ -474,18 +505,18 @@ public static void VerifyBodyProperties (MethodDefinition src, MethodDefinition
 			if (src.CustomAttributes.Any (attr => attr.AttributeType.Name == expectModifiedAttributeName)) {
 				Assert.That (
 					linkedValues,
-					Is.Not.EquivalentTo (srcValues),
+					Is.Not.EqualTo (srcValues),
 					$"Expected method `{src} to have {propertyDescription} modified, however, the {propertyDescription} were the same as the original\n{FormattingUtils.FormatSequenceCompareFailureMessage (linkedValues, srcValues)}");
 			} else if (expectedSequenceAttribute != null) {
 				var expected = getExpectFromSequenceAttribute (expectedSequenceAttribute).ToArray ();
 				Assert.That (
 					linkedValues,
-					Is.EquivalentTo (expected),
+					Is.EqualTo (expected),
 					$"Expected method `{src} to have it's {propertyDescription} modified, however, the sequence of {propertyDescription} does not match the expected value\n{FormattingUtils.FormatSequenceCompareFailureMessage2 (linkedValues, expected, srcValues)}");
 			} else {
 				Assert.That (
 					linkedValues,
-					Is.EquivalentTo (srcValues),
+					Is.EqualTo (srcValues),
 					$"Expected method `{src} to have it's {propertyDescription} unchanged, however, the {propertyDescription} differ from the original\n{FormattingUtils.FormatSequenceCompareFailureMessage (linkedValues, srcValues)}");
 			}
 		}

test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs

@@ -1075,7 +1075,7 @@ void VerifyExpectedInstructionSequenceOnMemberInAssembly (CustomAttribute inAsse
 			var memberName = (string) inAssemblyAttribute.ConstructorArguments[2].Value;
 
 			if (TryVerifyKeptMemberInAssemblyAsMethod (memberName, originalType, linkedType, out MethodDefinition originalMethod, out MethodDefinition linkedMethod)) {
-				static string[] valueCollector (MethodDefinition m) => m.Body.Instructions.Select (ins => AssemblyChecker.FormatInstruction (ins).ToLower ()).ToArray ();
+				static string[] valueCollector (MethodDefinition m) => AssemblyChecker.FormatMethodBody (m.Body);
 				var linkedValues = valueCollector (linkedMethod);
 				var srcValues = valueCollector (originalMethod);