From bad92f87fd4a1fa1e87c544a18026dcf9e16407b Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Tue, 10 Aug 2021 15:23:45 -0700 Subject: [PATCH 1/5] Simple fix for isinstr rewriter breaking finaly blocks --- src/linker/Linker.Steps/MarkStep.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index afdde1402834..9feefbf80559 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -532,6 +532,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; + } } } @@ -3455,6 +3468,12 @@ protected virtual void MarkInstruction (Instruction instruction, MethodDefinitio if (type.IsInterface) break; + if (type.Name == "QuicStreamAbortedException") + Debugger.Break (); + + if (type.Name == "TypeToCheckException") + Debugger.Break (); + if (!Annotations.IsInstantiated (type)) { _pending_isinst_instr.Add ((type, method.Body, instruction)); return; From 0f1ffd0e22a58e402ecebbf25e2c0c5640aa6d73 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Wed, 11 Aug 2021 04:09:01 -0700 Subject: [PATCH 2/5] Test support for try/catch/filter blocks. --- src/linker/Linker.Steps/MarkStep.cs | 6 --- .../Advanced/TypeCheckRemoval.cs | 40 +++++++++++++++++++ .../TestCasesRunner/AssemblyChecker.cs | 37 ++++++++++++++++- .../TestCasesRunner/ResultChecker.cs | 2 +- 4 files changed, 76 insertions(+), 9 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 9feefbf80559..000450db361a 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -3468,12 +3468,6 @@ protected virtual void MarkInstruction (Instruction instruction, MethodDefinitio if (type.IsInterface) break; - if (type.Name == "QuicStreamAbortedException") - Debugger.Break (); - - if (type.Name == "TypeToCheckException") - Debugger.Break (); - if (!Annotations.IsInstantiated (type)) { _pending_isinst_instr.Add ((type, method.Body, instruction)); return; diff --git a/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs b/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs index ae330c2073a7..04e734d59e9a 100644 --- a/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs +++ b/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs @@ -20,6 +20,8 @@ public static void Main () TestTypeCheckKept_2 (null); TestTypeCheckKept_3 (); TestTypeCheckKept_4 (null); + + TypeCheckRemovalInExceptionFilter.Test (); } [Kept] @@ -230,5 +232,43 @@ class T8 [Kept] public object Instance; } + + [Kept] + class TypeCheckRemovalInExceptionFilter + { + [Kept] + [KeptBaseType(typeof(Exception))] + class TypeToCheckException : Exception { + [Kept] + public int Value { + [Kept] + [ExpectedInstructionSequence (new string[] { + "ldstr", + "newobj", + "throw" + })] + get; } + } + + [Kept] + static void MethodWithFilter () + { + try { + new object (); // Do nothing + } + catch (TypeToCheckException ex) when (ex.Value == 0) { + throw new ApplicationException (); + } + catch (TypeToCheckException ex) when (ex.Value == 1) { + throw new ApplicationException (); + } + } + + [Kept] + public static void Test () + { + MethodWithFilter (); + } + } } } \ No newline at end of file diff --git a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs index eab25a2265ab..59d5a9525bb0 100644 --- a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs +++ b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs @@ -390,11 +390,44 @@ 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))); + } + + HashSet<(Instruction, Instruction)> existingTryBlocks = new HashSet<(Instruction, Instruction)> (); + foreach (var exHandler in body.ExceptionHandlers) { + if (!existingTryBlocks.Contains ((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"); + + existingTryBlocks.Add ((exHandler.TryStart, exHandler.TryEnd)); + } + + 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: diff --git a/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs b/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs index e03e732f8a6d..a9b9a20f4bd0 100644 --- a/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs +++ b/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); From 8a1b81ae27a32f83de1ab39d69245a16548c9822 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Wed, 11 Aug 2021 08:39:51 -0700 Subject: [PATCH 3/5] Improve tests --- .../Advanced/TypeCheckRemoval.cs | 103 +++++++++++++++--- .../TestCasesRunner/AssemblyChecker.cs | 8 +- 2 files changed, 93 insertions(+), 18 deletions(-) diff --git a/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs b/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs index 04e734d59e9a..0ce3da578afd 100644 --- a/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs +++ b/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs @@ -238,36 +238,111 @@ class TypeCheckRemovalInExceptionFilter { [Kept] [KeptBaseType(typeof(Exception))] - class TypeToCheckException : Exception { + class TypeToCheckException : Exception { [Kept] - public int Value { - [Kept] - [ExpectedInstructionSequence (new string[] { - "ldstr", - "newobj", - "throw" - })] - get; } + public int Value; } [Kept] - static void MethodWithFilter () + [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 { - new object (); // Do nothing + 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) { - throw new ApplicationException (); } catch (TypeToCheckException ex) when (ex.Value == 1) { - throw new ApplicationException (); } } [Kept] public static void Test () { - MethodWithFilter (); + MethodWithFilterRemovalInTry (null); + MethodWithTwoFilters (); } } } diff --git a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs index 59d5a9525bb0..7ac8b004763c 100644 --- a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs +++ b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs @@ -399,7 +399,7 @@ 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))); + result.Add ((instruction, FormatInstruction (instruction).ToLowerInvariant ())); } HashSet<(Instruction, Instruction)> existingTryBlocks = new HashSet<(Instruction, Instruction)> (); @@ -507,18 +507,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)}"); } } From bef0ca0ed74b51f7d2209cf2dccd18770c9ca441 Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Wed, 11 Aug 2021 09:01:10 -0700 Subject: [PATCH 4/5] Fix tests and formatting --- .../Advanced/TypeCheckRemoval.cs | 16 +++++----- .../UnreachableBlock/ReplacedReturns.cs | 30 ++++++++++++++----- .../UnreachableBlock/TryFilterBlocks.cs | 14 +++++++-- 3 files changed, 42 insertions(+), 18 deletions(-) diff --git a/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs b/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs index 0ce3da578afd..2086442003bd 100644 --- a/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs +++ b/test/Mono.Linker.Tests.Cases/Advanced/TypeCheckRemoval.cs @@ -237,8 +237,9 @@ class T8 class TypeCheckRemovalInExceptionFilter { [Kept] - [KeptBaseType(typeof(Exception))] - class TypeToCheckException : Exception { + [KeptBaseType (typeof (Exception))] + class TypeToCheckException : Exception + { [Kept] public int Value; } @@ -277,8 +278,7 @@ static void MethodWithFilterRemovalInTry (object o) try { if (o is TypeToCheckException) { } - } - catch (TypeToCheckException ex) when (ex.Value == 0) { + } catch (TypeToCheckException ex) when (ex.Value == 0) { } } @@ -325,16 +325,14 @@ static void MethodWithFilterRemovalInTry (object o) "pop", "leave.s il_3a", ".endcatch", - "ret", + "ret", })] static void MethodWithTwoFilters () { try { new object (); - } - catch (TypeToCheckException ex) when (ex.Value == 0) { - } - catch (TypeToCheckException ex) when (ex.Value == 1) { + } catch (TypeToCheckException ex) when (ex.Value == 0) { + } catch (TypeToCheckException ex) when (ex.Value == 1) { } } diff --git a/test/Mono.Linker.Tests.Cases/UnreachableBlock/ReplacedReturns.cs b/test/Mono.Linker.Tests.Cases/UnreachableBlock/ReplacedReturns.cs index 73d568fbf513..1ac92c851e6f 100644 --- a/test/Mono.Linker.Tests.Cases/UnreachableBlock/ReplacedReturns.cs +++ b/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,6 +166,7 @@ static void Test5 () [Kept] [ExpectedInstructionSequence (new[] { + ".try", "call", "pop", "call", @@ -169,14 +174,17 @@ static void Test5 () "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 () { diff --git a/test/Mono.Linker.Tests.Cases/UnreachableBlock/TryFilterBlocks.cs b/test/Mono.Linker.Tests.Cases/UnreachableBlock/TryFilterBlocks.cs index 124fce138b94..2c9c344f557e 100644 --- a/test/Mono.Linker.Tests.Cases/UnreachableBlock/TryFilterBlocks.cs +++ b/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,8 +51,11 @@ static int TestUnreachableInsideTry () [Kept] [ExpectedInstructionSequence (new[] { + ".try", "call", "leave.s il_18", + ".endtry", + ".filter", "pop", "call", "brfalse.s il_f", @@ -55,10 +63,12 @@ static int TestUnreachableInsideTry () "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 () From c32d26d99be4e47421de6f78a65981febe2fe57c Mon Sep 17 00:00:00 2001 From: vitek-karas Date: Wed, 11 Aug 2021 12:59:18 -0700 Subject: [PATCH 5/5] PR Feedback --- test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs index 7ac8b004763c..0bc38dc48afa 100644 --- a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs +++ b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs @@ -404,7 +404,7 @@ public static string[] FormatMethodBody (MethodBody body) HashSet<(Instruction, Instruction)> existingTryBlocks = new HashSet<(Instruction, Instruction)> (); foreach (var exHandler in body.ExceptionHandlers) { - if (!existingTryBlocks.Contains ((exHandler.TryStart, exHandler.TryEnd))) { + if (existingTryBlocks.Add ((exHandler.TryStart, exHandler.TryEnd))) { InsertBeforeInstruction (exHandler.TryStart, ".try"); InsertBeforeInstruction (exHandler.TryEnd, ".endtry"); } @@ -417,8 +417,6 @@ public static string[] FormatMethodBody (MethodBody body) if (exHandler.FilterStart != null) InsertBeforeInstruction (exHandler.FilterStart, ".filter"); - - existingTryBlocks.Add ((exHandler.TryStart, exHandler.TryEnd)); } return result.Select (i => i.Item2).ToArray ();