diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index 344eaf5b7..4b84edd8b 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -447,6 +447,13 @@ private void InstrumentIL(MethodDefinition method) var sequencePoint = method.DebugInformation.GetSequencePoint(instruction); var targetedBranchPoints = branchPoints.Where(p => p.EndOffset == instruction.Offset); + // Check if the instruction is coverable + if (CecilSymbolHelper.SkipNotCoverableInstruction(method, instruction)) + { + index++; + continue; + } + if (sequencePoint != null && !sequencePoint.IsHidden) { var target = AddInstrumentationCode(method, processor, instruction, sequencePoint); diff --git a/src/coverlet.core/Symbols/CecilSymbolHelper.cs b/src/coverlet.core/Symbols/CecilSymbolHelper.cs index 06bbb2bc1..3cf93afd6 100644 --- a/src/coverlet.core/Symbols/CecilSymbolHelper.cs +++ b/src/coverlet.core/Symbols/CecilSymbolHelper.cs @@ -3,6 +3,7 @@ // team in OpenCover.Framework.Symbols.CecilSymbolManager // using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Runtime.CompilerServices; @@ -18,6 +19,13 @@ namespace Coverlet.Core.Symbols internal static class CecilSymbolHelper { private const int StepOverLineCode = 0xFEEFEE; + private static ConcurrentDictionary CompilerGeneratedBranchesToExclude = null; + + static CecilSymbolHelper() + { + // Create single instance, we cannot collide because we use full method name as key + CompilerGeneratedBranchesToExclude = new ConcurrentDictionary(); + } // In case of nested compiler generated classes, only the root one presents the CompilerGenerated attribute. // So let's search up to the outermost declaring type to find the attribute @@ -227,6 +235,170 @@ Lambda cached field pattern return false; } + private static bool SkipGeneratedBranchForExceptionRethrown(List instructions, Instruction instruction) + { + /* + In case of exception re-thrown inside the catch block, + the compiler generates a branch to check if the exception reference is null. + + A sample of generated code: + + IL_00b4: isinst [System.Runtime]System.Exception + IL_00b9: stloc.s 6 + // if (ex == null) + IL_00bb: ldloc.s 6 + // (no C# code) + IL_00bd: brtrue.s IL_00c6 + + So we can go back to previous instructions and skip this branch if recognize that type of code block + */ + int branchIndex = instructions.BinarySearch(instruction, new InstructionByOffsetComparer()); + return branchIndex >= 3 && // avoid out of range exception (need almost 3 instruction before the branch) + instructions[branchIndex - 3].OpCode == OpCodes.Isinst && + instructions[branchIndex - 3].Operand is TypeReference tr && tr.FullName == "System.Exception" && + instructions[branchIndex - 2].OpCode == OpCodes.Stloc && + instructions[branchIndex - 1].OpCode == OpCodes.Ldloc && + // check for throw opcode after branch + instructions.Count - branchIndex >= 3 && + instructions[branchIndex + 1].OpCode == OpCodes.Ldarg && + instructions[branchIndex + 2].OpCode == OpCodes.Ldfld && + instructions[branchIndex + 3].OpCode == OpCodes.Throw; + } + + private static bool SkipGeneratedBranchesForExceptionHandlers(MethodDefinition methodDefinition, Instruction instruction, List bodyInstructions) + { + if (!CompilerGeneratedBranchesToExclude.ContainsKey(methodDefinition.FullName)) + { + /* + This method is used to parse compiler generated code inside async state machine and find branches generated for exception catch blocks + Typical generated code for catch block is: + + catch ... + { + // (no C# code) + IL_0028: stloc.2 + // object obj2 = <>s__1 = obj; + IL_0029: ldarg.0 + // (no C# code) + IL_002a: ldloc.2 + IL_002b: stfld object ...::'<>s__1' + // <>s__2 = 1; + IL_0030: ldarg.0 + IL_0031: ldc.i4.1 + IL_0032: stfld int32 ...::'<>s__2' <- store 1 into <>s__2 + // (no C# code) + IL_0037: leave.s IL_0039 + } // end handle + + // int num2 = <>s__2; + IL_0039: ldarg.0 + IL_003a: ldfld int32 ...::'<>s__2' <- load <>s__2 value and check if 1 + IL_003f: stloc.3 + // if (num2 == 1) + IL_0040: ldloc.3 + IL_0041: ldc.i4.1 + IL_0042: beq.s IL_0049 <- BRANCH : if <>s__2 value is 1 go to exception handler code + + IL_0044: br IL_00d6 + + IL_0049: nop <- start exception handler code + + In case of multiple catch blocks as + try + { + } + catch (ExceptionType1) + { + } + catch (ExceptionType2) + { + } + + generated IL contains multiple branches: + catch ...(type1) + { + ... + } + catch ...(type2) + { + ... + } + // int num2 = <>s__2; + IL_0039: ldarg.0 + IL_003a: ldfld int32 ...::'<>s__2' <- load <>s__2 value and check if 1 + IL_003f: stloc.3 + // if (num2 == 1) + IL_0040: ldloc.3 + IL_0041: ldc.i4.1 + IL_0042: beq.s IL_0049 <- BRANCH 1 (type 1) + + IL_0044: br IL_00d6 + + // if (num2 == 2) + IL_0067: ldloc.s 4 + IL_0069: ldc.i4.2 + IL_006a: beq IL_0104 <- BRANCH 2 (type 2) + + // (no C# code) + IL_006f: br IL_0191 + */ + List detectedBranches = new List(); + Collection handlers = methodDefinition.Body.ExceptionHandlers; + + int numberOfCatchBlocks = 1; + foreach (var handler in handlers) + { + if (handlers.Any(h => h.HandlerStart == handler.HandlerEnd)) + { + // In case of multiple consecutive catch block + numberOfCatchBlocks++; + continue; + } + + int currentIndex = bodyInstructions.BinarySearch(handler.HandlerEnd, new InstructionByOffsetComparer()); + + /* Detect flag load + // int num2 = <>s__2; + IL_0058: ldarg.0 + IL_0059: ldfld int32 ...::'<>s__2' + IL_005e: stloc.s 4 + */ + if (bodyInstructions.Count - currentIndex > 3 && // check boundary + bodyInstructions[currentIndex].OpCode == OpCodes.Ldarg && + bodyInstructions[currentIndex + 1].OpCode == OpCodes.Ldfld && bodyInstructions[currentIndex + 1].Operand is FieldReference fr && fr.Name.StartsWith("<>s__") && + bodyInstructions[currentIndex + 2].OpCode == OpCodes.Stloc) + { + currentIndex += 3; + for (int i = 0; i < numberOfCatchBlocks; i++) + { + /* + // if (num2 == 1) + IL_0060: ldloc.s 4 + IL_0062: ldc.i4.1 + IL_0063: beq.s IL_0074 + + // (no C# code) + IL_0065: br.s IL_0067 + */ + if (bodyInstructions.Count - currentIndex > 4 && // check boundary + bodyInstructions[currentIndex].OpCode == OpCodes.Ldloc && + bodyInstructions[currentIndex + 1].OpCode == OpCodes.Ldc_I4 && + bodyInstructions[currentIndex + 2].OpCode == OpCodes.Beq && + bodyInstructions[currentIndex + 3].OpCode == OpCodes.Br) + { + detectedBranches.Add(bodyInstructions[currentIndex + 2].Offset); + } + currentIndex += 4; + } + } + } + + CompilerGeneratedBranchesToExclude.TryAdd(methodDefinition.FullName, detectedBranches.ToArray()); + } + + return CompilerGeneratedBranchesToExclude[methodDefinition.FullName].Contains(instruction.Offset); + } + public static List GetBranchPoints(MethodDefinition methodDefinition) { var list = new List(); @@ -236,7 +408,7 @@ public static List GetBranchPoints(MethodDefinition methodDefinitio } uint ordinal = 0; - var instructions = methodDefinition.Body.Instructions; + var instructions = methodDefinition.Body.Instructions.ToList(); bool isAsyncStateMachineMoveNext = IsMoveNextInsideAsyncStateMachine(methodDefinition); bool isMoveNextInsideAsyncStateMachineProlog = isAsyncStateMachineMoveNext && IsMoveNextInsideAsyncStateMachineProlog(methodDefinition); @@ -265,6 +437,14 @@ public static List GetBranchPoints(MethodDefinition methodDefinitio continue; } + if (isAsyncStateMachineMoveNext) + { + if (SkipGeneratedBranchesForExceptionHandlers(methodDefinition, instruction, instructions) || + SkipGeneratedBranchForExceptionRethrown(instructions, instruction)) + { + continue; + } + } if (SkipBranchGeneratedExceptionFilter(instruction, methodDefinition)) { continue; @@ -303,7 +483,7 @@ public static List GetBranchPoints(MethodDefinition methodDefinitio private static bool BuildPointsForConditionalBranch(List list, Instruction instruction, int branchingInstructionLine, string document, int branchOffset, int pathCounter, - Collection instructions, ref uint ordinal, MethodDefinition methodDefinition) + List instructions, ref uint ordinal, MethodDefinition methodDefinition) { // Add Default branch (Path=0) @@ -351,7 +531,7 @@ private static bool BuildPointsForConditionalBranch(List list, Inst } private static uint BuildPointsForBranch(List list, Instruction then, int branchingInstructionLine, string document, - int branchOffset, uint ordinal, int pathCounter, BranchPoint path0, Collection instructions, MethodDefinition methodDefinition) + int branchOffset, uint ordinal, int pathCounter, BranchPoint path0, List instructions, MethodDefinition methodDefinition) { var pathOffsetList1 = GetBranchPath(@then); @@ -431,6 +611,100 @@ private static uint BuildPointsForSwitchCases(List list, BranchPoin return ordinal; } + /* + Need to skip instrumentation after exception re-throw inside catch block (only for async state machine MoveNext()) + es: + try + { + ... + } + catch + { + await ... + throw; + } // need to skip instrumentation here + + We can detect this type of code block by searching for method ExceptionDispatchInfo.Throw() inside the compiled IL + ... + // ExceptionDispatchInfo.Capture(ex).Throw(); + IL_00c6: ldloc.s 6 + IL_00c8: call class [System.Runtime]System.Runtime.ExceptionServices.ExceptionDispatchInfo [System.Runtime]System.Runtime.ExceptionServices.ExceptionDispatchInfo::Capture(class [System.Runtime]System.Exception) + IL_00cd: callvirt instance void [System.Runtime]System.Runtime.ExceptionServices.ExceptionDispatchInfo::Throw() + // NOT COVERABLE + IL_00d2: nop + IL_00d3: nop + ... + + In case of nested code blocks inside catch we need to detect also goto calls + ... + // ExceptionDispatchInfo.Capture(ex).Throw(); + IL_00d3: ldloc.s 7 + IL_00d5: call class [System.Runtime]System.Runtime.ExceptionServices.ExceptionDispatchInfo [System.Runtime]System.Runtime.ExceptionServices.ExceptionDispatchInfo::Capture(class [System.Runtime]System.Exception) + IL_00da: callvirt instance void [System.Runtime]System.Runtime.ExceptionServices.ExceptionDispatchInfo::Throw() + // NOT COVERABLE + IL_00df: nop + IL_00e0: nop + IL_00e1: br.s IL_00ea + ... + // NOT COVERABLE + IL_00ea: nop + IL_00eb: br.s IL_00ed + ... + */ + internal static bool SkipNotCoverableInstruction(MethodDefinition methodDefinition, Instruction instruction) + { + if (!IsMoveNextInsideAsyncStateMachine(methodDefinition)) + { + return false; + } + + if (instruction.OpCode != OpCodes.Nop) + { + return false; + } + + // detect if current instruction is not coverable + Instruction prev = GetPreviousNoNopInstruction(instruction); + if (prev != null && + prev.OpCode == OpCodes.Callvirt && + prev.Operand is MethodReference mr && mr.FullName == "System.Void System.Runtime.ExceptionServices.ExceptionDispatchInfo::Throw()") + { + return true; + } + + // find the caller of current instruction and detect if not coverable + prev = instruction.Previous; + while (prev != null) + { + if (prev.Operand is Instruction i && (i.Offset == instruction.Offset || i.Offset == prev.Next.Offset)) // caller + { + prev = GetPreviousNoNopInstruction(prev); + break; + } + prev = prev.Previous; + } + + return prev != null && + prev.OpCode == OpCodes.Callvirt && + prev.Operand is MethodReference mr1 && mr1.FullName == "System.Void System.Runtime.ExceptionServices.ExceptionDispatchInfo::Throw()"; + + // local helper + static Instruction GetPreviousNoNopInstruction(Instruction i) + { + Instruction instruction = i.Previous; + while (instruction != null) + { + if (instruction.OpCode != OpCodes.Nop) + { + return instruction; + } + instruction = instruction.Previous; + } + + return null; + } + } + private static bool SkipBranchGeneratedExceptionFilter(Instruction branchInstruction, MethodDefinition methodDefinition) { if (!methodDefinition.Body.HasExceptionHandlers) diff --git a/test/coverlet.core.tests/Coverage/CoverageTests.CatchBlock.cs b/test/coverlet.core.tests/Coverage/CoverageTests.CatchBlock.cs new file mode 100644 index 000000000..019043c88 --- /dev/null +++ b/test/coverlet.core.tests/Coverage/CoverageTests.CatchBlock.cs @@ -0,0 +1,78 @@ +using System.IO; +using System.Threading.Tasks; + +using Coverlet.Core.Samples.Tests; +using Coverlet.Tests.Xunit.Extensions; + +namespace Coverlet.Core.Tests +{ + public partial class CoverageTests + { + [ConditionalFact] + [SkipOnOS(OS.MacOS)] + public void CatchBlock_Issue465() + { + string path = Path.GetTempFileName(); + try + { + FunctionExecutor.Run(async (string[] pathSerialize) => + { + CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run(instance => + { + instance.Test(); + instance.Test_Catch(); + ((Task)instance.TestAsync()).ConfigureAwait(false).GetAwaiter().GetResult(); + ((Task)instance.TestAsync_Catch()).ConfigureAwait(false).GetAwaiter().GetResult(); + + instance.Test(true); + instance.Test_Catch(true); + ((Task)instance.TestAsync(true)).ConfigureAwait(false).GetAwaiter().GetResult(); + ((Task)instance.TestAsync_Catch(true)).ConfigureAwait(false).GetAwaiter().GetResult(); + + instance.Test(false); + instance.Test_Catch(false); + ((Task)instance.TestAsync(false)).ConfigureAwait(false).GetAwaiter().GetResult(); + ((Task)instance.TestAsync_Catch(false)).ConfigureAwait(false).GetAwaiter().GetResult(); + + instance.Test_WithTypedCatch(); + instance.Test_Catch_WithTypedCatch(); + ((Task)instance.TestAsync_WithTypedCatch()).ConfigureAwait(false).GetAwaiter().GetResult(); + ((Task)instance.TestAsync_Catch_WithTypedCatch()).ConfigureAwait(false).GetAwaiter().GetResult(); + + instance.Test_WithTypedCatch(true); + instance.Test_Catch_WithTypedCatch(true); + ((Task)instance.TestAsync_WithTypedCatch(true)).ConfigureAwait(false).GetAwaiter().GetResult(); + ((Task)instance.TestAsync_Catch_WithTypedCatch(true)).ConfigureAwait(false).GetAwaiter().GetResult(); + + instance.Test_WithTypedCatch(false); + instance.Test_Catch_WithTypedCatch(false); + ((Task)instance.TestAsync_WithTypedCatch(false)).ConfigureAwait(false).GetAwaiter().GetResult(); + ((Task)instance.TestAsync_Catch_WithTypedCatch(false)).ConfigureAwait(false).GetAwaiter().GetResult(); + + instance.Test_WithNestedCatch(true); + instance.Test_Catch_WithNestedCatch(true); + ((Task)instance.TestAsync_WithNestedCatch(true)).ConfigureAwait(false).GetAwaiter().GetResult(); + ((Task)instance.TestAsync_Catch_WithNestedCatch(true)).ConfigureAwait(false).GetAwaiter().GetResult(); + + instance.Test_WithNestedCatch(false); + instance.Test_Catch_WithNestedCatch(false); + ((Task)instance.TestAsync_WithNestedCatch(false)).ConfigureAwait(false).GetAwaiter().GetResult(); + ((Task)instance.TestAsync_Catch_WithNestedCatch(false)).ConfigureAwait(false).GetAwaiter().GetResult(); + + return Task.CompletedTask; + }, persistPrepareResultToFile: pathSerialize[0]); + return 0; + }, new string[] { path }); + + var res = TestInstrumentationHelper.GetCoverageResult(path); + res.Document("Instrumentation.CatchBlock.cs") + .AssertLinesCoveredAllBut(BuildConfiguration.Debug, 45, 59, 113, 127, 137, 138, 139, 153, 154, 155, 156, 175, 189, 199, 200, 201, 222, 223, 224, 225, 252, 266, 335, 349) + .ExpectedTotalNumberOfBranches(BuildConfiguration.Debug, 6); + } + finally + { + File.Delete(path); + } + } + } +} \ No newline at end of file diff --git a/test/coverlet.core.tests/Samples/Instrumentation.CatchBlock.cs b/test/coverlet.core.tests/Samples/Instrumentation.CatchBlock.cs new file mode 100644 index 000000000..cab0e6eaa --- /dev/null +++ b/test/coverlet.core.tests/Samples/Instrumentation.CatchBlock.cs @@ -0,0 +1,353 @@ +// Remember to use full name because adding new using directives change line numbers + +using System.Threading.Tasks; + +namespace Coverlet.Core.Samples.Tests +{ + public class CatchBlock + { + public int Parse(string str) + { + try + { + return int.Parse(str); + } + catch + { + throw; + } + } + + public async Task ParseAsync(string str) + { + try + { + return int.Parse(str); + } + catch + { + await Task.Delay(0); + + throw; + } + } + + public void Test() + { + Parse(nameof(Test).Length.ToString()); + } + + public void Test_Catch() + { + try + { + Parse(nameof(Test)); + } + catch { } + } + + public async Task TestAsync() + { + await ParseAsync(nameof(Test).Length.ToString()); + } + + public async Task TestAsync_Catch() + { + try + { + await ParseAsync(nameof(Test)); + } + catch { } + } + + public int Parse(string str, bool condition) + { + try + { + return int.Parse(str); + } + catch + { + if (condition) + { + throw; + } + else + { + throw new System.Exception(); + } + } + } + + public async Task ParseAsync(string str, bool condition) + { + try + { + return int.Parse(str); + } + catch + { + await Task.Delay(0); + + if (condition) + { + throw; + } + else + { + throw new System.Exception(); + } + } + } + + public void Test(bool condition) + { + Parse(nameof(Test).Length.ToString(), condition); + } + + public void Test_Catch(bool condition) + { + try + { + Parse(nameof(Test), condition); + } + catch { } + } + + public async Task TestAsync(bool condition) + { + await ParseAsync(nameof(Test).Length.ToString(), condition); + } + + public async Task TestAsync_Catch(bool condition) + { + try + { + await ParseAsync(nameof(Test), condition); + } + catch { } + } + + public int Parse_WithTypedCatch(string str) + { + try + { + return int.Parse(str); + } + catch (System.DivideByZeroException) + { + throw; + } + catch (System.FormatException) + { + throw; + } + } + + public async Task ParseAsync_WithTypedCatch(string str) + { + try + { + return int.Parse(str); + } + catch (System.DivideByZeroException) + { + await Task.Delay(0); + throw; + } + catch (System.FormatException) + { + await Task.Delay(0); + throw; + } + } + + public void Test_WithTypedCatch() + { + Parse_WithTypedCatch(nameof(Test).Length.ToString()); + } + + public void Test_Catch_WithTypedCatch() + { + try + { + Parse_WithTypedCatch(nameof(Test)); + } + catch { } + } + + public async Task TestAsync_WithTypedCatch() + { + await ParseAsync_WithTypedCatch(nameof(Test).Length.ToString()); + } + + public async Task TestAsync_Catch_WithTypedCatch() + { + try + { + await ParseAsync_WithTypedCatch(nameof(Test)); + } + catch { } + } + + public int Parse_WithTypedCatch(string str, bool condition) + { + try + { + return int.Parse(str); + } + catch (System.DivideByZeroException) + { + throw; + } + catch (System.FormatException) + { + if (condition) + { + throw; + } + else + { + throw new System.Exception(); + } + } + } + + public async Task ParseAsync_WithTypedCatch(string str, bool condition) + { + try + { + return int.Parse(str); + } + catch (System.DivideByZeroException) + { + await Task.Delay(0); + throw; + } + catch (System.FormatException) + { + await Task.Delay(0); + + if (condition) + { + throw; + } + else + { + throw new System.Exception(); + } + } + } + + public void Test_WithTypedCatch(bool condition) + { + Parse_WithTypedCatch(nameof(Test).Length.ToString(), condition); + } + + public void Test_Catch_WithTypedCatch(bool condition) + { + try + { + Parse_WithTypedCatch(nameof(Test), condition); + } + catch { } + } + + public async Task TestAsync_WithTypedCatch(bool condition) + { + await ParseAsync_WithTypedCatch(nameof(Test).Length.ToString(), condition); + } + + public async Task TestAsync_Catch_WithTypedCatch(bool condition) + { + try + { + await ParseAsync_WithTypedCatch(nameof(Test), condition); + } + catch { } + } + + public int Parse_WithNestedCatch(string str, bool condition) + { + try + { + try + { + return int.Parse(str); + } + catch + { + if (condition) + throw new System.Exception(); + else + throw; + } + } + catch (System.FormatException) + { + throw; + } + catch + { + throw; + } + } + + public async Task ParseAsync_WithNestedCatch(string str, bool condition) + { + try + { + try + { + return int.Parse(str); + } + catch + { + await Task.Delay(0); + if (condition) + throw new System.Exception(); + else + throw; + } + } + catch (System.FormatException) + { + await Task.Delay(0); + throw; + } + catch + { + await Task.Delay(0); + throw; + } + } + + public void Test_WithNestedCatch(bool condition) + { + Parse_WithNestedCatch(nameof(Test).Length.ToString(), condition); + } + + public void Test_Catch_WithNestedCatch(bool condition) + { + try + { + Parse_WithNestedCatch(nameof(Test), condition); + } + catch { } + } + + public async Task TestAsync_WithNestedCatch(bool condition) + { + await ParseAsync_WithNestedCatch(nameof(Test).Length.ToString(), condition); + } + + public async Task TestAsync_Catch_WithNestedCatch(bool condition) + { + try + { + await ParseAsync_WithNestedCatch(nameof(Test), condition); + } + catch { } + } + } +}