From e915869de3ad0f76d0dca9e8fbca65054dc7b42f Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Tue, 14 Jul 2020 22:18:23 -0400 Subject: [PATCH 01/18] Implementing reachability analysis, which will allow instructions that follow calls to methods that never return to be excluded from coverage metrics. Determines what methods will return with [DoesNotReturn], though that is configurable. Adds a test that explores a few "odd" patterns. There are a number of open questions: - Preferred way to punch configuration of this attribute in? - Test suite is really fragile, especially around multiple tests locking each other out of the same files - is there a better way to run them? - Reachability does not distinguish between "unreachable because of method call" and "unreachable because the IL is actually unreachable" * The compiler is... disinclined to produce unreachable IL, but this is a real (potentially) significant change - Related, reachability is always calculated and used - should it be ignored if there are no calls to [DoesNotReturn] methods? - "Odder" unconditional branches are not handled (leave, leave_s, jmp, etc.) yet - Naming and code layout things * There are several new classes (mostly nested), where should these go? * There's yet-another-block-class, what should it be named? Are there existing classes that can be used instead? Finally, there are performance concerns. Current approach doesn't go wild with allocations or an insane number of passes or anything, but could probably be improved. --- .../Attributes/DoesNotReturnAttribute.cs | 7 + src/coverlet.core/Coverage.cs | 5 +- .../Instrumentation/Instrumenter.cs | 520 +++++++++++++++++- .../Coverage/InstrumenterHelper.Assertions.cs | 49 +- .../Instrumentation/InstrumenterTests.cs | 56 +- .../Samples/Instrumentation.DoesNotReturn.cs | 116 ++++ 6 files changed, 730 insertions(+), 23 deletions(-) create mode 100644 src/coverlet.core/Attributes/DoesNotReturnAttribute.cs create mode 100644 test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs diff --git a/src/coverlet.core/Attributes/DoesNotReturnAttribute.cs b/src/coverlet.core/Attributes/DoesNotReturnAttribute.cs new file mode 100644 index 000000000..0a5738130 --- /dev/null +++ b/src/coverlet.core/Attributes/DoesNotReturnAttribute.cs @@ -0,0 +1,7 @@ +using System; + +namespace Coverlet.Core.Attributes +{ + [AttributeUsage(AttributeTargets.Property | AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class)] + public class DoesNotReturnAttribute : Attribute { } +} diff --git a/src/coverlet.core/Coverage.cs b/src/coverlet.core/Coverage.cs index c19d007ce..4460b7225 100644 --- a/src/coverlet.core/Coverage.cs +++ b/src/coverlet.core/Coverage.cs @@ -23,6 +23,7 @@ internal class CoverageParameters public bool SingleHit { get; set; } public string MergeWith { get; set; } public bool UseSourceLink { get; set; } + public string[] DoesNotReturnAttributes { get; set; } } internal class Coverage @@ -38,6 +39,7 @@ internal class Coverage private bool _singleHit; private string _mergeWith; private bool _useSourceLink; + private string[] _doesNotReturnAttributes; private ILogger _logger; private IInstrumentationHelper _instrumentationHelper; private IFileSystem _fileSystem; @@ -68,6 +70,7 @@ public Coverage(string module, _singleHit = parameters.SingleHit; _mergeWith = parameters.MergeWith; _useSourceLink = parameters.UseSourceLink; + _doesNotReturnAttributes = parameters.DoesNotReturnAttributes; _logger = logger; _instrumentationHelper = instrumentationHelper; _fileSystem = fileSystem; @@ -115,7 +118,7 @@ public CoveragePrepareResult PrepareModules() continue; } - var instrumenter = new Instrumenter(module, _identifier, _excludeFilters, _includeFilters, _excludedSourceFiles, _excludeAttributes, _singleHit, _logger, _instrumentationHelper, _fileSystem, _sourceRootTranslator, _cecilSymbolHelper); + var instrumenter = new Instrumenter(module, _identifier, _excludeFilters, _includeFilters, _excludedSourceFiles, _excludeAttributes, _doesNotReturnAttributes, _singleHit, _logger, _instrumentationHelper, _fileSystem, _sourceRootTranslator, _cecilSymbolHelper); if (instrumenter.CanInstrument()) { diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index 23b8f3a26..ef1b93f12 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Immutable; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.IO; @@ -13,6 +14,7 @@ using Mono.Cecil; using Mono.Cecil.Cil; using Mono.Cecil.Rocks; +using Mono.Collections.Generic; namespace Coverlet.Core.Instrumentation { @@ -44,6 +46,7 @@ internal class Instrumenter private List _branchesInCompiledGeneratedClass; private List<(MethodDefinition, int)> _excludedMethods; private List _excludedCompilerGeneratedTypes; + private readonly string[] _doesNotReturnAttributes; public bool SkipModule { get; set; } = false; @@ -54,6 +57,7 @@ public Instrumenter( string[] includeFilters, string[] excludedFiles, string[] excludedAttributes, + string[] doesNotReturnAttributes, bool singleHit, ILogger logger, IInstrumentationHelper instrumentationHelper, @@ -66,17 +70,7 @@ public Instrumenter( _excludeFilters = excludeFilters; _includeFilters = includeFilters; _excludedFilesHelper = new ExcludedFilesHelper(excludedFiles, logger); - _excludedAttributes = (excludedAttributes ?? Array.Empty()) - // In case the attribute class ends in "Attribute", but it wasn't specified. - // Both names are included (if it wasn't specified) because the attribute class might not actually end in the prefix. - .SelectMany(a => a.EndsWith("Attribute") ? new[] { a } : new[] { a, $"{a}Attribute" }) - // The default custom attributes used to exclude from coverage. - .Union(new List() - { - nameof(ExcludeFromCoverageAttribute), - nameof(ExcludeFromCodeCoverageAttribute) - }) - .ToArray(); + _excludedAttributes = PrepareAttributes(excludedAttributes, nameof(ExcludeFromCoverageAttribute), nameof(ExcludeFromCodeCoverageAttribute)); _singleHit = singleHit; _isCoreLibrary = Path.GetFileNameWithoutExtension(_module) == "System.Private.CoreLib"; _logger = logger; @@ -84,6 +78,19 @@ public Instrumenter( _fileSystem = fileSystem; _sourceRootTranslator = sourceRootTranslator; _cecilSymbolHelper = cecilSymbolHelper; + _doesNotReturnAttributes = PrepareAttributes(doesNotReturnAttributes, nameof(DoesNotReturnAttribute)); + } + + private static string[] PrepareAttributes(IEnumerable providedAttrs, params string[] defaultAttrs) + { + return + (providedAttrs ?? Array.Empty()) + // In case the attribute class ends in "Attribute", but it wasn't specified. + // Both names are included (if it wasn't specified) because the attribute class might not actually end in the prefix. + .SelectMany(a => a.EndsWith("Attribute") ? new[] { a } : new[] { a, $"{a}Attribute" }) + // The default custom attributes used to exclude from coverage. + .Union(defaultAttrs) + .ToArray(); } public bool CanInstrument() @@ -501,14 +508,32 @@ private void InstrumentIL(MethodDefinition method) var branchPoints = _cecilSymbolHelper.GetBranchPoints(method); + var unreachableRanges = ReachabilityHelper.FindUnreachableIL(processor.Body.Instructions, _doesNotReturnAttributes); + var currentUnreachableRangeIx = 0; + for (int n = 0; n < count; n++) { var instruction = processor.Body.Instructions[index]; 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)) + // make sure we're looking at the correct unreachable range (if any) + var instrOffset = instruction.Offset; + while (currentUnreachableRangeIx < unreachableRanges.Length && instrOffset > unreachableRanges[currentUnreachableRangeIx].EndOffset) + { + currentUnreachableRangeIx++; + } + + // determine if the unreachable + var isUnreachable = false; + if (currentUnreachableRangeIx < unreachableRanges.Length) + { + var range = unreachableRanges[currentUnreachableRangeIx]; + isUnreachable = instrOffset >= range.StartOffset && instrOffset <= range.EndOffset; + } + + // Check is both reachable, _and_ coverable + if (isUnreachable || _cecilSymbolHelper.SkipNotCoverableInstruction(method, instruction)) { index++; continue; @@ -858,4 +883,473 @@ public bool Exclude(string sourceFile) return _matcher.Match(Path.IsPathRooted(sourceFile) ? sourceFile.Substring(Path.GetPathRoot(sourceFile).Length) : sourceFile).HasMatches; } } + + /// + /// Helper for find unreachable IL instructions. + /// + internal class ReachabilityHelper + { + internal readonly struct UnreachableRange + { + /// + /// Offset of first unreachable instruction. + /// + public int StartOffset { get; } + /// + /// Offset of last unreachable instruction. + /// + public int EndOffset { get; } + + public UnreachableRange(int start, int end) + { + StartOffset = start; + EndOffset = end; + } + + public override string ToString() + => $"[{StartOffset}, {EndOffset}]"; + } + + private class Block + { + /// + /// Offset of the instruction that starts the block + /// + public int StartOffset { get; } + /// + /// Whether it is possible for control to flow past the end of the block, + /// ie. whether it's tail is reachable + /// + public bool TailReachable => UnreachableAfter == null; + /// + /// If control flows to the end of the block, where it can flow to + /// + public ImmutableArray BranchesTo { get; } + + /// + /// If this block contains a call(i) to a method that does not return + /// this will be the first such call. + /// + public Instruction UnreachableAfter { get; } + + /// + /// Mutable, records whether control can flow into the block, + /// ie. whether it's head is reachable + /// + public bool HeadReachable { get; set; } + + public Block(int startOffset, Instruction unreachableAfter, ImmutableArray branchesTo) + { + StartOffset = startOffset; + UnreachableAfter = unreachableAfter; + BranchesTo = branchesTo; + } + + public override string ToString() + => $"{nameof(StartOffset)}={StartOffset}, {nameof(HeadReachable)}={HeadReachable}, {nameof(TailReachable)}={TailReachable}, {nameof(BranchesTo)}=({string.Join(", ", BranchesTo)}), {nameof(UnreachableAfter)}={UnreachableAfter}"; + } + + /// + /// Represents an Instruction that transitions control flow (ie. branches). + /// + /// This is _different_ from other branch types, like Branch and BranchPoint + /// because it includes unconditional branches too. + /// + private readonly struct BranchInstruction + { + /// + /// Location of the branching instruction + /// + public int Offset { get; } + + public bool HasMultiTargets => _TargetOffsets.Any(); + + private readonly int _TargetOffset; + + /// + /// Target of the branch, assuming it has a single target. + /// + /// It is illegal to access this if there are multiple targets. + /// + public int TargetOffset + { + get + { + if (HasMultiTargets) + { + throw new InvalidOperationException($"{HasMultiTargets} is true"); + } + + return _TargetOffset; + } + } + + private readonly IEnumerable _TargetOffsets; + + /// + /// Targets of the branch, assuming it has multiple targets. + /// + /// It is illegal to access this if there is a single target. + /// + public IEnumerable TargetOffsets + { + get + { + if (!HasMultiTargets) + { + throw new InvalidOperationException($"{HasMultiTargets} is false"); + } + + return _TargetOffsets; + } + } + + public BranchInstruction(int offset, int targetOffset) + { + Offset = offset; + _TargetOffset = targetOffset; + _TargetOffsets = Enumerable.Empty(); + } + + public BranchInstruction(int offset, IEnumerable targetOffset) + { + if (targetOffset.Count() < 1) + { + throw new ArgumentException("Must have at least 2 entries", nameof(targetOffset)); + } + + Offset = offset; + _TargetOffset = -1; + _TargetOffsets = targetOffset; + } + } + + /// + /// OpCodes that transfer control code, even if they do not + /// introduce branch points. + /// + private static readonly ImmutableHashSet BRANCH_OPCODES = + ImmutableHashSet.CreateRange( + new[] + { + OpCodes.Beq, + OpCodes.Beq_S, + OpCodes.Bge, + OpCodes.Bge_S, + OpCodes.Bge_Un, + OpCodes.Bge_Un_S, + OpCodes.Bgt, + OpCodes.Bgt_S, + OpCodes.Bgt_Un, + OpCodes.Bgt_Un_S, + OpCodes.Ble, + OpCodes.Ble_S, + OpCodes.Ble_Un, + OpCodes.Ble_Un_S, + OpCodes.Blt, + OpCodes.Blt_S, + OpCodes.Blt_Un, + OpCodes.Blt_Un_S, + OpCodes.Bne_Un, + OpCodes.Bne_Un_S, + OpCodes.Br, + OpCodes.Br_S, + OpCodes.Brfalse, + OpCodes.Brfalse_S, + OpCodes.Brtrue, + OpCodes.Brtrue_S, + OpCodes.Switch + } + ); + + /// + /// OpCodes that unconditionally transfer control, so there + /// is not "fall through" branch target. + /// + private static readonly ImmutableHashSet UNCONDITIONAL_BRANCH_OPCODES = + ImmutableHashSet.CreateRange( + new[] + { + OpCodes.Br, + OpCodes.Br_S + } + ); + + /// + /// Calculates which IL instructions are reachable given an instruction stream and branch points extracted from a method. + /// + /// The algorithm works like so: + /// 1. determine the "blocks" that make up a function + /// * A block starts with either the start of the method, or a branch _target_ + /// * blocks are "where some other code might jump to" + /// * blocks end with either another branch, another branch target, or the end of the method + /// * this means blocks contain no control flow, except (maybe) as the very last instruction + /// 2. blocks have head and tail reachability + /// * a block has head reachablility if some other block that is reachable can branch to it + /// * a block has tail reachability if it contains no calls to methods that never return + /// 4. push the first block onto a stack + /// 5. while the stack is not empty + /// a. pop a block off the stack + /// b. give it head reachability + /// c. if the pop'd block is tail reachable, push the blocks it can branch to onto the stack + /// 6. consider each block + /// * if it is head and tail reachable, all instructions in it are reachable + /// * if it is not head reachable (regardless of tail reachability), no instructions in it are reachable + /// * if it is only head reachable, all instructions up to and including the first call to a method that does not return are reachable + /// + public static ImmutableArray FindUnreachableIL(Collection instrs, string[] doesNotReturnAttributes) + { + if (!instrs.Any() || !doesNotReturnAttributes.Any()) + { + return ImmutableArray.Empty; + } + + var brs = FindBranches(instrs); + + var lastInstr = instrs.Last(); + + var blocks = CreateBlocks(instrs, brs, doesNotReturnAttributes); + DetermineHeadReachability(blocks); + return DetermineUnreachableRanges(blocks, lastInstr.Offset); + } + + /// + /// Discovers branches, including unconditional ones, in the given instruction stream. + /// + private static ImmutableArray FindBranches(Collection instrs) + { + var ret = ImmutableArray.CreateBuilder(); + foreach (var i in instrs) + { + if (BRANCH_OPCODES.Contains(i.OpCode)) + { + int? singleTargetOffset; + IEnumerable multiTargetOffsets; + + if (i.Operand is Instruction[] multiTarget) + { + // it's a switch + singleTargetOffset = null; + multiTargetOffsets = multiTarget.Select(t => t.Offset).Concat(new[] { i.Next.Offset }).Distinct().ToList(); + } + else if (i.Operand is Instruction singleTarget) + { + // it's any of the B.*(_S)? instructions + + if (UNCONDITIONAL_BRANCH_OPCODES.Contains(i.OpCode)) + { + multiTargetOffsets = null; + singleTargetOffset = singleTarget.Offset; + } + else + { + singleTargetOffset = null; + multiTargetOffsets = new[] { i.Next.Offset, singleTarget.Offset }; + } + } + else + { + throw new InvalidOperationException($"Unexpected operand when processing branch {i.Operand}: {i.Operand}"); + } + + if (singleTargetOffset != null) + { + ret.Add(new BranchInstruction(i.Offset, singleTargetOffset.Value)); + } + else + { + ret.Add(new BranchInstruction(i.Offset, multiTargetOffsets)); + } + } + } + + return ret.ToImmutable(); + } + + /// + /// Calculates which ranges of IL are unreachable, given blocks which have head and tail reachability calculated + /// and an insturction stream. + /// + private static ImmutableArray DetermineUnreachableRanges(IReadOnlyList blocks, int lastInstructionOffset) + { + var ret = ImmutableArray.CreateBuilder(); + + var endOfMethodOffset = lastInstructionOffset + 1; // add 1 so we point _past_ the end of the method + + for (var curBlockIx = 0; curBlockIx < blocks.Count; curBlockIx++) + { + var curBlock = blocks[curBlockIx]; + + int endOfCurBlockOffset; + if (curBlockIx == blocks.Count - 1) + { + endOfCurBlockOffset = endOfMethodOffset; + } + else + { + endOfCurBlockOffset = blocks[curBlockIx + 1].StartOffset - 1; // minus 1 so we don't include anything of the following block + } + + if (curBlock.HeadReachable) + { + if (curBlock.TailReachable) + { + // it's all reachable + continue; + } + + // tail isn't reachable, which means there's a call to something that doesn't return... + var doesNotReturnInstr = curBlock.UnreachableAfter; + + // and it's everything _after_ the following instruction that is unreachable + // so record the following instruction through the end of the block + var followingInstr = doesNotReturnInstr.Next; + + ret.Add(new UnreachableRange(followingInstr.Offset, endOfCurBlockOffset)); + } + else + { + // none of it is reachable + ret.Add(new UnreachableRange(curBlock.StartOffset, endOfCurBlockOffset)); + } + } + + return ret.ToImmutable(); + } + + /// + /// Process all the blocks and determine if their first instruction is reachable, + /// that is if they have "head reachability". + /// + /// "Tail reachability" will have already been determined in CreateBlocks. + /// + private static void DetermineHeadReachability(IEnumerable blocks) + { + var blockLookup = blocks.ToImmutableDictionary(b => b.StartOffset); + + var headBlock = blockLookup[0]; + + var knownLive = new Stack(); + knownLive.Push(headBlock); + + while (knownLive.Count > 0) + { + var block = knownLive.Pop(); + + if (block.HeadReachable) + { + // already seen this block + continue; + } + + // we can reach this block, clearly + block.HeadReachable = true; + + if (block.TailReachable) + { + // we can reach all the blocks it might flow to + foreach (var reachableOffset in block.BranchesTo) + { + var reachableBlock = blockLookup[reachableOffset]; + knownLive.Push(reachableBlock); + } + } + } + } + + /// + /// Create blocks from an instruction stream and branches. + /// + /// Each block starts either at the start of the method, immediately after a branch or at a target for a branch, + /// and ends with another branch, another branch target, or the end of the method. + /// + /// "Tail reachability" is also calculated, which is whether the block can ever actually get to its last instruction. + /// + private static List CreateBlocks(Collection instrs, IReadOnlyList branches, string[] doesNotReturnAttributes) + { + // every branch-like instruction starts or stops a block + var branchInstrLocs = branches.ToLookup(i => i.Offset); + var branchInstrOffsets = branchInstrLocs.Select(k => k.Key).ToImmutableHashSet(); + + // every target that might be branched to starts or stops a block + var branchTargetOffsets = branches.SelectMany(b => b.HasMultiTargets ? b.TargetOffsets : new[] { b.TargetOffset }).ToImmutableHashSet(); + + // ending the method is also important + var endOfMethodOffset = instrs.Last().Offset; + + var blocks = new List(); + int? blockStartedAt = null; + Instruction unreachableAfter = null; + foreach (var i in instrs) + { + var offset = i.Offset; + var branchesAtLoc = branchInstrLocs[offset]; + + if (blockStartedAt == null) + { + blockStartedAt = offset; + unreachableAfter = null; + } + + var isBranch = branchInstrOffsets.Contains(offset); + var isFollowedByBranchTarget = i.Next != null && branchTargetOffsets.Contains(i.Next.Offset); + var isEndOfMtd = endOfMethodOffset == offset; + + if (unreachableAfter == null && DoesNotReturn(i, doesNotReturnAttributes)) + { + unreachableAfter = i; + } + + var blockEnds = isBranch || isFollowedByBranchTarget || isEndOfMtd; + if (blockEnds) + { + var nextInstr = i.Next; + var goesTo = + branchesAtLoc.Any() ? + branchesAtLoc.SelectMany( + b => b.HasMultiTargets ? b.TargetOffsets : new[] { b.TargetOffset } + ) : + nextInstr != null ? new[] { nextInstr.Offset } : Enumerable.Empty(); + + blocks.Add(new Block(blockStartedAt.Value, unreachableAfter, goesTo.ToImmutableArray())); + + blockStartedAt = null; + unreachableAfter = null; + } + } + + return blocks; + } + + /// + /// Returns true if the given instruction will never return, + /// and thus subsequent instructions will never be run. + /// + private static bool DoesNotReturn(Instruction instr, string[] doesNotReturnAttributeNames) + { + var opcode = instr.OpCode; + if (opcode != OpCodes.Call && opcode != OpCodes.Callvirt) + { + return false; + } + + var mtd = instr.Operand as MethodReference; + var def = mtd.Resolve(); + + if (def == null || !def.HasCustomAttributes) + { + return false; + } + + foreach (var attr in def.CustomAttributes) + { + if (Array.IndexOf(doesNotReturnAttributeNames, attr.AttributeType.Name) != -1) + { + return true; + } + } + + return false; + } + } } diff --git a/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs b/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs index 3188d6e91..e05c782b1 100644 --- a/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs +++ b/test/coverlet.core.tests/Coverage/InstrumenterHelper.Assertions.cs @@ -336,9 +336,54 @@ public static Document AssertNonInstrumentedLines(this Document document, BuildC int[] lineRange = Enumerable.Range(from, to - from + 1).ToArray(); - if (document.Lines.Select(l => l.Value.Number).Intersect(lineRange).Count() > 0) + return AssertNonInstrumentedLines(document, configuration, lineRange); + } + + public static Document AssertNonInstrumentedLines(this Document document, BuildConfiguration configuration, params int[] lines) + { + if (document is null) + { + throw new ArgumentNullException(nameof(document)); + } + + BuildConfiguration buildConfiguration = GetAssemblyBuildConfiguration(); + + if ((buildConfiguration & configuration) != buildConfiguration) + { + return document; + } + + var unexpectedlyInstrumented = document.Lines.Select(l => l.Value.Number).Intersect(lines); + + if (unexpectedlyInstrumented.Any()) + { + throw new XunitException($"Unexpected instrumented lines, '{string.Join(',', unexpectedlyInstrumented)}'"); + } + + return document; + } + + public static Document AssertInstrumentLines(this Document document, BuildConfiguration configuration, params int[] lines) + { + if (document is null) + { + throw new ArgumentNullException(nameof(document)); + } + + BuildConfiguration buildConfiguration = GetAssemblyBuildConfiguration(); + + if ((buildConfiguration & configuration) != buildConfiguration) + { + return document; + } + + var instrumentedLines = document.Lines.Select(l => l.Value.Number).ToHashSet(); + + var missing = lines.Where(l => !instrumentedLines.Contains(l)); + + if (missing.Any()) { - throw new XunitException($"Unexpected instrumented lines, '{string.Join(',', lineRange)}'"); + throw new XunitException($"Expected lines to be instrumented, '{string.Join(',', missing)}'"); } return document; diff --git a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs index 78e5365b5..89669af2b 100644 --- a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs +++ b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs @@ -18,6 +18,7 @@ using Xunit; using Microsoft.Extensions.DependencyModel; using Microsoft.VisualStudio.TestPlatform; +using Coverlet.Core.Tests; namespace Coverlet.Core.Instrumentation.Tests { @@ -77,7 +78,7 @@ public void TestCoreLibInstrumentation() InstrumentationHelper instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), partialMockFileSystem.Object, _mockLogger.Object, sourceRootTranslator); Instrumenter instrumenter = new Instrumenter(Path.Combine(directory.FullName, files[0]), "_coverlet_instrumented", Array.Empty(), Array.Empty(), Array.Empty(), - Array.Empty(), false, _mockLogger.Object, instrumentationHelper, partialMockFileSystem.Object, sourceRootTranslator, new CecilSymbolHelper()); + Array.Empty(), Array.Empty(), false, _mockLogger.Object, instrumentationHelper, partialMockFileSystem.Object, sourceRootTranslator, new CecilSymbolHelper()); Assert.True(instrumenter.CanInstrument()); InstrumenterResult result = instrumenter.Instrument(); @@ -242,7 +243,7 @@ private InstrumenterTest CreateInstrumentor(bool fakeCoreLibModule = false, stri new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem(), new Mock().Object, new SourceRootTranslator(new Mock().Object, new FileSystem())); module = Path.Combine(directory.FullName, destModule); - Instrumenter instrumenter = new Instrumenter(module, identifier, Array.Empty(), Array.Empty(), Array.Empty(), attributesToIgnore, false, + Instrumenter instrumenter = new Instrumenter(module, identifier, Array.Empty(), Array.Empty(), Array.Empty(), attributesToIgnore, Array.Empty(), false, _mockLogger.Object, instrumentationHelper, new FileSystem(), new SourceRootTranslator(_mockLogger.Object, new FileSystem()), new CecilSymbolHelper()); return new InstrumenterTest { @@ -420,7 +421,7 @@ public void SkipEmbeddedPpdbWithoutLocalSource() new SourceRootTranslator(xunitDll, new Mock().Object, new FileSystem())); Instrumenter instrumenter = new Instrumenter(xunitDll, "_xunit_instrumented", Array.Empty(), Array.Empty(), Array.Empty(), - Array.Empty(), false, loggerMock.Object, instrumentationHelper, new FileSystem(), new SourceRootTranslator(xunitDll, loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); + Array.Empty(), Array.Empty(), false, loggerMock.Object, instrumentationHelper, new FileSystem(), new SourceRootTranslator(xunitDll, loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); Assert.True(instrumentationHelper.HasPdb(xunitDll, out bool embedded)); Assert.True(embedded); Assert.False(instrumenter.CanInstrument()); @@ -433,7 +434,7 @@ public void SkipEmbeddedPpdbWithoutLocalSource() new SourceRootTranslator(sample, new Mock().Object, new FileSystem())); instrumenter = new Instrumenter(sample, "_coverlet_tests_projectsample_empty", Array.Empty(), Array.Empty(), Array.Empty(), - Array.Empty(), false, loggerMock.Object, instrumentationHelper, new FileSystem(), new SourceRootTranslator(sample, loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); + Array.Empty(), Array.Empty(), false, loggerMock.Object, instrumentationHelper, new FileSystem(), new SourceRootTranslator(sample, loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); Assert.True(instrumentationHelper.HasPdb(sample, out embedded)); Assert.False(embedded); @@ -479,7 +480,7 @@ public void SkipPpdbWithoutLocalSource() string sample = Directory.GetFiles(Path.Combine(Directory.GetCurrentDirectory(), "TestAssets"), dllFileName).First(); var loggerMock = new Mock(); Instrumenter instrumenter = new Instrumenter(sample, "_75d9f96508d74def860a568f426ea4a4_instrumented", Array.Empty(), Array.Empty(), Array.Empty(), - Array.Empty(), false, loggerMock.Object, instrumentationHelper, partialMockFileSystem.Object, new SourceRootTranslator(loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); + Array.Empty(), Array.Empty(), false, loggerMock.Object, instrumentationHelper, partialMockFileSystem.Object, new SourceRootTranslator(loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); Assert.True(instrumentationHelper.HasPdb(sample, out bool embedded)); Assert.False(embedded); Assert.False(instrumenter.CanInstrument()); @@ -496,7 +497,7 @@ public void TestInstrument_MissingModule() new SourceRootTranslator(new Mock().Object, new FileSystem())); var instrumenter = new Instrumenter("test", "_test_instrumented", Array.Empty(), Array.Empty(), Array.Empty(), - Array.Empty(), false, loggerMock.Object, instrumentationHelper, new FileSystem(), new SourceRootTranslator(loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); + Array.Empty(), Array.Empty(), false, loggerMock.Object, instrumentationHelper, new FileSystem(), new SourceRootTranslator(loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); Assert.False(instrumenter.CanInstrument()); loggerMock.Verify(l => l.LogWarning(It.IsAny())); } @@ -519,7 +520,7 @@ public void TestInstrument_AssemblyMarkedAsExcludeFromCodeCoverage() new SourceRootTranslator(new Mock().Object, new FileSystem())); Instrumenter instrumenter = new Instrumenter(excludedbyattributeDll, "_xunit_excludedbyattribute", Array.Empty(), Array.Empty(), Array.Empty(), - Array.Empty(), false, loggerMock.Object, instrumentationHelper, partialMockFileSystem.Object, new SourceRootTranslator(loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); + Array.Empty(), Array.Empty(), false, loggerMock.Object, instrumentationHelper, partialMockFileSystem.Object, new SourceRootTranslator(loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); InstrumenterResult result = instrumenter.Instrument(); Assert.Empty(result.Documents); loggerMock.Verify(l => l.LogVerbose(It.IsAny())); @@ -638,5 +639,46 @@ public void TestInstrument_AsyncAwaitInsideMethodWithExcludeAttributeAreExcluded instrumenterTest.Directory.Delete(true); } + + [Fact] + public void TestReachabilityHelper() + { + var allInstrumentableLines = + new[] + { + 7, 8, 12, 13, 14, 15, 16, 19, 20, 22, 23, 24, 25, 26, 27, 29, 30, 33, 34, 36, 39, 40, 41, 42, 44, 45, 49, 50, 52, 53, 55, 56, 58, 59, 61, 62, 64, 65, 68, 69, 72, 73, 75, 78, 79, 80, 82, 83, 86, 87, 88, 91, 92, 95, 96, 98, 99, 101, 102, 103, + 106, 107, 108, 110, 111, 112, 113, 114 + }; + var notReachableLines = + new[] + { + // NoBranches + 15, + 16, + // If + 26, 27, + // Switch + 41, 42, + // Subtle + 79, 80, 88, 96, 98, 99, + // UnreachableBranch + 110, 111, 112, 113, 114 + }; + + var expectedToBeInstrumented = allInstrumentableLines.Except(notReachableLines).ToArray(); + + var instrumenterTest = CreateInstrumentor(); + var result = instrumenterTest.Instrumenter.Instrument(); + + var doc = result.Documents.Values.FirstOrDefault(d => Path.GetFileName(d.Path) == "Instrumentation.DoesNotReturn.cs"); + + // check for instrumented lines + doc.AssertNonInstrumentedLines(BuildConfiguration.Debug, notReachableLines); + doc.AssertInstrumentLines(BuildConfiguration.Debug, expectedToBeInstrumented); + + // todo: check for instrumented branches + + instrumenterTest.Directory.Delete(true); + } } } diff --git a/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs b/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs new file mode 100644 index 000000000..ec28ba225 --- /dev/null +++ b/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs @@ -0,0 +1,116 @@ +namespace Coverlet.Core.Samples.Tests +{ + public class DoesNotReturn + { + [System.Diagnostics.CodeAnalysis.DoesNotReturn] + public int Throws() + { + throw new System.Exception(); + } + + public void NoBranches() + { + System.Console.WriteLine("Before"); + Throws(); + System.Console.WriteLine("After"); // unreachable + } // unreachable + + public void If() + { + System.Console.WriteLine("In-After"); + + if (System.Console.ReadKey().KeyChar == 'Y') + { + System.Console.WriteLine("In-Before"); + Throws(); + System.Console.WriteLine("In-After"); // unreachable + } // unreachable + + System.Console.WriteLine("Out-After"); + } + + public void Switch() + { + System.Console.WriteLine("Out-Before"); + + switch (System.Console.ReadKey().KeyChar) + { + case 'A': + System.Console.WriteLine("In-Before"); + Throws(); + System.Console.WriteLine("In-After"); // should be unreachable + break; // should be unreachable + case 'B': + System.Console.WriteLine("In-Constant-1"); + break; + + // need a number of additional, in order, branches to get a Switch generated + case 'C': + System.Console.WriteLine("In-Constant-1"); + break; + case 'D': + System.Console.WriteLine("In-Constant-1"); + break; + case 'E': + System.Console.WriteLine("In-Constant-1"); + break; + case 'F': + System.Console.WriteLine("In-Constant-1"); + break; + case 'G': + System.Console.WriteLine("In-Constant-1"); + break; + case 'H': + System.Console.WriteLine("In-Constant-1"); + break; + } + + System.Console.WriteLine("Out-After"); + } + + public void Subtle() + { + var key = System.Console.ReadKey(); + + switch (key.KeyChar) + { + case 'A': + Throws(); + System.Console.WriteLine("In-Constant-1"); // unreachable + goto case 'B'; // unreachable + case 'B': + System.Console.WriteLine("In-Constant-2"); + break; + + case 'C': + System.Console.WriteLine("In-Constant-3"); + Throws(); + goto alwayUnreachable; // unreachable + + case 'D': + System.Console.WriteLine("In-Constant-4"); + goto subtlyReachable; + } + + Throws(); + System.Console.WriteLine("Out-Constant-1"); // unreachable + + alwayUnreachable: // unreachable + System.Console.WriteLine("Out-Constant-2"); // unreachable + + subtlyReachable: + System.Console.WriteLine("Out-Constant-3"); + } + + public void UnreachableBranch() + { + var key = System.Console.ReadKey(); + Throws(); + + if (key.KeyChar == 'A') // unreachable + { // unreachable + System.Console.WriteLine("Constant-1"); // unreachable + } // unreachable + } // unreachable + } +} From 8314d660bb857b9c50648cbafdca2d5411872d17 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Mon, 20 Jul 2020 20:21:18 -0400 Subject: [PATCH 02/18] Do a read-only pass through modules to find "does not return" methods before we start instrumenting them. Trying to do it in the same pass inevitably leads to issues with Resolve. Also adds tests for generic methods and generic types, which were not functional before. Has been tested against https://github.com/kevin-montrose/Cesil , and confirmed to work. --- .../Instrumentation/Instrumenter.cs | 189 +++++++++++++++--- .../Instrumentation/InstrumenterTests.cs | 30 ++- .../Samples/Instrumentation.DoesNotReturn.cs | 42 +++- 3 files changed, 218 insertions(+), 43 deletions(-) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index ef1b93f12..b18b8eb34 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -47,6 +47,7 @@ internal class Instrumenter private List<(MethodDefinition, int)> _excludedMethods; private List _excludedCompilerGeneratedTypes; private readonly string[] _doesNotReturnAttributes; + private ReachabilityHelper _reachabilityHelper; public bool SkipModule { get; set; } = false; @@ -187,8 +188,31 @@ private bool Is_System_Threading_Interlocked_CoreLib_Type(TypeDefinition type) return _isCoreLibrary && type.FullName == "System.Threading.Interlocked"; } + // Have to do this before we start writing to a module, as we'll get into file + // locking issues if we do. + private void CreateReachabilityHelper() + { + using (var stream = _fileSystem.NewFileStream(_module, FileMode.Open, FileAccess.Read)) + using (var resolver = new NetstandardAwareAssemblyResolver(_module, _logger)) + { + resolver.AddSearchDirectory(Path.GetDirectoryName(_module)); + var parameters = new ReaderParameters { ReadSymbols = true, AssemblyResolver = resolver }; + if (_isCoreLibrary) + { + parameters.MetadataImporterProvider = new CoreLibMetadataImporterProvider(); + } + + using (var module = ModuleDefinition.ReadModule(stream, parameters)) + { + _reachabilityHelper = ReachabilityHelper.CreateForModule(module, _doesNotReturnAttributes); + } + } + } + private void InstrumentModule() { + CreateReachabilityHelper(); + using (var stream = _fileSystem.NewFileStream(_module, FileMode.Open, FileAccess.ReadWrite)) using (var resolver = new NetstandardAwareAssemblyResolver(_module, _logger)) { @@ -508,7 +532,7 @@ private void InstrumentIL(MethodDefinition method) var branchPoints = _cecilSymbolHelper.GetBranchPoints(method); - var unreachableRanges = ReachabilityHelper.FindUnreachableIL(processor.Body.Instructions, _doesNotReturnAttributes); + var unreachableRanges = _reachabilityHelper.FindUnreachableIL(processor.Body.Instructions); var currentUnreachableRangeIx = 0; for (int n = 0; n < count; n++) @@ -910,7 +934,7 @@ public override string ToString() => $"[{StartOffset}, {EndOffset}]"; } - private class Block + private class BasicBlock { /// /// Offset of the instruction that starts the block @@ -938,7 +962,7 @@ private class Block /// public bool HeadReachable { get; set; } - public Block(int startOffset, Instruction unreachableAfter, ImmutableArray branchesTo) + public BasicBlock(int startOffset, Instruction unreachableAfter, ImmutableArray branchesTo) { StartOffset = startOffset; UnreachableAfter = unreachableAfter; @@ -1075,6 +1099,99 @@ public BranchInstruction(int offset, IEnumerable targetOffset) } ); + private readonly ImmutableHashSet DoesNotReturnMethods; + + private ReachabilityHelper(ImmutableHashSet doesNotReturnMethods) + { + DoesNotReturnMethods = doesNotReturnMethods; + } + + /// + /// Build a ReachabilityHelper for the given module. + /// + /// Predetermines methods that will not return, as + /// indicated by the presense of the given attributes. + /// + public static ReachabilityHelper CreateForModule(ModuleDefinition module, string[] doesNotReturnAttributes) + { + if (doesNotReturnAttributes.Length == 0) + { + return new ReachabilityHelper(ImmutableHashSet.Empty); + } + + var processedMethods = new HashSet(); + var doNotReturn = ImmutableHashSet.CreateBuilder(); + foreach (var type in module.Types) + { + foreach (var mtd in type.Methods) + { + if (mtd.IsNative) + { + continue; + } + + MethodBody body; + try + { + if (!mtd.HasBody) + { + continue; + } + + body = mtd.Body; + } + catch + { + continue; + } + + foreach (var instr in body.Instructions) + { + if (!IsCall(instr, out var calledMtd)) + { + continue; + } + + var token = calledMtd.MetadataToken; + if (!processedMethods.Add(token)) + { + continue; + } + + var mtdDef = calledMtd.Resolve(); + if (mtdDef == null) + { + continue; + } + + if (!mtdDef.HasCustomAttributes) + { + continue; + } + + var hasDoesNotReturnAttribute = false; + foreach (var attr in mtdDef.CustomAttributes) + { + if (Array.IndexOf(doesNotReturnAttributes, attr.AttributeType.Name) != -1) + { + hasDoesNotReturnAttribute = true; + break; + } + } + + if (hasDoesNotReturnAttribute) + { + doNotReturn.Add(token); + } + } + } + } + + var doNoReturnTokens = doNotReturn.ToImmutable(); + + return new ReachabilityHelper(doNoReturnTokens); + } + /// /// Calculates which IL instructions are reachable given an instruction stream and branch points extracted from a method. /// @@ -1097,9 +1214,16 @@ public BranchInstruction(int offset, IEnumerable targetOffset) /// * if it is not head reachable (regardless of tail reachability), no instructions in it are reachable /// * if it is only head reachable, all instructions up to and including the first call to a method that does not return are reachable /// - public static ImmutableArray FindUnreachableIL(Collection instrs, string[] doesNotReturnAttributes) + public ImmutableArray FindUnreachableIL(Collection instrs) { - if (!instrs.Any() || !doesNotReturnAttributes.Any()) + // no instructions, means nothing to... not reach + if (!instrs.Any()) + { + return ImmutableArray.Empty; + } + + // no known methods that do not return, so everything is reachable by definition + if (DoesNotReturnMethods.IsEmpty) { return ImmutableArray.Empty; } @@ -1108,7 +1232,7 @@ public static ImmutableArray FindUnreachableIL(Collection FindUnreachableIL(Collection /// Discovers branches, including unconditional ones, in the given instruction stream. /// - private static ImmutableArray FindBranches(Collection instrs) + private ImmutableArray FindBranches(Collection instrs) { var ret = ImmutableArray.CreateBuilder(); foreach (var i in instrs) @@ -1167,10 +1291,9 @@ private static ImmutableArray FindBranches(Collection - /// Calculates which ranges of IL are unreachable, given blocks which have head and tail reachability calculated - /// and an insturction stream. + /// Calculates which ranges of IL are unreachable, given blocks which have head and tail reachability calculated. /// - private static ImmutableArray DetermineUnreachableRanges(IReadOnlyList blocks, int lastInstructionOffset) + private ImmutableArray DetermineUnreachableRanges(IReadOnlyList blocks, int lastInstructionOffset) { var ret = ImmutableArray.CreateBuilder(); @@ -1223,13 +1346,13 @@ private static ImmutableArray DetermineUnreachableRanges(IRead /// /// "Tail reachability" will have already been determined in CreateBlocks. /// - private static void DetermineHeadReachability(IEnumerable blocks) + private void DetermineHeadReachability(IEnumerable blocks) { var blockLookup = blocks.ToImmutableDictionary(b => b.StartOffset); var headBlock = blockLookup[0]; - var knownLive = new Stack(); + var knownLive = new Stack(); knownLive.Push(headBlock); while (knownLive.Count > 0) @@ -1258,14 +1381,14 @@ private static void DetermineHeadReachability(IEnumerable blocks) } /// - /// Create blocks from an instruction stream and branches. + /// Create BasicBlocks from an instruction stream and branches. /// /// Each block starts either at the start of the method, immediately after a branch or at a target for a branch, /// and ends with another branch, another branch target, or the end of the method. /// - /// "Tail reachability" is also calculated, which is whether the block can ever actually get to its last instruction. + /// "Tail reachability" is also calculated, which is whether the block can ever actually get past its last instruction. /// - private static List CreateBlocks(Collection instrs, IReadOnlyList branches, string[] doesNotReturnAttributes) + private List CreateBasicBlocks(Collection instrs, IReadOnlyList branches) { // every branch-like instruction starts or stops a block var branchInstrLocs = branches.ToLookup(i => i.Offset); @@ -1277,7 +1400,7 @@ private static List CreateBlocks(Collection instrs, IReadOnl // ending the method is also important var endOfMethodOffset = instrs.Last().Offset; - var blocks = new List(); + var blocks = new List(); int? blockStartedAt = null; Instruction unreachableAfter = null; foreach (var i in instrs) @@ -1295,7 +1418,7 @@ private static List CreateBlocks(Collection instrs, IReadOnl var isFollowedByBranchTarget = i.Next != null && branchTargetOffsets.Contains(i.Next.Offset); var isEndOfMtd = endOfMethodOffset == offset; - if (unreachableAfter == null && DoesNotReturn(i, doesNotReturnAttributes)) + if (unreachableAfter == null && DoesNotReturn(i)) { unreachableAfter = i; } @@ -1311,7 +1434,7 @@ private static List CreateBlocks(Collection instrs, IReadOnl ) : nextInstr != null ? new[] { nextInstr.Offset } : Enumerable.Empty(); - blocks.Add(new Block(blockStartedAt.Value, unreachableAfter, goesTo.ToImmutableArray())); + blocks.Add(new BasicBlock(blockStartedAt.Value, unreachableAfter, goesTo.ToImmutableArray())); blockStartedAt = null; unreachableAfter = null; @@ -1325,31 +1448,33 @@ private static List CreateBlocks(Collection instrs, IReadOnl /// Returns true if the given instruction will never return, /// and thus subsequent instructions will never be run. /// - private static bool DoesNotReturn(Instruction instr, string[] doesNotReturnAttributeNames) + private bool DoesNotReturn(Instruction instr) { - var opcode = instr.OpCode; - if (opcode != OpCodes.Call && opcode != OpCodes.Callvirt) + if (!IsCall(instr, out var mtd)) { return false; } - var mtd = instr.Operand as MethodReference; - var def = mtd.Resolve(); + return DoesNotReturnMethods.Contains(mtd.MetadataToken); + } - if (def == null || !def.HasCustomAttributes) + /// + /// Returns true if the given instruction is a Call or Callvirt. + /// + /// If it is a call, extracts the MethodReference that is being called. + /// + private static bool IsCall(Instruction instr, out MethodReference mtd) + { + var opcode = instr.OpCode; + if (opcode != OpCodes.Call && opcode != OpCodes.Callvirt) { + mtd = null; return false; } - foreach (var attr in def.CustomAttributes) - { - if (Array.IndexOf(doesNotReturnAttributeNames, attr.AttributeType.Name) != -1) - { - return true; - } - } + mtd = (MethodReference)instr.Operand; - return false; + return true; } } } diff --git a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs index 89669af2b..30b321257 100644 --- a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs +++ b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs @@ -646,8 +646,26 @@ public void TestReachabilityHelper() var allInstrumentableLines = new[] { - 7, 8, 12, 13, 14, 15, 16, 19, 20, 22, 23, 24, 25, 26, 27, 29, 30, 33, 34, 36, 39, 40, 41, 42, 44, 45, 49, 50, 52, 53, 55, 56, 58, 59, 61, 62, 64, 65, 68, 69, 72, 73, 75, 78, 79, 80, 82, 83, 86, 87, 88, 91, 92, 95, 96, 98, 99, 101, 102, 103, - 106, 107, 108, 110, 111, 112, 113, 114 + // Throws + 7, 8, + // NoBranches + 12, 13, 14, 15, 16, + // If + 19, 20, 22, 23, 24, 25, 26, 27, 29, 30, + // Switch + 33, 34, 36, 39, 40, 41, 42, 44, 45, 49, 50, 52, 53, 55, 56, 58, 59, 61, 62, 64, 65, 68, 69, + // Subtle + 72, 73, 75, 78, 79, 80, 82, 83, 86, 87, 88, 91, 92, 95, 96, 98, 99, 101, 102, 103, + // UnreachableBranch + 106, 107, 108, 110, 111, 112, 113, 114, + // ThrowsGeneric + 118, 119, + // CallsGenericMethodDoesNotReturn + 124, 125, 126, 127, 128, + // AlsoThrows + 134, 135, + // CallsGenericClassDoesNotReturn + 140, 141, 142, 143, 144 }; var notReachableLines = new[] @@ -662,7 +680,11 @@ public void TestReachabilityHelper() // Subtle 79, 80, 88, 96, 98, 99, // UnreachableBranch - 110, 111, 112, 113, 114 + 110, 111, 112, 113, 114, + // CallsGenericMethodDoesNotReturn + 127, 128, + // CallsGenericClassDoesNotReturn + 143, 144 }; var expectedToBeInstrumented = allInstrumentableLines.Except(notReachableLines).ToArray(); @@ -676,8 +698,6 @@ public void TestReachabilityHelper() doc.AssertNonInstrumentedLines(BuildConfiguration.Debug, notReachableLines); doc.AssertInstrumentLines(BuildConfiguration.Debug, expectedToBeInstrumented); - // todo: check for instrumented branches - instrumenterTest.Directory.Delete(true); } } diff --git a/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs b/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs index ec28ba225..0b24f41b4 100644 --- a/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs +++ b/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs @@ -46,22 +46,22 @@ public void Switch() // need a number of additional, in order, branches to get a Switch generated case 'C': - System.Console.WriteLine("In-Constant-1"); + System.Console.WriteLine("In-Constant-2"); break; case 'D': - System.Console.WriteLine("In-Constant-1"); + System.Console.WriteLine("In-Constant-3"); break; case 'E': - System.Console.WriteLine("In-Constant-1"); + System.Console.WriteLine("In-Constant-4"); break; case 'F': - System.Console.WriteLine("In-Constant-1"); + System.Console.WriteLine("In-Constant-5"); break; case 'G': - System.Console.WriteLine("In-Constant-1"); + System.Console.WriteLine("In-Constant-6"); break; case 'H': - System.Console.WriteLine("In-Constant-1"); + System.Console.WriteLine("In-Constant-7"); break; } @@ -112,5 +112,35 @@ public void UnreachableBranch() System.Console.WriteLine("Constant-1"); // unreachable } // unreachable } // unreachable + + [System.Diagnostics.CodeAnalysis.DoesNotReturn] + public void ThrowsGeneric() + { + throw new System.Exception(typeof(T).Name); + } + + + public void CallsGenericMethodDoesNotReturn() + { + System.Console.WriteLine("Constant-1"); + ThrowsGeneric(); + System.Console.WriteLine("Constant-2"); // unreachable + } + + private class GenericClass + { + [System.Diagnostics.CodeAnalysis.DoesNotReturn] + public static void AlsoThrows() + { + throw new System.Exception(typeof(T).Name); + } + } + + public void CallsGenericClassDoesNotReturn() + { + System.Console.WriteLine("Constant-1"); + GenericClass.AlsoThrows(); + System.Console.WriteLine("Constant-2"); // unreachable + } } } From 575edcbadfd80aa73cb3d843d5b590dc307ccabe Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Sat, 25 Jul 2020 15:56:08 -0400 Subject: [PATCH 03/18] .Resolve() can still fail, just for non-self-inflicted reasons; handle that gracefully and log as a warning. Also log when methods are first found to not return --- .../Instrumentation/Instrumenter.cs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index b18b8eb34..fbd865894 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -204,7 +204,7 @@ private void CreateReachabilityHelper() using (var module = ModuleDefinition.ReadModule(stream, parameters)) { - _reachabilityHelper = ReachabilityHelper.CreateForModule(module, _doesNotReturnAttributes); + _reachabilityHelper = ReachabilityHelper.CreateForModule(module, _doesNotReturnAttributes, _logger); } } } @@ -1112,7 +1112,7 @@ private ReachabilityHelper(ImmutableHashSet doesNotReturnMethods) /// Predetermines methods that will not return, as /// indicated by the presense of the given attributes. /// - public static ReachabilityHelper CreateForModule(ModuleDefinition module, string[] doesNotReturnAttributes) + public static ReachabilityHelper CreateForModule(ModuleDefinition module, string[] doesNotReturnAttributes, ILogger logger) { if (doesNotReturnAttributes.Length == 0) { @@ -1158,7 +1158,17 @@ public static ReachabilityHelper CreateForModule(ModuleDefinition module, string continue; } - var mtdDef = calledMtd.Resolve(); + MethodDefinition mtdDef; + try + { + mtdDef = calledMtd.Resolve(); + } + catch + { + logger.LogWarning($"Unable to resolve method reference \"{calledMtd.FullName}\", assuming calls to will return"); + mtdDef = null; + } + if (mtdDef == null) { continue; @@ -1181,6 +1191,7 @@ public static ReachabilityHelper CreateForModule(ModuleDefinition module, string if (hasDoesNotReturnAttribute) { + logger.LogVerbose($"Determined call to \"{calledMtd.FullName}\" will not return"); doNotReturn.Add(token); } } From 8524e7200e18907d83f9ad724c2c46ffaeefd1a6 Mon Sep 17 00:00:00 2001 From: Marco Rossignoli Date: Wed, 19 Aug 2020 09:42:06 +0200 Subject: [PATCH 04/18] add test sample --- .../Coverage/CoverageTests.DoesNotReturn.cs | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 test/coverlet.core.tests/Coverage/CoverageTests.DoesNotReturn.cs diff --git a/test/coverlet.core.tests/Coverage/CoverageTests.DoesNotReturn.cs b/test/coverlet.core.tests/Coverage/CoverageTests.DoesNotReturn.cs new file mode 100644 index 000000000..bf86416b3 --- /dev/null +++ b/test/coverlet.core.tests/Coverage/CoverageTests.DoesNotReturn.cs @@ -0,0 +1,51 @@ +using System.IO; +using System.Threading.Tasks; + +using Coverlet.Core.Samples.Tests; +using Tmds.Utils; +using Xunit; + +namespace Coverlet.Core.Tests +{ + public partial class CoverageTests : ExternalProcessExecutionTest + { + [Fact] + public void DoesNotReturnSample() + { + string path = Path.GetTempFileName(); + try + { + // After debugging and before to push on PR change to Run for out of process test on CI + // FunctionExecutor.Run(async (string[] pathSerialize) => + + // For in-process debugging + FunctionExecutor.RunInProcess(async (string[] pathSerialize) => + { + CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run(instance => + { + try + { + instance.NoBranches(); // call method to test it's a dynamic + } + catch + { + // will throw do nothing + } + return Task.CompletedTask; + }, persistPrepareResultToFile: pathSerialize[0]); + return 0; + }, new string[] { path }); + + TestInstrumentationHelper.GetCoverageResult(path) + .GenerateReport(show: true) // remove at the end of debugging it allows to open and show report for fast check + .Document("Instrumentation.DoesNotReturn.cs") // chose cs source of samples where check rows + // .AssertBranchesCovered(... Add coverage check + ; + } + finally + { + File.Delete(path); + } + } + } +} \ No newline at end of file From ef9eb957ff940b710f8e5b8efaa4ccb98cc326e3 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Thu, 20 Aug 2020 19:51:19 -0400 Subject: [PATCH 05/18] move Reachability related objects to new file and namespace per https://github.com/coverlet-coverage/coverlet/pull/904#issuecomment-675925575 --- .../Instrumentation/Instrumenter.cs | 583 +---------------- .../Instrumentation/ReachabilityHelper.cs | 592 ++++++++++++++++++ 2 files changed, 593 insertions(+), 582 deletions(-) create mode 100644 src/coverlet.core/Instrumentation/ReachabilityHelper.cs diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index fbd865894..cae5a3e13 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -6,7 +6,7 @@ using System.IO; using System.Linq; using System.Runtime.CompilerServices; - +using coverlet.core.Instrumentation.Reachability; using Coverlet.Core.Abstractions; using Coverlet.Core.Attributes; using Coverlet.Core.Symbols; @@ -907,585 +907,4 @@ public bool Exclude(string sourceFile) return _matcher.Match(Path.IsPathRooted(sourceFile) ? sourceFile.Substring(Path.GetPathRoot(sourceFile).Length) : sourceFile).HasMatches; } } - - /// - /// Helper for find unreachable IL instructions. - /// - internal class ReachabilityHelper - { - internal readonly struct UnreachableRange - { - /// - /// Offset of first unreachable instruction. - /// - public int StartOffset { get; } - /// - /// Offset of last unreachable instruction. - /// - public int EndOffset { get; } - - public UnreachableRange(int start, int end) - { - StartOffset = start; - EndOffset = end; - } - - public override string ToString() - => $"[{StartOffset}, {EndOffset}]"; - } - - private class BasicBlock - { - /// - /// Offset of the instruction that starts the block - /// - public int StartOffset { get; } - /// - /// Whether it is possible for control to flow past the end of the block, - /// ie. whether it's tail is reachable - /// - public bool TailReachable => UnreachableAfter == null; - /// - /// If control flows to the end of the block, where it can flow to - /// - public ImmutableArray BranchesTo { get; } - - /// - /// If this block contains a call(i) to a method that does not return - /// this will be the first such call. - /// - public Instruction UnreachableAfter { get; } - - /// - /// Mutable, records whether control can flow into the block, - /// ie. whether it's head is reachable - /// - public bool HeadReachable { get; set; } - - public BasicBlock(int startOffset, Instruction unreachableAfter, ImmutableArray branchesTo) - { - StartOffset = startOffset; - UnreachableAfter = unreachableAfter; - BranchesTo = branchesTo; - } - - public override string ToString() - => $"{nameof(StartOffset)}={StartOffset}, {nameof(HeadReachable)}={HeadReachable}, {nameof(TailReachable)}={TailReachable}, {nameof(BranchesTo)}=({string.Join(", ", BranchesTo)}), {nameof(UnreachableAfter)}={UnreachableAfter}"; - } - - /// - /// Represents an Instruction that transitions control flow (ie. branches). - /// - /// This is _different_ from other branch types, like Branch and BranchPoint - /// because it includes unconditional branches too. - /// - private readonly struct BranchInstruction - { - /// - /// Location of the branching instruction - /// - public int Offset { get; } - - public bool HasMultiTargets => _TargetOffsets.Any(); - - private readonly int _TargetOffset; - - /// - /// Target of the branch, assuming it has a single target. - /// - /// It is illegal to access this if there are multiple targets. - /// - public int TargetOffset - { - get - { - if (HasMultiTargets) - { - throw new InvalidOperationException($"{HasMultiTargets} is true"); - } - - return _TargetOffset; - } - } - - private readonly IEnumerable _TargetOffsets; - - /// - /// Targets of the branch, assuming it has multiple targets. - /// - /// It is illegal to access this if there is a single target. - /// - public IEnumerable TargetOffsets - { - get - { - if (!HasMultiTargets) - { - throw new InvalidOperationException($"{HasMultiTargets} is false"); - } - - return _TargetOffsets; - } - } - - public BranchInstruction(int offset, int targetOffset) - { - Offset = offset; - _TargetOffset = targetOffset; - _TargetOffsets = Enumerable.Empty(); - } - - public BranchInstruction(int offset, IEnumerable targetOffset) - { - if (targetOffset.Count() < 1) - { - throw new ArgumentException("Must have at least 2 entries", nameof(targetOffset)); - } - - Offset = offset; - _TargetOffset = -1; - _TargetOffsets = targetOffset; - } - } - - /// - /// OpCodes that transfer control code, even if they do not - /// introduce branch points. - /// - private static readonly ImmutableHashSet BRANCH_OPCODES = - ImmutableHashSet.CreateRange( - new[] - { - OpCodes.Beq, - OpCodes.Beq_S, - OpCodes.Bge, - OpCodes.Bge_S, - OpCodes.Bge_Un, - OpCodes.Bge_Un_S, - OpCodes.Bgt, - OpCodes.Bgt_S, - OpCodes.Bgt_Un, - OpCodes.Bgt_Un_S, - OpCodes.Ble, - OpCodes.Ble_S, - OpCodes.Ble_Un, - OpCodes.Ble_Un_S, - OpCodes.Blt, - OpCodes.Blt_S, - OpCodes.Blt_Un, - OpCodes.Blt_Un_S, - OpCodes.Bne_Un, - OpCodes.Bne_Un_S, - OpCodes.Br, - OpCodes.Br_S, - OpCodes.Brfalse, - OpCodes.Brfalse_S, - OpCodes.Brtrue, - OpCodes.Brtrue_S, - OpCodes.Switch - } - ); - - /// - /// OpCodes that unconditionally transfer control, so there - /// is not "fall through" branch target. - /// - private static readonly ImmutableHashSet UNCONDITIONAL_BRANCH_OPCODES = - ImmutableHashSet.CreateRange( - new[] - { - OpCodes.Br, - OpCodes.Br_S - } - ); - - private readonly ImmutableHashSet DoesNotReturnMethods; - - private ReachabilityHelper(ImmutableHashSet doesNotReturnMethods) - { - DoesNotReturnMethods = doesNotReturnMethods; - } - - /// - /// Build a ReachabilityHelper for the given module. - /// - /// Predetermines methods that will not return, as - /// indicated by the presense of the given attributes. - /// - public static ReachabilityHelper CreateForModule(ModuleDefinition module, string[] doesNotReturnAttributes, ILogger logger) - { - if (doesNotReturnAttributes.Length == 0) - { - return new ReachabilityHelper(ImmutableHashSet.Empty); - } - - var processedMethods = new HashSet(); - var doNotReturn = ImmutableHashSet.CreateBuilder(); - foreach (var type in module.Types) - { - foreach (var mtd in type.Methods) - { - if (mtd.IsNative) - { - continue; - } - - MethodBody body; - try - { - if (!mtd.HasBody) - { - continue; - } - - body = mtd.Body; - } - catch - { - continue; - } - - foreach (var instr in body.Instructions) - { - if (!IsCall(instr, out var calledMtd)) - { - continue; - } - - var token = calledMtd.MetadataToken; - if (!processedMethods.Add(token)) - { - continue; - } - - MethodDefinition mtdDef; - try - { - mtdDef = calledMtd.Resolve(); - } - catch - { - logger.LogWarning($"Unable to resolve method reference \"{calledMtd.FullName}\", assuming calls to will return"); - mtdDef = null; - } - - if (mtdDef == null) - { - continue; - } - - if (!mtdDef.HasCustomAttributes) - { - continue; - } - - var hasDoesNotReturnAttribute = false; - foreach (var attr in mtdDef.CustomAttributes) - { - if (Array.IndexOf(doesNotReturnAttributes, attr.AttributeType.Name) != -1) - { - hasDoesNotReturnAttribute = true; - break; - } - } - - if (hasDoesNotReturnAttribute) - { - logger.LogVerbose($"Determined call to \"{calledMtd.FullName}\" will not return"); - doNotReturn.Add(token); - } - } - } - } - - var doNoReturnTokens = doNotReturn.ToImmutable(); - - return new ReachabilityHelper(doNoReturnTokens); - } - - /// - /// Calculates which IL instructions are reachable given an instruction stream and branch points extracted from a method. - /// - /// The algorithm works like so: - /// 1. determine the "blocks" that make up a function - /// * A block starts with either the start of the method, or a branch _target_ - /// * blocks are "where some other code might jump to" - /// * blocks end with either another branch, another branch target, or the end of the method - /// * this means blocks contain no control flow, except (maybe) as the very last instruction - /// 2. blocks have head and tail reachability - /// * a block has head reachablility if some other block that is reachable can branch to it - /// * a block has tail reachability if it contains no calls to methods that never return - /// 4. push the first block onto a stack - /// 5. while the stack is not empty - /// a. pop a block off the stack - /// b. give it head reachability - /// c. if the pop'd block is tail reachable, push the blocks it can branch to onto the stack - /// 6. consider each block - /// * if it is head and tail reachable, all instructions in it are reachable - /// * if it is not head reachable (regardless of tail reachability), no instructions in it are reachable - /// * if it is only head reachable, all instructions up to and including the first call to a method that does not return are reachable - /// - public ImmutableArray FindUnreachableIL(Collection instrs) - { - // no instructions, means nothing to... not reach - if (!instrs.Any()) - { - return ImmutableArray.Empty; - } - - // no known methods that do not return, so everything is reachable by definition - if (DoesNotReturnMethods.IsEmpty) - { - return ImmutableArray.Empty; - } - - var brs = FindBranches(instrs); - - var lastInstr = instrs.Last(); - - var blocks = CreateBasicBlocks(instrs, brs); - DetermineHeadReachability(blocks); - return DetermineUnreachableRanges(blocks, lastInstr.Offset); - } - - /// - /// Discovers branches, including unconditional ones, in the given instruction stream. - /// - private ImmutableArray FindBranches(Collection instrs) - { - var ret = ImmutableArray.CreateBuilder(); - foreach (var i in instrs) - { - if (BRANCH_OPCODES.Contains(i.OpCode)) - { - int? singleTargetOffset; - IEnumerable multiTargetOffsets; - - if (i.Operand is Instruction[] multiTarget) - { - // it's a switch - singleTargetOffset = null; - multiTargetOffsets = multiTarget.Select(t => t.Offset).Concat(new[] { i.Next.Offset }).Distinct().ToList(); - } - else if (i.Operand is Instruction singleTarget) - { - // it's any of the B.*(_S)? instructions - - if (UNCONDITIONAL_BRANCH_OPCODES.Contains(i.OpCode)) - { - multiTargetOffsets = null; - singleTargetOffset = singleTarget.Offset; - } - else - { - singleTargetOffset = null; - multiTargetOffsets = new[] { i.Next.Offset, singleTarget.Offset }; - } - } - else - { - throw new InvalidOperationException($"Unexpected operand when processing branch {i.Operand}: {i.Operand}"); - } - - if (singleTargetOffset != null) - { - ret.Add(new BranchInstruction(i.Offset, singleTargetOffset.Value)); - } - else - { - ret.Add(new BranchInstruction(i.Offset, multiTargetOffsets)); - } - } - } - - return ret.ToImmutable(); - } - - /// - /// Calculates which ranges of IL are unreachable, given blocks which have head and tail reachability calculated. - /// - private ImmutableArray DetermineUnreachableRanges(IReadOnlyList blocks, int lastInstructionOffset) - { - var ret = ImmutableArray.CreateBuilder(); - - var endOfMethodOffset = lastInstructionOffset + 1; // add 1 so we point _past_ the end of the method - - for (var curBlockIx = 0; curBlockIx < blocks.Count; curBlockIx++) - { - var curBlock = blocks[curBlockIx]; - - int endOfCurBlockOffset; - if (curBlockIx == blocks.Count - 1) - { - endOfCurBlockOffset = endOfMethodOffset; - } - else - { - endOfCurBlockOffset = blocks[curBlockIx + 1].StartOffset - 1; // minus 1 so we don't include anything of the following block - } - - if (curBlock.HeadReachable) - { - if (curBlock.TailReachable) - { - // it's all reachable - continue; - } - - // tail isn't reachable, which means there's a call to something that doesn't return... - var doesNotReturnInstr = curBlock.UnreachableAfter; - - // and it's everything _after_ the following instruction that is unreachable - // so record the following instruction through the end of the block - var followingInstr = doesNotReturnInstr.Next; - - ret.Add(new UnreachableRange(followingInstr.Offset, endOfCurBlockOffset)); - } - else - { - // none of it is reachable - ret.Add(new UnreachableRange(curBlock.StartOffset, endOfCurBlockOffset)); - } - } - - return ret.ToImmutable(); - } - - /// - /// Process all the blocks and determine if their first instruction is reachable, - /// that is if they have "head reachability". - /// - /// "Tail reachability" will have already been determined in CreateBlocks. - /// - private void DetermineHeadReachability(IEnumerable blocks) - { - var blockLookup = blocks.ToImmutableDictionary(b => b.StartOffset); - - var headBlock = blockLookup[0]; - - var knownLive = new Stack(); - knownLive.Push(headBlock); - - while (knownLive.Count > 0) - { - var block = knownLive.Pop(); - - if (block.HeadReachable) - { - // already seen this block - continue; - } - - // we can reach this block, clearly - block.HeadReachable = true; - - if (block.TailReachable) - { - // we can reach all the blocks it might flow to - foreach (var reachableOffset in block.BranchesTo) - { - var reachableBlock = blockLookup[reachableOffset]; - knownLive.Push(reachableBlock); - } - } - } - } - - /// - /// Create BasicBlocks from an instruction stream and branches. - /// - /// Each block starts either at the start of the method, immediately after a branch or at a target for a branch, - /// and ends with another branch, another branch target, or the end of the method. - /// - /// "Tail reachability" is also calculated, which is whether the block can ever actually get past its last instruction. - /// - private List CreateBasicBlocks(Collection instrs, IReadOnlyList branches) - { - // every branch-like instruction starts or stops a block - var branchInstrLocs = branches.ToLookup(i => i.Offset); - var branchInstrOffsets = branchInstrLocs.Select(k => k.Key).ToImmutableHashSet(); - - // every target that might be branched to starts or stops a block - var branchTargetOffsets = branches.SelectMany(b => b.HasMultiTargets ? b.TargetOffsets : new[] { b.TargetOffset }).ToImmutableHashSet(); - - // ending the method is also important - var endOfMethodOffset = instrs.Last().Offset; - - var blocks = new List(); - int? blockStartedAt = null; - Instruction unreachableAfter = null; - foreach (var i in instrs) - { - var offset = i.Offset; - var branchesAtLoc = branchInstrLocs[offset]; - - if (blockStartedAt == null) - { - blockStartedAt = offset; - unreachableAfter = null; - } - - var isBranch = branchInstrOffsets.Contains(offset); - var isFollowedByBranchTarget = i.Next != null && branchTargetOffsets.Contains(i.Next.Offset); - var isEndOfMtd = endOfMethodOffset == offset; - - if (unreachableAfter == null && DoesNotReturn(i)) - { - unreachableAfter = i; - } - - var blockEnds = isBranch || isFollowedByBranchTarget || isEndOfMtd; - if (blockEnds) - { - var nextInstr = i.Next; - var goesTo = - branchesAtLoc.Any() ? - branchesAtLoc.SelectMany( - b => b.HasMultiTargets ? b.TargetOffsets : new[] { b.TargetOffset } - ) : - nextInstr != null ? new[] { nextInstr.Offset } : Enumerable.Empty(); - - blocks.Add(new BasicBlock(blockStartedAt.Value, unreachableAfter, goesTo.ToImmutableArray())); - - blockStartedAt = null; - unreachableAfter = null; - } - } - - return blocks; - } - - /// - /// Returns true if the given instruction will never return, - /// and thus subsequent instructions will never be run. - /// - private bool DoesNotReturn(Instruction instr) - { - if (!IsCall(instr, out var mtd)) - { - return false; - } - - return DoesNotReturnMethods.Contains(mtd.MetadataToken); - } - - /// - /// Returns true if the given instruction is a Call or Callvirt. - /// - /// If it is a call, extracts the MethodReference that is being called. - /// - private static bool IsCall(Instruction instr, out MethodReference mtd) - { - var opcode = instr.OpCode; - if (opcode != OpCodes.Call && opcode != OpCodes.Callvirt) - { - mtd = null; - return false; - } - - mtd = (MethodReference)instr.Operand; - - return true; - } - } } diff --git a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs new file mode 100644 index 000000000..b7ff63b92 --- /dev/null +++ b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs @@ -0,0 +1,592 @@ +using Coverlet.Core.Abstractions; +using Mono.Cecil; +using Mono.Cecil.Cil; +using Mono.Collections.Generic; +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; + +namespace coverlet.core.Instrumentation.Reachability +{ + /// + /// Helper for find unreachable IL instructions. + /// + internal class ReachabilityHelper + { + internal readonly struct UnreachableRange + { + /// + /// Offset of first unreachable instruction. + /// + public int StartOffset { get; } + /// + /// Offset of last unreachable instruction. + /// + public int EndOffset { get; } + + public UnreachableRange(int start, int end) + { + StartOffset = start; + EndOffset = end; + } + + public override string ToString() + => $"[{StartOffset}, {EndOffset}]"; + } + + private class BasicBlock + { + /// + /// Offset of the instruction that starts the block + /// + public int StartOffset { get; } + /// + /// Whether it is possible for control to flow past the end of the block, + /// ie. whether it's tail is reachable + /// + public bool TailReachable => UnreachableAfter == null; + /// + /// If control flows to the end of the block, where it can flow to + /// + public ImmutableArray BranchesTo { get; } + + /// + /// If this block contains a call(i) to a method that does not return + /// this will be the first such call. + /// + public Instruction UnreachableAfter { get; } + + /// + /// Mutable, records whether control can flow into the block, + /// ie. whether it's head is reachable + /// + public bool HeadReachable { get; set; } + + public BasicBlock(int startOffset, Instruction unreachableAfter, ImmutableArray branchesTo) + { + StartOffset = startOffset; + UnreachableAfter = unreachableAfter; + BranchesTo = branchesTo; + } + + public override string ToString() + => $"{nameof(StartOffset)}={StartOffset}, {nameof(HeadReachable)}={HeadReachable}, {nameof(TailReachable)}={TailReachable}, {nameof(BranchesTo)}=({string.Join(", ", BranchesTo)}), {nameof(UnreachableAfter)}={UnreachableAfter}"; + } + + /// + /// Represents an Instruction that transitions control flow (ie. branches). + /// + /// This is _different_ from other branch types, like Branch and BranchPoint + /// because it includes unconditional branches too. + /// + private readonly struct BranchInstruction + { + /// + /// Location of the branching instruction + /// + public int Offset { get; } + + public bool HasMultiTargets => _TargetOffsets.Any(); + + private readonly int _TargetOffset; + + /// + /// Target of the branch, assuming it has a single target. + /// + /// It is illegal to access this if there are multiple targets. + /// + public int TargetOffset + { + get + { + if (HasMultiTargets) + { + throw new InvalidOperationException($"{HasMultiTargets} is true"); + } + + return _TargetOffset; + } + } + + private readonly IEnumerable _TargetOffsets; + + /// + /// Targets of the branch, assuming it has multiple targets. + /// + /// It is illegal to access this if there is a single target. + /// + public IEnumerable TargetOffsets + { + get + { + if (!HasMultiTargets) + { + throw new InvalidOperationException($"{HasMultiTargets} is false"); + } + + return _TargetOffsets; + } + } + + public BranchInstruction(int offset, int targetOffset) + { + Offset = offset; + _TargetOffset = targetOffset; + _TargetOffsets = Enumerable.Empty(); + } + + public BranchInstruction(int offset, IEnumerable targetOffset) + { + if (targetOffset.Count() < 1) + { + throw new ArgumentException("Must have at least 2 entries", nameof(targetOffset)); + } + + Offset = offset; + _TargetOffset = -1; + _TargetOffsets = targetOffset; + } + } + + /// + /// OpCodes that transfer control code, even if they do not + /// introduce branch points. + /// + private static readonly ImmutableHashSet BRANCH_OPCODES = + ImmutableHashSet.CreateRange( + new[] + { + OpCodes.Beq, + OpCodes.Beq_S, + OpCodes.Bge, + OpCodes.Bge_S, + OpCodes.Bge_Un, + OpCodes.Bge_Un_S, + OpCodes.Bgt, + OpCodes.Bgt_S, + OpCodes.Bgt_Un, + OpCodes.Bgt_Un_S, + OpCodes.Ble, + OpCodes.Ble_S, + OpCodes.Ble_Un, + OpCodes.Ble_Un_S, + OpCodes.Blt, + OpCodes.Blt_S, + OpCodes.Blt_Un, + OpCodes.Blt_Un_S, + OpCodes.Bne_Un, + OpCodes.Bne_Un_S, + OpCodes.Br, + OpCodes.Br_S, + OpCodes.Brfalse, + OpCodes.Brfalse_S, + OpCodes.Brtrue, + OpCodes.Brtrue_S, + OpCodes.Switch + } + ); + + /// + /// OpCodes that unconditionally transfer control, so there + /// is not "fall through" branch target. + /// + private static readonly ImmutableHashSet UNCONDITIONAL_BRANCH_OPCODES = + ImmutableHashSet.CreateRange( + new[] + { + OpCodes.Br, + OpCodes.Br_S + } + ); + + private readonly ImmutableHashSet DoesNotReturnMethods; + + private ReachabilityHelper(ImmutableHashSet doesNotReturnMethods) + { + DoesNotReturnMethods = doesNotReturnMethods; + } + + /// + /// Build a ReachabilityHelper for the given module. + /// + /// Predetermines methods that will not return, as + /// indicated by the presense of the given attributes. + /// + public static ReachabilityHelper CreateForModule(ModuleDefinition module, string[] doesNotReturnAttributes, ILogger logger) + { + if (doesNotReturnAttributes.Length == 0) + { + return new ReachabilityHelper(ImmutableHashSet.Empty); + } + + var processedMethods = new HashSet(); + var doNotReturn = ImmutableHashSet.CreateBuilder(); + foreach (var type in module.Types) + { + foreach (var mtd in type.Methods) + { + if (mtd.IsNative) + { + continue; + } + + MethodBody body; + try + { + if (!mtd.HasBody) + { + continue; + } + + body = mtd.Body; + } + catch + { + continue; + } + + foreach (var instr in body.Instructions) + { + if (!IsCall(instr, out var calledMtd)) + { + continue; + } + + var token = calledMtd.MetadataToken; + if (!processedMethods.Add(token)) + { + continue; + } + + MethodDefinition mtdDef; + try + { + mtdDef = calledMtd.Resolve(); + } + catch + { + logger.LogWarning($"Unable to resolve method reference \"{calledMtd.FullName}\", assuming calls to will return"); + mtdDef = null; + } + + if (mtdDef == null) + { + continue; + } + + if (!mtdDef.HasCustomAttributes) + { + continue; + } + + var hasDoesNotReturnAttribute = false; + foreach (var attr in mtdDef.CustomAttributes) + { + if (Array.IndexOf(doesNotReturnAttributes, attr.AttributeType.Name) != -1) + { + hasDoesNotReturnAttribute = true; + break; + } + } + + if (hasDoesNotReturnAttribute) + { + logger.LogVerbose($"Determined call to \"{calledMtd.FullName}\" will not return"); + doNotReturn.Add(token); + } + } + } + } + + var doNoReturnTokens = doNotReturn.ToImmutable(); + + return new ReachabilityHelper(doNoReturnTokens); + } + + /// + /// Calculates which IL instructions are reachable given an instruction stream and branch points extracted from a method. + /// + /// The algorithm works like so: + /// 1. determine the "blocks" that make up a function + /// * A block starts with either the start of the method, or a branch _target_ + /// * blocks are "where some other code might jump to" + /// * blocks end with either another branch, another branch target, or the end of the method + /// * this means blocks contain no control flow, except (maybe) as the very last instruction + /// 2. blocks have head and tail reachability + /// * a block has head reachablility if some other block that is reachable can branch to it + /// * a block has tail reachability if it contains no calls to methods that never return + /// 4. push the first block onto a stack + /// 5. while the stack is not empty + /// a. pop a block off the stack + /// b. give it head reachability + /// c. if the pop'd block is tail reachable, push the blocks it can branch to onto the stack + /// 6. consider each block + /// * if it is head and tail reachable, all instructions in it are reachable + /// * if it is not head reachable (regardless of tail reachability), no instructions in it are reachable + /// * if it is only head reachable, all instructions up to and including the first call to a method that does not return are reachable + /// + public ImmutableArray FindUnreachableIL(Collection instrs) + { + // no instructions, means nothing to... not reach + if (!instrs.Any()) + { + return ImmutableArray.Empty; + } + + // no known methods that do not return, so everything is reachable by definition + if (DoesNotReturnMethods.IsEmpty) + { + return ImmutableArray.Empty; + } + + var brs = FindBranches(instrs); + + var lastInstr = instrs.Last(); + + var blocks = CreateBasicBlocks(instrs, brs); + DetermineHeadReachability(blocks); + return DetermineUnreachableRanges(blocks, lastInstr.Offset); + } + + /// + /// Discovers branches, including unconditional ones, in the given instruction stream. + /// + private ImmutableArray FindBranches(Collection instrs) + { + var ret = ImmutableArray.CreateBuilder(); + foreach (var i in instrs) + { + if (BRANCH_OPCODES.Contains(i.OpCode)) + { + int? singleTargetOffset; + IEnumerable multiTargetOffsets; + + if (i.Operand is Instruction[] multiTarget) + { + // it's a switch + singleTargetOffset = null; + multiTargetOffsets = multiTarget.Select(t => t.Offset).Concat(new[] { i.Next.Offset }).Distinct().ToList(); + } + else if (i.Operand is Instruction singleTarget) + { + // it's any of the B.*(_S)? instructions + + if (UNCONDITIONAL_BRANCH_OPCODES.Contains(i.OpCode)) + { + multiTargetOffsets = null; + singleTargetOffset = singleTarget.Offset; + } + else + { + singleTargetOffset = null; + multiTargetOffsets = new[] { i.Next.Offset, singleTarget.Offset }; + } + } + else + { + throw new InvalidOperationException($"Unexpected operand when processing branch {i.Operand}: {i.Operand}"); + } + + if (singleTargetOffset != null) + { + ret.Add(new BranchInstruction(i.Offset, singleTargetOffset.Value)); + } + else + { + ret.Add(new BranchInstruction(i.Offset, multiTargetOffsets)); + } + } + } + + return ret.ToImmutable(); + } + + /// + /// Calculates which ranges of IL are unreachable, given blocks which have head and tail reachability calculated. + /// + private ImmutableArray DetermineUnreachableRanges(IReadOnlyList blocks, int lastInstructionOffset) + { + var ret = ImmutableArray.CreateBuilder(); + + var endOfMethodOffset = lastInstructionOffset + 1; // add 1 so we point _past_ the end of the method + + for (var curBlockIx = 0; curBlockIx < blocks.Count; curBlockIx++) + { + var curBlock = blocks[curBlockIx]; + + int endOfCurBlockOffset; + if (curBlockIx == blocks.Count - 1) + { + endOfCurBlockOffset = endOfMethodOffset; + } + else + { + endOfCurBlockOffset = blocks[curBlockIx + 1].StartOffset - 1; // minus 1 so we don't include anything of the following block + } + + if (curBlock.HeadReachable) + { + if (curBlock.TailReachable) + { + // it's all reachable + continue; + } + + // tail isn't reachable, which means there's a call to something that doesn't return... + var doesNotReturnInstr = curBlock.UnreachableAfter; + + // and it's everything _after_ the following instruction that is unreachable + // so record the following instruction through the end of the block + var followingInstr = doesNotReturnInstr.Next; + + ret.Add(new UnreachableRange(followingInstr.Offset, endOfCurBlockOffset)); + } + else + { + // none of it is reachable + ret.Add(new UnreachableRange(curBlock.StartOffset, endOfCurBlockOffset)); + } + } + + return ret.ToImmutable(); + } + + /// + /// Process all the blocks and determine if their first instruction is reachable, + /// that is if they have "head reachability". + /// + /// "Tail reachability" will have already been determined in CreateBlocks. + /// + private void DetermineHeadReachability(IEnumerable blocks) + { + var blockLookup = blocks.ToImmutableDictionary(b => b.StartOffset); + + var headBlock = blockLookup[0]; + + var knownLive = new Stack(); + knownLive.Push(headBlock); + + while (knownLive.Count > 0) + { + var block = knownLive.Pop(); + + if (block.HeadReachable) + { + // already seen this block + continue; + } + + // we can reach this block, clearly + block.HeadReachable = true; + + if (block.TailReachable) + { + // we can reach all the blocks it might flow to + foreach (var reachableOffset in block.BranchesTo) + { + var reachableBlock = blockLookup[reachableOffset]; + knownLive.Push(reachableBlock); + } + } + } + } + + /// + /// Create BasicBlocks from an instruction stream and branches. + /// + /// Each block starts either at the start of the method, immediately after a branch or at a target for a branch, + /// and ends with another branch, another branch target, or the end of the method. + /// + /// "Tail reachability" is also calculated, which is whether the block can ever actually get past its last instruction. + /// + private List CreateBasicBlocks(Collection instrs, IReadOnlyList branches) + { + // every branch-like instruction starts or stops a block + var branchInstrLocs = branches.ToLookup(i => i.Offset); + var branchInstrOffsets = branchInstrLocs.Select(k => k.Key).ToImmutableHashSet(); + + // every target that might be branched to starts or stops a block + var branchTargetOffsets = branches.SelectMany(b => b.HasMultiTargets ? b.TargetOffsets : new[] { b.TargetOffset }).ToImmutableHashSet(); + + // ending the method is also important + var endOfMethodOffset = instrs.Last().Offset; + + var blocks = new List(); + int? blockStartedAt = null; + Instruction unreachableAfter = null; + foreach (var i in instrs) + { + var offset = i.Offset; + var branchesAtLoc = branchInstrLocs[offset]; + + if (blockStartedAt == null) + { + blockStartedAt = offset; + unreachableAfter = null; + } + + var isBranch = branchInstrOffsets.Contains(offset); + var isFollowedByBranchTarget = i.Next != null && branchTargetOffsets.Contains(i.Next.Offset); + var isEndOfMtd = endOfMethodOffset == offset; + + if (unreachableAfter == null && DoesNotReturn(i)) + { + unreachableAfter = i; + } + + var blockEnds = isBranch || isFollowedByBranchTarget || isEndOfMtd; + if (blockEnds) + { + var nextInstr = i.Next; + var goesTo = + branchesAtLoc.Any() ? + branchesAtLoc.SelectMany( + b => b.HasMultiTargets ? b.TargetOffsets : new[] { b.TargetOffset } + ) : + nextInstr != null ? new[] { nextInstr.Offset } : Enumerable.Empty(); + + blocks.Add(new BasicBlock(blockStartedAt.Value, unreachableAfter, goesTo.ToImmutableArray())); + + blockStartedAt = null; + unreachableAfter = null; + } + } + + return blocks; + } + + /// + /// Returns true if the given instruction will never return, + /// and thus subsequent instructions will never be run. + /// + private bool DoesNotReturn(Instruction instr) + { + if (!IsCall(instr, out var mtd)) + { + return false; + } + + return DoesNotReturnMethods.Contains(mtd.MetadataToken); + } + + /// + /// Returns true if the given instruction is a Call or Callvirt. + /// + /// If it is a call, extracts the MethodReference that is being called. + /// + private static bool IsCall(Instruction instr, out MethodReference mtd) + { + var opcode = instr.OpCode; + if (opcode != OpCodes.Call && opcode != OpCodes.Callvirt) + { + mtd = null; + return false; + } + + mtd = (MethodReference)instr.Operand; + + return true; + } + } +} From 1ba63a113f16e4911f92c2d8fba3b790965e4a95 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Thu, 20 Aug 2020 20:30:59 -0400 Subject: [PATCH 06/18] Move everything in ReachabilityHelper over to immutable collections (rather than mixing and matching) and remove _most_ LINQ (clarifies some code, and performs a little better). --- .../Instrumentation/ReachabilityHelper.cs | 120 ++++++++++++------ 1 file changed, 84 insertions(+), 36 deletions(-) diff --git a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs index b7ff63b92..dd9a08f46 100644 --- a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs +++ b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs @@ -3,7 +3,6 @@ using Mono.Cecil.Cil; using Mono.Collections.Generic; using System; -using System.Collections.Generic; using System.Collections.Immutable; using System.Linq; @@ -87,7 +86,7 @@ private readonly struct BranchInstruction /// public int Offset { get; } - public bool HasMultiTargets => _TargetOffsets.Any(); + public bool HasMultiTargets => _TargetOffsets.Length > 0; private readonly int _TargetOffset; @@ -109,14 +108,14 @@ public int TargetOffset } } - private readonly IEnumerable _TargetOffsets; + private readonly ImmutableArray _TargetOffsets; /// /// Targets of the branch, assuming it has multiple targets. /// /// It is illegal to access this if there is a single target. /// - public IEnumerable TargetOffsets + public ImmutableArray TargetOffsets { get { @@ -133,12 +132,12 @@ public BranchInstruction(int offset, int targetOffset) { Offset = offset; _TargetOffset = targetOffset; - _TargetOffsets = Enumerable.Empty(); + _TargetOffsets = ImmutableArray.Empty; } - public BranchInstruction(int offset, IEnumerable targetOffset) + public BranchInstruction(int offset, ImmutableArray targetOffset) { - if (targetOffset.Count() < 1) + if (targetOffset.Length < 1) { throw new ArgumentException("Must have at least 2 entries", nameof(targetOffset)); } @@ -220,7 +219,7 @@ public static ReachabilityHelper CreateForModule(ModuleDefinition module, string return new ReachabilityHelper(ImmutableHashSet.Empty); } - var processedMethods = new HashSet(); + var processedMethods = ImmutableHashSet.Empty; var doNotReturn = ImmutableHashSet.CreateBuilder(); foreach (var type in module.Types) { @@ -254,11 +253,13 @@ public static ReachabilityHelper CreateForModule(ModuleDefinition module, string } var token = calledMtd.MetadataToken; - if (!processedMethods.Add(token)) + if (processedMethods.Contains(token)) { continue; } + processedMethods = processedMethods.Add(token); + MethodDefinition mtdDef; try { @@ -329,7 +330,7 @@ public static ReachabilityHelper CreateForModule(ModuleDefinition module, string public ImmutableArray FindUnreachableIL(Collection instrs) { // no instructions, means nothing to... not reach - if (!instrs.Any()) + if (instrs.Count == 0) { return ImmutableArray.Empty; } @@ -342,7 +343,7 @@ public ImmutableArray FindUnreachableIL(Collection FindBranches(Collection i if (BRANCH_OPCODES.Contains(i.OpCode)) { int? singleTargetOffset; - IEnumerable multiTargetOffsets; + ImmutableArray multiTargetOffsets; if (i.Operand is Instruction[] multiTarget) { // it's a switch singleTargetOffset = null; - multiTargetOffsets = multiTarget.Select(t => t.Offset).Concat(new[] { i.Next.Offset }).Distinct().ToList(); + + multiTargetOffsets = ImmutableArray.Create(i.Next.Offset); + foreach(var instr in multiTarget) + { + // in practice these are small arrays, so a scan should be fine + if(multiTargetOffsets.Contains(instr.Offset)) + { + continue; + } + + multiTargetOffsets = multiTargetOffsets.Add(instr.Offset); + } } else if (i.Operand is Instruction singleTarget) { @@ -374,13 +386,13 @@ private ImmutableArray FindBranches(Collection i if (UNCONDITIONAL_BRANCH_OPCODES.Contains(i.OpCode)) { - multiTargetOffsets = null; + multiTargetOffsets = ImmutableArray.Empty; singleTargetOffset = singleTarget.Offset; } else { singleTargetOffset = null; - multiTargetOffsets = new[] { i.Next.Offset, singleTarget.Offset }; + multiTargetOffsets = ImmutableArray.Create(i.Next.Offset, singleTarget.Offset); } } else @@ -405,18 +417,18 @@ private ImmutableArray FindBranches(Collection i /// /// Calculates which ranges of IL are unreachable, given blocks which have head and tail reachability calculated. /// - private ImmutableArray DetermineUnreachableRanges(IReadOnlyList blocks, int lastInstructionOffset) + private ImmutableArray DetermineUnreachableRanges(ImmutableArray blocks, int lastInstructionOffset) { var ret = ImmutableArray.CreateBuilder(); var endOfMethodOffset = lastInstructionOffset + 1; // add 1 so we point _past_ the end of the method - for (var curBlockIx = 0; curBlockIx < blocks.Count; curBlockIx++) + for (var curBlockIx = 0; curBlockIx < blocks.Length; curBlockIx++) { var curBlock = blocks[curBlockIx]; int endOfCurBlockOffset; - if (curBlockIx == blocks.Count - 1) + if (curBlockIx == blocks.Length - 1) { endOfCurBlockOffset = endOfMethodOffset; } @@ -458,18 +470,17 @@ private ImmutableArray DetermineUnreachableRanges(IReadOnlyLis /// /// "Tail reachability" will have already been determined in CreateBlocks. /// - private void DetermineHeadReachability(IEnumerable blocks) + private void DetermineHeadReachability(ImmutableArray blocks) { var blockLookup = blocks.ToImmutableDictionary(b => b.StartOffset); var headBlock = blockLookup[0]; - var knownLive = new Stack(); - knownLive.Push(headBlock); + var knownLive = ImmutableStack.Create(headBlock); - while (knownLive.Count > 0) + while (!knownLive.IsEmpty) { - var block = knownLive.Pop(); + knownLive = knownLive.Pop(out var block); if (block.HeadReachable) { @@ -486,7 +497,7 @@ private void DetermineHeadReachability(IEnumerable blocks) foreach (var reachableOffset in block.BranchesTo) { var reachableBlock = blockLookup[reachableOffset]; - knownLive.Push(reachableBlock); + knownLive = knownLive.Push(reachableBlock); } } } @@ -500,19 +511,33 @@ private void DetermineHeadReachability(IEnumerable blocks) /// /// "Tail reachability" is also calculated, which is whether the block can ever actually get past its last instruction. /// - private List CreateBasicBlocks(Collection instrs, IReadOnlyList branches) + private ImmutableArray CreateBasicBlocks(Collection instrs, ImmutableArray branches) { // every branch-like instruction starts or stops a block var branchInstrLocs = branches.ToLookup(i => i.Offset); var branchInstrOffsets = branchInstrLocs.Select(k => k.Key).ToImmutableHashSet(); // every target that might be branched to starts or stops a block - var branchTargetOffsets = branches.SelectMany(b => b.HasMultiTargets ? b.TargetOffsets : new[] { b.TargetOffset }).ToImmutableHashSet(); + var branchTargetOffsets = ImmutableHashSet.Empty; + foreach (var branch in branches) + { + if (branch.HasMultiTargets) + { + foreach (var target in branch.TargetOffsets) + { + branchTargetOffsets = branchTargetOffsets.Add(target); + } + } + else + { + branchTargetOffsets = branchTargetOffsets.Add(branch.TargetOffset); + } + } // ending the method is also important - var endOfMethodOffset = instrs.Last().Offset; + var endOfMethodOffset = instrs[instrs.Count - 1].Offset; - var blocks = new List(); + var blocks = ImmutableArray.Empty; int? blockStartedAt = null; Instruction unreachableAfter = null; foreach (var i in instrs) @@ -539,14 +564,37 @@ private List CreateBasicBlocks(Collection instrs, IRead if (blockEnds) { var nextInstr = i.Next; - var goesTo = - branchesAtLoc.Any() ? - branchesAtLoc.SelectMany( - b => b.HasMultiTargets ? b.TargetOffsets : new[] { b.TargetOffset } - ) : - nextInstr != null ? new[] { nextInstr.Offset } : Enumerable.Empty(); - - blocks.Add(new BasicBlock(blockStartedAt.Value, unreachableAfter, goesTo.ToImmutableArray())); + + // figure out all the different places the basic block could lead to + ImmutableArray goesTo; + if(branchesAtLoc.Any()) + { + // it ends in a branch, where all does it branch? + goesTo = ImmutableArray.Empty; + foreach(var branch in branchesAtLoc) + { + if(branch.HasMultiTargets) + { + goesTo = goesTo.AddRange(branch.TargetOffsets); + } + else + { + goesTo = goesTo.Add(branch.TargetOffset); + } + } + } + else if (nextInstr != null) + { + // it falls throw to another instruction + goesTo = ImmutableArray.Create(nextInstr.Offset); + } + else + { + // it ends the method + goesTo = ImmutableArray.Empty; + } + + blocks = blocks.Add(new BasicBlock(blockStartedAt.Value, unreachableAfter, goesTo)); blockStartedAt = null; unreachableAfter = null; From b6692d9872a85059af211d2e6d22a68468fcde07 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 10:14:33 -0400 Subject: [PATCH 07/18] Implements support for tracing throw exception handlers. This is necessary to support Leave(_S) in the non-exception cases, since it's legal for the compiler to emit those in a try{} even if they don't "leave". --- .../Instrumentation/Instrumenter.cs | 7 +- .../Instrumentation/ReachabilityHelper.cs | 373 +++++++++++++++--- .../Instrumentation/InstrumenterTests.cs | 14 +- .../Samples/Instrumentation.DoesNotReturn.cs | 53 +++ 4 files changed, 386 insertions(+), 61 deletions(-) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index cae5a3e13..cff6ab512 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -524,6 +524,11 @@ private void InstrumentMethod(MethodDefinition method) private void InstrumentIL(MethodDefinition method) { + if (method.Name == "FiltersAndFinallies") + { + Console.WriteLine(); + } + method.Body.SimplifyMacros(); ILProcessor processor = method.Body.GetILProcessor(); @@ -532,7 +537,7 @@ private void InstrumentIL(MethodDefinition method) var branchPoints = _cecilSymbolHelper.GetBranchPoints(method); - var unreachableRanges = _reachabilityHelper.FindUnreachableIL(processor.Body.Instructions); + var unreachableRanges = _reachabilityHelper.FindUnreachableIL(processor.Body.Instructions, processor.Body.ExceptionHandlers); var currentUnreachableRangeIx = 0; for (int n = 0; n < count; n++) diff --git a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs index dd9a08f46..62f146102 100644 --- a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs +++ b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Immutable; using System.Linq; +using System.Text; namespace coverlet.core.Instrumentation.Reachability { @@ -31,7 +32,7 @@ public UnreachableRange(int start, int end) } public override string ToString() - => $"[{StartOffset}, {EndOffset}]"; + => $"[IL_{StartOffset:x4}, IL_{EndOffset:x4}]"; } private class BasicBlock @@ -56,21 +57,29 @@ private class BasicBlock /// public Instruction UnreachableAfter { get; } + /// + /// If an exception is raised in this block, where control might branch to. + /// + /// Note that this can happen even if the block's end is unreachable. + /// + public ImmutableArray ExceptionBranchesTo { get; } + /// /// Mutable, records whether control can flow into the block, /// ie. whether it's head is reachable /// public bool HeadReachable { get; set; } - public BasicBlock(int startOffset, Instruction unreachableAfter, ImmutableArray branchesTo) + public BasicBlock(int startOffset, Instruction unreachableAfter, ImmutableArray branchesTo, ImmutableArray exceptionBranchesTo) { StartOffset = startOffset; UnreachableAfter = unreachableAfter; BranchesTo = branchesTo; + ExceptionBranchesTo = exceptionBranchesTo; } public override string ToString() - => $"{nameof(StartOffset)}={StartOffset}, {nameof(HeadReachable)}={HeadReachable}, {nameof(TailReachable)}={TailReachable}, {nameof(BranchesTo)}=({string.Join(", ", BranchesTo)}), {nameof(UnreachableAfter)}={UnreachableAfter}"; + => $"{nameof(StartOffset)}=IL_{StartOffset:x4}, {nameof(HeadReachable)}={HeadReachable}, {nameof(TailReachable)}={TailReachable}, {nameof(BranchesTo)}=({string.Join(", ", BranchesTo.Select(b => $"IL_{b:x4}"))}), {nameof(ExceptionBranchesTo)}=({string.Join(", ", ExceptionBranchesTo.Select(b => $"IL_{b:x4}"))}), {nameof(UnreachableAfter)}={(UnreachableAfter != null ? $"IL_{UnreachableAfter:x4}" : "")}"; } /// @@ -86,7 +95,10 @@ private readonly struct BranchInstruction /// public int Offset { get; } - public bool HasMultiTargets => _TargetOffsets.Length > 0; + /// + /// Returns true if this branch has multiple targets. + /// + public bool HasMultiTargets => _TargetOffset == -1; private readonly int _TargetOffset; @@ -137,15 +149,18 @@ public BranchInstruction(int offset, int targetOffset) public BranchInstruction(int offset, ImmutableArray targetOffset) { - if (targetOffset.Length < 1) + if (targetOffset.Length == 1) { - throw new ArgumentException("Must have at least 2 entries", nameof(targetOffset)); + throw new ArgumentException("Use single entry constructor for single targets", nameof(targetOffset)); } Offset = offset; _TargetOffset = -1; _TargetOffsets = targetOffset; } + + public override string ToString() + => $"IL_{Offset:x4}: {(HasMultiTargets ? string.Join(", ", TargetOffsets.Select(x => $"IL_{x:x4}")) : $"IL_{TargetOffset:x4}")}"; } /// @@ -182,7 +197,24 @@ public BranchInstruction(int offset, ImmutableArray targetOffset) OpCodes.Brfalse_S, OpCodes.Brtrue, OpCodes.Brtrue_S, - OpCodes.Switch + OpCodes.Switch, + + // These are forms of Br(_S) that are legal to use to exit + // an exception block + // + // So they're "weird" but not too weird for our purposes + // + // The somewhat nasty subtlety is that, within an exception block, + // it's perfectly legal to replace all normal branches with Leaves + // even if they don't actually exit the block. + OpCodes.Leave, + OpCodes.Leave_S, + + // these implicitly branch at the end of a filter or finally block + // their operands do not encode anything interesting, we have to + // look at exception handlers to figure out where they go to + OpCodes.Endfilter, + OpCodes.Endfinally } ); @@ -195,7 +227,10 @@ public BranchInstruction(int offset, ImmutableArray targetOffset) new[] { OpCodes.Br, - OpCodes.Br_S + OpCodes.Br_S, + OpCodes.Leave, + OpCodes.Leave_S, + OpCodes.Endfinally } ); @@ -327,7 +362,7 @@ public static ReachabilityHelper CreateForModule(ModuleDefinition module, string /// * if it is not head reachable (regardless of tail reachability), no instructions in it are reachable /// * if it is only head reachable, all instructions up to and including the first call to a method that does not return are reachable /// - public ImmutableArray FindUnreachableIL(Collection instrs) + public ImmutableArray FindUnreachableIL(Collection instrs, Collection exceptionHandlers) { // no instructions, means nothing to... not reach if (instrs.Count == 0) @@ -341,11 +376,107 @@ public ImmutableArray FindUnreachableIL(Collection.Empty; } - var brs = FindBranches(instrs); + var brs = FindBranches(instrs, exceptionHandlers); var lastInstr = instrs[instrs.Count - 1]; - var blocks = CreateBasicBlocks(instrs, brs); + var blocks = CreateBasicBlocks(instrs, exceptionHandlers, brs); + + // debug + + var sb = new StringBuilder(); + sb.AppendLine("Exception Handlers"); + sb.AppendLine("------------------"); + for (var i = 0; i < exceptionHandlers.Count; i++) + { + var handler = exceptionHandlers[i]; + + sb.AppendLine($"{i:00}: Try: (IL_{handler.TryStart.Offset:x4} - IL_{handler.TryEnd.Offset:x4}){(handler.FilterStart != null ? $", Filter: IL_{handler.FilterStart.Offset:x4}" : "")}, Handler: (IL_{handler.HandlerStart.Offset:x4}{(handler.HandlerEnd != null ? $" - IL_{handler.HandlerEnd.Offset:x4})" : "")})"); + } + sb.AppendLine(); + + sb.AppendLine("Branches"); + sb.AppendLine("--------"); + for (var i = 0; i < brs.Length; i++) + { + var br = brs[i]; + + sb.AppendLine($"{i:00}: {br}"); + } + sb.AppendLine(); + + sb.AppendLine("Blocks"); + sb.AppendLine("------"); + for (var i = 0; i < blocks.Length; i++) + { + var block = blocks[i]; + + sb.AppendLine($"{i:00}: {block}"); + } + sb.AppendLine(); + + sb.AppendLine("Instructions"); + sb.AppendLine("------------"); + for (var i = 0; i < instrs.Count; i++) + { + var instr = instrs[i]; + + var blockStarts = blocks.Select((t, ix) => (Value: t, Index: ix)).Where(b => b.Value.StartOffset == instr.Offset); + var tryStarts = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.TryStart.Offset == instr.Offset); + var tryEnd = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.TryEnd.Offset == instr.Offset); + var filterStart = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.FilterStart != null && s.Value.FilterStart.Offset == instr.Offset); + var handlerStart = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.HandlerStart.Offset == instr.Offset); + var handlerStop = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.HandlerEnd?.Offset == instr.Offset); + var branchesTo = brs.Select((t, ix) => (Value: t, Index: ix)).Where(b => b.Value.Offset == instr.Offset); + var branchesFrom = brs.Select((t, ix) => (Value: t, Index: ix)).Where(b => b.Value.HasMultiTargets ? b.Value.TargetOffsets.Contains(instr.Offset) : b.Value.TargetOffset == instr.Offset); + + var hasSpecial = blockStarts.Any() || tryStarts.Any() || tryEnd.Any() || filterStart.Any() || handlerStart.Any() || handlerStop.Any() || branchesTo.Any() || branchesFrom.Any(); + + if (hasSpecial) + { + sb.AppendLine(); + + foreach (var x in blockStarts) + { + sb.AppendLine($"=== BLOCK {x.Index:00} ==="); + } + foreach (var x in tryStarts) + { + sb.AppendLine($"--- try start {x.Index:00}"); + } + foreach (var x in tryEnd) + { + sb.AppendLine($"--- try end {x.Index:00}"); + } + foreach (var x in filterStart) + { + sb.AppendLine($"--- filter start {x.Index:00}"); + } + foreach (var x in handlerStart) + { + sb.AppendLine($"--- handler start {x.Index:00}"); + } + foreach (var x in handlerStop) + { + sb.AppendLine($"--- handler end {x.Index:00}"); + } + foreach (var x in branchesTo) + { + sb.AppendLine($"--- branch {x.Index:00} -> {(x.Value.HasMultiTargets ? string.Join(", ", x.Value.TargetOffsets.Select(x => $"IL_{x:x4}")) : $"IL_{x.Value.TargetOffset:x4}")}"); + } + foreach (var x in branchesFrom) + { + sb.AppendLine($"--- from branch {x.Index:00} at IL_{x.Value.Offset:x4}"); + } + } + + sb.AppendLine(instr.ToString()); + } + + var debugStr = sb.ToString(); + + // end debug + DetermineHeadReachability(blocks); return DetermineUnreachableRanges(blocks, lastInstr.Offset); } @@ -353,65 +484,127 @@ public ImmutableArray FindUnreachableIL(Collection /// Discovers branches, including unconditional ones, in the given instruction stream. /// - private ImmutableArray FindBranches(Collection instrs) + private ImmutableArray FindBranches(Collection instrs, Collection exceptionHandlers) { var ret = ImmutableArray.CreateBuilder(); foreach (var i in instrs) { if (BRANCH_OPCODES.Contains(i.OpCode)) { - int? singleTargetOffset; - ImmutableArray multiTargetOffsets; + var (singleTargetOffset, multiTargetOffsets) = GetInstructionTargets(i, exceptionHandlers); - if (i.Operand is Instruction[] multiTarget) + if (singleTargetOffset != null) { - // it's a switch - singleTargetOffset = null; - - multiTargetOffsets = ImmutableArray.Create(i.Next.Offset); - foreach(var instr in multiTarget) - { - // in practice these are small arrays, so a scan should be fine - if(multiTargetOffsets.Contains(instr.Offset)) - { - continue; - } - - multiTargetOffsets = multiTargetOffsets.Add(instr.Offset); - } + ret.Add(new BranchInstruction(i.Offset, singleTargetOffset.Value)); } - else if (i.Operand is Instruction singleTarget) + else { - // it's any of the B.*(_S)? instructions + ret.Add(new BranchInstruction(i.Offset, multiTargetOffsets)); + } + } + } - if (UNCONDITIONAL_BRANCH_OPCODES.Contains(i.OpCode)) - { - multiTargetOffsets = ImmutableArray.Empty; - singleTargetOffset = singleTarget.Offset; - } - else - { - singleTargetOffset = null; - multiTargetOffsets = ImmutableArray.Create(i.Next.Offset, singleTarget.Offset); - } + return ret.ToImmutable(); + } + + /// + /// For a single instruction, determines all the places it might branch to. + /// + private static (int? SingleTargetOffset, ImmutableArray MultiTargetOffsets) GetInstructionTargets(Instruction i, Collection exceptionHandlers) + { + int? singleTargetOffset; + ImmutableArray multiTargetOffsets; + + if (i.Operand is Instruction[] multiTarget) + { + // it's a switch + singleTargetOffset = null; + + multiTargetOffsets = ImmutableArray.Create(i.Next.Offset); + foreach (var instr in multiTarget) + { + // in practice these are small arrays, so a scan should be fine + if (multiTargetOffsets.Contains(instr.Offset)) + { + continue; } - else + + multiTargetOffsets = multiTargetOffsets.Add(instr.Offset); + } + } + else if (i.Operand is Instruction targetInstr) + { + // it's any of the B.*(_S)? or Leave(_S)? instructions + + if (UNCONDITIONAL_BRANCH_OPCODES.Contains(i.OpCode)) + { + multiTargetOffsets = ImmutableArray.Empty; + singleTargetOffset = targetInstr.Offset; + } + else + { + singleTargetOffset = null; + multiTargetOffsets = ImmutableArray.Create(i.Next.Offset, targetInstr.Offset); + } + } + else if (i.OpCode == OpCodes.Endfilter) + { + // Endfilter is always the last instruction in a filter block, and no sort of control + // flow is allowed so we can scan backwards to see find the block + + ExceptionHandler filterForHandler = null; + foreach (var handler in exceptionHandlers) + { + if (handler.FilterStart == null) { - throw new InvalidOperationException($"Unexpected operand when processing branch {i.Operand}: {i.Operand}"); + continue; } - if (singleTargetOffset != null) + var startsAt = handler.FilterStart; + var cur = startsAt; + while (cur != null && cur.Offset < i.Offset) { - ret.Add(new BranchInstruction(i.Offset, singleTargetOffset.Value)); + cur = cur.Next; } - else + + if (cur != null && cur.Offset == i.Offset) { - ret.Add(new BranchInstruction(i.Offset, multiTargetOffsets)); + filterForHandler = handler; + break; } } + + if (filterForHandler == null) + { + throw new InvalidOperationException($"Could not find ExceptionHandler associated with {i}"); + } + + // filter can do one of two things: + // - branch into handler + // - percolate to another catch block, which might not be in this method + // + // so we chose to model this as an unconditional branch into the handler + singleTargetOffset = filterForHandler.HandlerStart.Offset; + multiTargetOffsets = ImmutableArray.Empty; + } + else if (i.OpCode == OpCodes.Endfinally) + { + // Endfinally is very weird + // + // what it does, effectively is "take whatever branch would normally happen after the instruction + // that left the paired try + // + // practically, this makes endfinally a branch with no target + + singleTargetOffset = null; + multiTargetOffsets = ImmutableArray.Empty; + } + else + { + throw new InvalidOperationException($"Unexpected operand when processing branch {i}"); } - return ret.ToImmutable(); + return (singleTargetOffset, multiTargetOffsets); } /// @@ -500,40 +693,64 @@ private void DetermineHeadReachability(ImmutableArray blocks) knownLive = knownLive.Push(reachableBlock); } } + + // if the block is covered by an exception handler, then executing _any_ instruction in it + // could conceivably cause those handlers to be visited + foreach(var exceptionHandlerOffset in block.ExceptionBranchesTo) + { + var reachableHandler = blockLookup[exceptionHandlerOffset]; + knownLive = knownLive.Push(reachableHandler); + } } } /// - /// Create BasicBlocks from an instruction stream and branches. + /// Create BasicBlocks from an instruction stream, exception blocks, and branches. /// /// Each block starts either at the start of the method, immediately after a branch or at a target for a branch, /// and ends with another branch, another branch target, or the end of the method. /// /// "Tail reachability" is also calculated, which is whether the block can ever actually get past its last instruction. /// - private ImmutableArray CreateBasicBlocks(Collection instrs, ImmutableArray branches) + private ImmutableArray CreateBasicBlocks(Collection instrs, Collection exceptionHandlers, ImmutableArray branches) { // every branch-like instruction starts or stops a block var branchInstrLocs = branches.ToLookup(i => i.Offset); var branchInstrOffsets = branchInstrLocs.Select(k => k.Key).ToImmutableHashSet(); // every target that might be branched to starts or stops a block - var branchTargetOffsets = ImmutableHashSet.Empty; + var branchTargetOffsetsBuilder = ImmutableHashSet.CreateBuilder(); foreach (var branch in branches) { if (branch.HasMultiTargets) { foreach (var target in branch.TargetOffsets) { - branchTargetOffsets = branchTargetOffsets.Add(target); + branchTargetOffsetsBuilder.Add(target); } } else { - branchTargetOffsets = branchTargetOffsets.Add(branch.TargetOffset); + branchTargetOffsetsBuilder.Add(branch.TargetOffset); + } + } + + // every exception handler an entry point + // either it's handler, or it's filter (if present) + foreach (var handler in exceptionHandlers) + { + if (handler.FilterStart != null) + { + branchTargetOffsetsBuilder.Add(handler.FilterStart.Offset); + } + else + { + branchTargetOffsetsBuilder.Add(handler.HandlerStart.Offset); } } + var branchTargetOffsets = branchTargetOffsetsBuilder.ToImmutable(); + // ending the method is also important var endOfMethodOffset = instrs[instrs.Count - 1].Offset; @@ -567,13 +784,13 @@ private ImmutableArray CreateBasicBlocks(Collection ins // figure out all the different places the basic block could lead to ImmutableArray goesTo; - if(branchesAtLoc.Any()) + if (branchesAtLoc.Any()) { // it ends in a branch, where all does it branch? goesTo = ImmutableArray.Empty; - foreach(var branch in branchesAtLoc) + foreach (var branch in branchesAtLoc) { - if(branch.HasMultiTargets) + if (branch.HasMultiTargets) { goesTo = goesTo.AddRange(branch.TargetOffsets); } @@ -594,7 +811,49 @@ private ImmutableArray CreateBasicBlocks(Collection ins goesTo = ImmutableArray.Empty; } - blocks = blocks.Add(new BasicBlock(blockStartedAt.Value, unreachableAfter, goesTo)); + var exceptionSwitchesTo = ImmutableArray.Empty; + + // if the block is covered by any exception handlers then + // it is possible that it will branch to its handler block + foreach (var handler in exceptionHandlers) + { + var tryStart = handler.TryStart.Offset; + var tryEnd = handler.TryEnd.Offset; + + var containsStartOfTry = + tryStart >= blockStartedAt.Value && + tryStart <= i.Offset; + + var containsEndOfTry = + tryEnd >= blockStartedAt.Value && + tryEnd <= i.Offset; + + var blockInsideTry = blockStartedAt.Value >= tryStart && i.Offset <= tryEnd; + + // blocks do not necessarily align to the TRY part of exception handlers, so we need to handle three cases: + // - the try _starts_ in the block + // - the try _ends_ in the block + // - the try complete covers the block, but starts and ends before and after it (respectively) + var tryOverlapsBlock = containsStartOfTry || containsEndOfTry || blockInsideTry; + + if (!tryOverlapsBlock) + { + continue; + } + + // if there's a filter, that runs first + if (handler.FilterStart != null) + { + exceptionSwitchesTo = exceptionSwitchesTo.Add(handler.FilterStart.Offset); + } + else + { + // otherwise, go straight to the handler + exceptionSwitchesTo = exceptionSwitchesTo.Add(handler.HandlerStart.Offset); + } + } + + blocks = blocks.Add(new BasicBlock(blockStartedAt.Value, unreachableAfter, goesTo, exceptionSwitchesTo)); blockStartedAt = null; unreachableAfter = null; diff --git a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs index 30b321257..7342fa2f9 100644 --- a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs +++ b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs @@ -665,13 +665,17 @@ public void TestReachabilityHelper() // AlsoThrows 134, 135, // CallsGenericClassDoesNotReturn - 140, 141, 142, 143, 144 + 140, 141, 142, 143, 144, + // WithLeave + 147, 149, 150, 151, 152, 153, 154, 155, 156, 159, 161, 163, 166, 167, 168, + // FiltersAndFinallies + 171, 173, 174, 175, 176, 177, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 192, 193, 194, 195, 196, 197 }; var notReachableLines = new[] { // NoBranches - 15, + 15, 16, // If 26, 27, @@ -684,7 +688,11 @@ public void TestReachabilityHelper() // CallsGenericMethodDoesNotReturn 127, 128, // CallsGenericClassDoesNotReturn - 143, 144 + 143, 144, + // WithLeave + 163, 164, + // FiltersAndFinallies + 176, 177, 183, 184, 189, 190, 195, 196, 197 }; var expectedToBeInstrumented = allInstrumentableLines.Except(notReachableLines).ToArray(); diff --git a/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs b/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs index 0b24f41b4..75512c304 100644 --- a/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs +++ b/test/coverlet.core.tests/Samples/Instrumentation.DoesNotReturn.cs @@ -142,5 +142,58 @@ public void CallsGenericClassDoesNotReturn() GenericClass.AlsoThrows(); System.Console.WriteLine("Constant-2"); // unreachable } + + public void WithLeave() + { + try + { + System.Console.WriteLine("Constant-1"); + } + catch (System.Exception e) + { + if (e is System.InvalidOperationException) + { + goto endOfMethod; + } + + System.Console.WriteLine("InCatch-1"); + + Throws(); + + System.Console.WriteLine("InCatch-2"); // unreachable + } // unreachable + + endOfMethod: + System.Console.WriteLine("Constant-2"); + } + + public void FiltersAndFinallies() + { + try + { + System.Console.WriteLine("Constant-1"); + Throws(); + System.Console.WriteLine("Constant-2"); //unreachable + } //unreachable + catch (System.InvalidOperationException e) + when (e.Message != null) + { + System.Console.WriteLine("InCatch-1"); + Throws(); + System.Console.WriteLine("InCatch-2"); //unreachable + } //unreachable + catch (System.InvalidOperationException) + { + System.Console.WriteLine("InCatch-3"); + Throws(); + System.Console.WriteLine("InCatch-4"); //unreachable + } //unreachable + finally + { + System.Console.WriteLine("InFinally-1"); + Throws(); + System.Console.WriteLine("InFinally-2"); //unreachable + } //unreachable + } //unreachable } } From 9660b923f4d227b3f3927ee6caa01ca2bb7a3737 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 15:15:30 -0400 Subject: [PATCH 08/18] Remove debugging code. --- .../Instrumentation/Instrumenter.cs | 5 - .../Instrumentation/ReachabilityHelper.cs | 96 ------------------- 2 files changed, 101 deletions(-) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index cff6ab512..86bb01fc0 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -524,11 +524,6 @@ private void InstrumentMethod(MethodDefinition method) private void InstrumentIL(MethodDefinition method) { - if (method.Name == "FiltersAndFinallies") - { - Console.WriteLine(); - } - method.Body.SimplifyMacros(); ILProcessor processor = method.Body.GetILProcessor(); diff --git a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs index 62f146102..8f2f3833a 100644 --- a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs +++ b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Immutable; using System.Linq; -using System.Text; namespace coverlet.core.Instrumentation.Reachability { @@ -382,101 +381,6 @@ public ImmutableArray FindUnreachableIL(Collection (Value: t, Index: ix)).Where(b => b.Value.StartOffset == instr.Offset); - var tryStarts = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.TryStart.Offset == instr.Offset); - var tryEnd = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.TryEnd.Offset == instr.Offset); - var filterStart = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.FilterStart != null && s.Value.FilterStart.Offset == instr.Offset); - var handlerStart = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.HandlerStart.Offset == instr.Offset); - var handlerStop = exceptionHandlers.Select((t, ix) => (Value: t, Index: ix)).Where(s => s.Value.HandlerEnd?.Offset == instr.Offset); - var branchesTo = brs.Select((t, ix) => (Value: t, Index: ix)).Where(b => b.Value.Offset == instr.Offset); - var branchesFrom = brs.Select((t, ix) => (Value: t, Index: ix)).Where(b => b.Value.HasMultiTargets ? b.Value.TargetOffsets.Contains(instr.Offset) : b.Value.TargetOffset == instr.Offset); - - var hasSpecial = blockStarts.Any() || tryStarts.Any() || tryEnd.Any() || filterStart.Any() || handlerStart.Any() || handlerStop.Any() || branchesTo.Any() || branchesFrom.Any(); - - if (hasSpecial) - { - sb.AppendLine(); - - foreach (var x in blockStarts) - { - sb.AppendLine($"=== BLOCK {x.Index:00} ==="); - } - foreach (var x in tryStarts) - { - sb.AppendLine($"--- try start {x.Index:00}"); - } - foreach (var x in tryEnd) - { - sb.AppendLine($"--- try end {x.Index:00}"); - } - foreach (var x in filterStart) - { - sb.AppendLine($"--- filter start {x.Index:00}"); - } - foreach (var x in handlerStart) - { - sb.AppendLine($"--- handler start {x.Index:00}"); - } - foreach (var x in handlerStop) - { - sb.AppendLine($"--- handler end {x.Index:00}"); - } - foreach (var x in branchesTo) - { - sb.AppendLine($"--- branch {x.Index:00} -> {(x.Value.HasMultiTargets ? string.Join(", ", x.Value.TargetOffsets.Select(x => $"IL_{x:x4}")) : $"IL_{x.Value.TargetOffset:x4}")}"); - } - foreach (var x in branchesFrom) - { - sb.AppendLine($"--- from branch {x.Index:00} at IL_{x.Value.Offset:x4}"); - } - } - - sb.AppendLine(instr.ToString()); - } - - var debugStr = sb.ToString(); - - // end debug - DetermineHeadReachability(blocks); return DetermineUnreachableRanges(blocks, lastInstr.Offset); } From 8d01c06290037545f0a29ea9e98b1e0174fce9cf Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 15:18:44 -0400 Subject: [PATCH 09/18] While this wouldn't produce an error, it is incorrect to consider this an unconditional branch (in the same way these other terms are branches, anyway). --- src/coverlet.core/Instrumentation/ReachabilityHelper.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs index 8f2f3833a..871ecf903 100644 --- a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs +++ b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs @@ -228,8 +228,7 @@ public override string ToString() OpCodes.Br, OpCodes.Br_S, OpCodes.Leave, - OpCodes.Leave_S, - OpCodes.Endfinally + OpCodes.Leave_S } ); From 5eb5bac3086f864529c0177a0096ecce8e69dff4 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 15:26:11 -0400 Subject: [PATCH 10/18] don't do control flow analysis in reachability if there are no [DoesNotReturn] methods --- .../Instrumentation/ReachabilityHelper.cs | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs index 871ecf903..9ade77d93 100644 --- a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs +++ b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs @@ -374,24 +374,35 @@ public ImmutableArray FindUnreachableIL(Collection.Empty; } - var brs = FindBranches(instrs, exceptionHandlers); + var (mayContainUnreachableCode, branches) = AnalyzeInstructions(instrs, exceptionHandlers); + + // no need to do any more work, nothing unreachable here + if (!mayContainUnreachableCode) + { + return ImmutableArray.Empty; + } var lastInstr = instrs[instrs.Count - 1]; - var blocks = CreateBasicBlocks(instrs, exceptionHandlers, brs); + var blocks = CreateBasicBlocks(instrs, exceptionHandlers, branches); DetermineHeadReachability(blocks); return DetermineUnreachableRanges(blocks, lastInstr.Offset); } /// - /// Discovers branches, including unconditional ones, in the given instruction stream. + /// Analyzes the instructiona and exception handlers provided to find branches and determine if + /// it is possible for their to be unreachable code. /// - private ImmutableArray FindBranches(Collection instrs, Collection exceptionHandlers) + private (bool MayContainUnreachableCode, ImmutableArray Branches) AnalyzeInstructions(Collection instrs, Collection exceptionHandlers) { + var containsDoesNotReturnCall = false; + var ret = ImmutableArray.CreateBuilder(); foreach (var i in instrs) { + containsDoesNotReturnCall = containsDoesNotReturnCall || DoesNotReturn(i); + if (BRANCH_OPCODES.Contains(i.OpCode)) { var (singleTargetOffset, multiTargetOffsets) = GetInstructionTargets(i, exceptionHandlers); @@ -407,7 +418,7 @@ private ImmutableArray FindBranches(Collection i } } - return ret.ToImmutable(); + return (containsDoesNotReturnCall, ret.ToImmutable()); } /// @@ -599,7 +610,7 @@ private void DetermineHeadReachability(ImmutableArray blocks) // if the block is covered by an exception handler, then executing _any_ instruction in it // could conceivably cause those handlers to be visited - foreach(var exceptionHandlerOffset in block.ExceptionBranchesTo) + foreach (var exceptionHandlerOffset in block.ExceptionBranchesTo) { var reachableHandler = blockLookup[exceptionHandlerOffset]; knownLive = knownLive.Push(reachableHandler); From eb6cced144d65ead09b2c2742d9e87ea0a31b62f Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 15:26:38 -0400 Subject: [PATCH 11/18] formatting --- test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs index 7342fa2f9..ea5370689 100644 --- a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs +++ b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs @@ -675,8 +675,7 @@ public void TestReachabilityHelper() new[] { // NoBranches - 15, - 16, + 15, 16, // If 26, 27, // Switch From d953b56c22436e934677840314569773724fc103 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 15:40:27 -0400 Subject: [PATCH 12/18] remove some now dead usings, and fix a comment --- src/coverlet.core/Instrumentation/Instrumenter.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index e489ecf34..6204a312e 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Collections.Immutable; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.IO; @@ -14,7 +13,6 @@ using Mono.Cecil; using Mono.Cecil.Cil; using Mono.Cecil.Rocks; -using Mono.Collections.Generic; namespace Coverlet.Core.Instrumentation { @@ -192,7 +190,7 @@ private bool Is_System_Threading_Interlocked_CoreLib_Type(TypeDefinition type) } // Have to do this before we start writing to a module, as we'll get into file - // locking issues if we do. + // locking issues if we do it while writing. private void CreateReachabilityHelper() { using (var stream = _fileSystem.NewFileStream(_module, FileMode.Open, FileAccess.Read)) From f0f715508f93ee726a331d7ecf6e72e3077b586f Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 15:43:05 -0400 Subject: [PATCH 13/18] doh, did not notice this example... removing since I've already got the TestReachabilityHelper test working in VS. --- .../Coverage/CoverageTests.DoesNotReturn.cs | 51 ------------------- 1 file changed, 51 deletions(-) delete mode 100644 test/coverlet.core.tests/Coverage/CoverageTests.DoesNotReturn.cs diff --git a/test/coverlet.core.tests/Coverage/CoverageTests.DoesNotReturn.cs b/test/coverlet.core.tests/Coverage/CoverageTests.DoesNotReturn.cs deleted file mode 100644 index bf86416b3..000000000 --- a/test/coverlet.core.tests/Coverage/CoverageTests.DoesNotReturn.cs +++ /dev/null @@ -1,51 +0,0 @@ -using System.IO; -using System.Threading.Tasks; - -using Coverlet.Core.Samples.Tests; -using Tmds.Utils; -using Xunit; - -namespace Coverlet.Core.Tests -{ - public partial class CoverageTests : ExternalProcessExecutionTest - { - [Fact] - public void DoesNotReturnSample() - { - string path = Path.GetTempFileName(); - try - { - // After debugging and before to push on PR change to Run for out of process test on CI - // FunctionExecutor.Run(async (string[] pathSerialize) => - - // For in-process debugging - FunctionExecutor.RunInProcess(async (string[] pathSerialize) => - { - CoveragePrepareResult coveragePrepareResult = await TestInstrumentationHelper.Run(instance => - { - try - { - instance.NoBranches(); // call method to test it's a dynamic - } - catch - { - // will throw do nothing - } - return Task.CompletedTask; - }, persistPrepareResultToFile: pathSerialize[0]); - return 0; - }, new string[] { path }); - - TestInstrumentationHelper.GetCoverageResult(path) - .GenerateReport(show: true) // remove at the end of debugging it allows to open and show report for fast check - .Document("Instrumentation.DoesNotReturn.cs") // chose cs source of samples where check rows - // .AssertBranchesCovered(... Add coverage check - ; - } - finally - { - File.Delete(path); - } - } - } -} \ No newline at end of file From 8e907f2bac7738f16f0adf4fb16d5fd1f247e5cf Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 16:36:55 -0400 Subject: [PATCH 14/18] Put DoesNotReturnAttributes attributes into the appropriate places for commandline and msbuild use. --- .../DataCollection/CoverageWrapper.cs | 3 +- .../DataCollection/CoverletSettings.cs | 6 ++++ .../DataCollection/CoverletSettingsParser.cs | 12 ++++++++ .../Utilities/CoverletConstants.cs | 1 + src/coverlet.console/Program.cs | 4 ++- .../Instrumentation/Instrumenter.cs | 2 +- .../Instrumentation/ReachabilityHelper.cs | 2 +- .../InstrumentationTask.cs | 5 +++- .../coverlet.msbuild.targets | 3 +- .../CoverletSettingsParserTests.cs | 30 +++++++++++-------- 10 files changed, 49 insertions(+), 19 deletions(-) diff --git a/src/coverlet.collector/DataCollection/CoverageWrapper.cs b/src/coverlet.collector/DataCollection/CoverageWrapper.cs index d99e3bd8d..c56d9982e 100644 --- a/src/coverlet.collector/DataCollection/CoverageWrapper.cs +++ b/src/coverlet.collector/DataCollection/CoverageWrapper.cs @@ -28,7 +28,8 @@ public Coverage CreateCoverage(CoverletSettings settings, ILogger coverletLogger SingleHit = settings.SingleHit, MergeWith = settings.MergeWith, UseSourceLink = settings.UseSourceLink, - SkipAutoProps = settings.SkipAutoProps + SkipAutoProps = settings.SkipAutoProps, + DoesNotReturnAttributes = settings.DoesNotReturnAttributes }; return new Coverage( diff --git a/src/coverlet.collector/DataCollection/CoverletSettings.cs b/src/coverlet.collector/DataCollection/CoverletSettings.cs index eaa1afbb6..85747612b 100644 --- a/src/coverlet.collector/DataCollection/CoverletSettings.cs +++ b/src/coverlet.collector/DataCollection/CoverletSettings.cs @@ -68,6 +68,11 @@ internal class CoverletSettings /// public bool SkipAutoProps { get; set; } + /// + /// Attributes that mark methods that never return. + /// + public string[] DoesNotReturnAttributes { get; set; } + public override string ToString() { var builder = new StringBuilder(); @@ -83,6 +88,7 @@ public override string ToString() builder.AppendFormat("SingleHit: '{0}'", SingleHit); builder.AppendFormat("IncludeTestAssembly: '{0}'", IncludeTestAssembly); builder.AppendFormat("SkipAutoProps: '{0}'", SkipAutoProps); + builder.AppendFormat("DoesNotReturnAttributes: '{0}'", string.Join(",", DoesNotReturnAttributes ?? Enumerable.Empty())); return builder.ToString(); } diff --git a/src/coverlet.collector/DataCollection/CoverletSettingsParser.cs b/src/coverlet.collector/DataCollection/CoverletSettingsParser.cs index ce37c237c..a3b1b1124 100644 --- a/src/coverlet.collector/DataCollection/CoverletSettingsParser.cs +++ b/src/coverlet.collector/DataCollection/CoverletSettingsParser.cs @@ -43,6 +43,7 @@ public CoverletSettings Parse(XmlElement configurationElement, IEnumerable + /// Parse attributes that mark methods that do not return. + /// + /// Configuration element + /// DoesNotReturn attributes + private string[] ParseDoesNotReturnAttributes(XmlElement configurationElement) + { + XmlElement doesNotReturnAttributesElement = configurationElement[CoverletConstants.DoesNotReturnAttributesElementName]; + return this.SplitElement(doesNotReturnAttributesElement); + } + /// /// Splits a comma separated elements into an array /// diff --git a/src/coverlet.collector/Utilities/CoverletConstants.cs b/src/coverlet.collector/Utilities/CoverletConstants.cs index c89846a59..8e4313875 100644 --- a/src/coverlet.collector/Utilities/CoverletConstants.cs +++ b/src/coverlet.collector/Utilities/CoverletConstants.cs @@ -21,5 +21,6 @@ internal static class CoverletConstants public const string DefaultExcludeFilter = "[coverlet.*]*"; public const string InProcDataCollectorName = "CoverletInProcDataCollector"; public const string SkipAutoProps = "SkipAutoProps"; + public const string DoesNotReturnAttributesElementName = "DoesNotReturnAttribute"; } } diff --git a/src/coverlet.console/Program.cs b/src/coverlet.console/Program.cs index 0099b4db0..1b4cbf219 100644 --- a/src/coverlet.console/Program.cs +++ b/src/coverlet.console/Program.cs @@ -63,6 +63,7 @@ static int Main(string[] args) CommandOption skipAutoProp = app.Option("--skipautoprops", "Neither track nor record auto-implemented properties.", CommandOptionType.NoValue); CommandOption mergeWith = app.Option("--merge-with", "Path to existing coverage result to merge.", CommandOptionType.SingleValue); CommandOption useSourceLink = app.Option("--use-source-link", "Specifies whether to use SourceLink URIs in place of file system paths.", CommandOptionType.NoValue); + CommandOption doesNotReturnAttributes = app.Option("--does-not-return-attribute", "Attributes that mark methods that do not return.", CommandOptionType.MultipleValue); app.OnExecute(() => { @@ -89,7 +90,8 @@ static int Main(string[] args) SingleHit = singleHit.HasValue(), MergeWith = mergeWith.Value(), UseSourceLink = useSourceLink.HasValue(), - SkipAutoProps = skipAutoProp.HasValue() + SkipAutoProps = skipAutoProp.HasValue(), + DoesNotReturnAttributes = doesNotReturnAttributes.Values.ToArray() }; Coverage coverage = new Coverage(module.Value, diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index 6204a312e..d7768bfc3 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -5,7 +5,7 @@ using System.IO; using System.Linq; using System.Runtime.CompilerServices; -using coverlet.core.Instrumentation.Reachability; +using Coverlet.Core.Instrumentation.Reachability; using Coverlet.Core.Abstractions; using Coverlet.Core.Attributes; using Coverlet.Core.Symbols; diff --git a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs index 9ade77d93..4d7474ea6 100644 --- a/src/coverlet.core/Instrumentation/ReachabilityHelper.cs +++ b/src/coverlet.core/Instrumentation/ReachabilityHelper.cs @@ -6,7 +6,7 @@ using System.Collections.Immutable; using System.Linq; -namespace coverlet.core.Instrumentation.Reachability +namespace Coverlet.Core.Instrumentation.Reachability { /// /// Helper for find unreachable IL instructions. diff --git a/src/coverlet.msbuild.tasks/InstrumentationTask.cs b/src/coverlet.msbuild.tasks/InstrumentationTask.cs index cb2a34784..11bfbc6db 100644 --- a/src/coverlet.msbuild.tasks/InstrumentationTask.cs +++ b/src/coverlet.msbuild.tasks/InstrumentationTask.cs @@ -40,6 +40,8 @@ public class InstrumentationTask : BaseTask public bool SkipAutoProps { get; set; } + public string DoesNotReturnAttribute { get; set; } + [Output] public ITaskItem InstrumenterState { get; set; } @@ -98,7 +100,8 @@ public override bool Execute() SingleHit = SingleHit, MergeWith = MergeWith, UseSourceLink = UseSourceLink, - SkipAutoProps = SkipAutoProps + SkipAutoProps = SkipAutoProps, + DoesNotReturnAttributes = DoesNotReturnAttribute?.Split(',') }; Coverage coverage = new Coverage(Path, diff --git a/src/coverlet.msbuild.tasks/coverlet.msbuild.targets b/src/coverlet.msbuild.tasks/coverlet.msbuild.targets index fefd1cbdb..7d6c3906a 100644 --- a/src/coverlet.msbuild.tasks/coverlet.msbuild.targets +++ b/src/coverlet.msbuild.tasks/coverlet.msbuild.targets @@ -39,7 +39,8 @@ SingleHit="$(SingleHit)" MergeWith="$(MergeWith)" UseSourceLink="$(UseSourceLink)" - SkipAutoProps="$(SkipAutoProps)" > + SkipAutoProps="$(SkipAutoProps)" + DoesNotReturnAttribute="$(DoesNotReturnAttribute)"> diff --git a/test/coverlet.collector.tests/CoverletSettingsParserTests.cs b/test/coverlet.collector.tests/CoverletSettingsParserTests.cs index dd56a6572..9de9993d5 100644 --- a/test/coverlet.collector.tests/CoverletSettingsParserTests.cs +++ b/test/coverlet.collector.tests/CoverletSettingsParserTests.cs @@ -43,23 +43,24 @@ public void ParseShouldSelectFirstTestModuleFromTestModulesList() } [Theory] - [InlineData("[*]*,[coverlet]*", "[coverlet.*.tests?]*,[coverlet.*.tests.*]*", @"E:\temp,/var/tmp", "module1.cs,module2.cs", "Obsolete,GeneratedCodeAttribute,CompilerGeneratedAttribute")] - [InlineData("[*]*,,[coverlet]*", "[coverlet.*.tests?]*,,[coverlet.*.tests.*]*", @"E:\temp,,/var/tmp", "module1.cs,,module2.cs", "Obsolete,,GeneratedCodeAttribute,,CompilerGeneratedAttribute")] - [InlineData("[*]*, ,[coverlet]*", "[coverlet.*.tests?]*, ,[coverlet.*.tests.*]*", @"E:\temp, ,/var/tmp", "module1.cs, ,module2.cs", "Obsolete, ,GeneratedCodeAttribute, ,CompilerGeneratedAttribute")] - [InlineData("[*]*,\t,[coverlet]*", "[coverlet.*.tests?]*,\t,[coverlet.*.tests.*]*", "E:\\temp,\t,/var/tmp", "module1.cs,\t,module2.cs", "Obsolete,\t,GeneratedCodeAttribute,\t,CompilerGeneratedAttribute")] - [InlineData("[*]*, [coverlet]*", "[coverlet.*.tests?]*, [coverlet.*.tests.*]*", @"E:\temp, /var/tmp", "module1.cs, module2.cs", "Obsolete, GeneratedCodeAttribute, CompilerGeneratedAttribute")] - [InlineData("[*]*,\t[coverlet]*", "[coverlet.*.tests?]*,\t[coverlet.*.tests.*]*", "E:\\temp,\t/var/tmp", "module1.cs,\tmodule2.cs", "Obsolete,\tGeneratedCodeAttribute,\tCompilerGeneratedAttribute")] - [InlineData("[*]*, \t[coverlet]*", "[coverlet.*.tests?]*, \t[coverlet.*.tests.*]*", "E:\\temp, \t/var/tmp", "module1.cs, \tmodule2.cs", "Obsolete, \tGeneratedCodeAttribute, \tCompilerGeneratedAttribute")] - [InlineData("[*]*,\r\n[coverlet]*", "[coverlet.*.tests?]*,\r\n[coverlet.*.tests.*]*", "E:\\temp,\r\n/var/tmp", "module1.cs,\r\nmodule2.cs", "Obsolete,\r\nGeneratedCodeAttribute,\r\nCompilerGeneratedAttribute")] - [InlineData("[*]*, \r\n [coverlet]*", "[coverlet.*.tests?]*, \r\n [coverlet.*.tests.*]*", "E:\\temp, \r\n /var/tmp", "module1.cs, \r\n module2.cs", "Obsolete, \r\n GeneratedCodeAttribute, \r\n CompilerGeneratedAttribute")] - [InlineData("[*]*,\t\r\n\t[coverlet]*", "[coverlet.*.tests?]*,\t\r\n\t[coverlet.*.tests.*]*", "E:\\temp,\t\r\n\t/var/tmp", "module1.cs,\t\r\n\tmodule2.cs", "Obsolete,\t\r\n\tGeneratedCodeAttribute,\t\r\n\tCompilerGeneratedAttribute")] - [InlineData("[*]*, \t \r\n \t [coverlet]*", "[coverlet.*.tests?]*, \t \r\n \t [coverlet.*.tests.*]*", "E:\\temp, \t \r\n \t /var/tmp", "module1.cs, \t \r\n \t module2.cs", "Obsolete, \t \r\n \t GeneratedCodeAttribute, \t \r\n \t CompilerGeneratedAttribute")] - [InlineData(" [*]* , [coverlet]* ", " [coverlet.*.tests?]* , [coverlet.*.tests.*]* ", " E:\\temp , /var/tmp ", " module1.cs , module2.cs ", " Obsolete , GeneratedCodeAttribute , CompilerGeneratedAttribute ")] + [InlineData("[*]*,[coverlet]*", "[coverlet.*.tests?]*,[coverlet.*.tests.*]*", @"E:\temp,/var/tmp", "module1.cs,module2.cs", "Obsolete,GeneratedCodeAttribute,CompilerGeneratedAttribute", "DoesNotReturnAttribute,ThrowsAttribute")] + [InlineData("[*]*,,[coverlet]*", "[coverlet.*.tests?]*,,[coverlet.*.tests.*]*", @"E:\temp,,/var/tmp", "module1.cs,,module2.cs", "Obsolete,,GeneratedCodeAttribute,,CompilerGeneratedAttribute", "DoesNotReturnAttribute,,ThrowsAttribute")] + [InlineData("[*]*, ,[coverlet]*", "[coverlet.*.tests?]*, ,[coverlet.*.tests.*]*", @"E:\temp, ,/var/tmp", "module1.cs, ,module2.cs", "Obsolete, ,GeneratedCodeAttribute, ,CompilerGeneratedAttribute", "DoesNotReturnAttribute, ,ThrowsAttribute")] + [InlineData("[*]*,\t,[coverlet]*", "[coverlet.*.tests?]*,\t,[coverlet.*.tests.*]*", "E:\\temp,\t,/var/tmp", "module1.cs,\t,module2.cs", "Obsolete,\t,GeneratedCodeAttribute,\t,CompilerGeneratedAttribute", "DoesNotReturnAttribute,\t,ThrowsAttribute")] + [InlineData("[*]*, [coverlet]*", "[coverlet.*.tests?]*, [coverlet.*.tests.*]*", @"E:\temp, /var/tmp", "module1.cs, module2.cs", "Obsolete, GeneratedCodeAttribute, CompilerGeneratedAttribute", "DoesNotReturnAttribute, ThrowsAttribute")] + [InlineData("[*]*,\t[coverlet]*", "[coverlet.*.tests?]*,\t[coverlet.*.tests.*]*", "E:\\temp,\t/var/tmp", "module1.cs,\tmodule2.cs", "Obsolete,\tGeneratedCodeAttribute,\tCompilerGeneratedAttribute", "DoesNotReturnAttribute,\tThrowsAttribute")] + [InlineData("[*]*, \t[coverlet]*", "[coverlet.*.tests?]*, \t[coverlet.*.tests.*]*", "E:\\temp, \t/var/tmp", "module1.cs, \tmodule2.cs", "Obsolete, \tGeneratedCodeAttribute, \tCompilerGeneratedAttribute", "DoesNotReturnAttribute, \tThrowsAttribute")] + [InlineData("[*]*,\r\n[coverlet]*", "[coverlet.*.tests?]*,\r\n[coverlet.*.tests.*]*", "E:\\temp,\r\n/var/tmp", "module1.cs,\r\nmodule2.cs", "Obsolete,\r\nGeneratedCodeAttribute,\r\nCompilerGeneratedAttribute", "DoesNotReturnAttribute,\r\nThrowsAttribute")] + [InlineData("[*]*, \r\n [coverlet]*", "[coverlet.*.tests?]*, \r\n [coverlet.*.tests.*]*", "E:\\temp, \r\n /var/tmp", "module1.cs, \r\n module2.cs", "Obsolete, \r\n GeneratedCodeAttribute, \r\n CompilerGeneratedAttribute", "DoesNotReturnAttribute, \r\n ThrowsAttribute")] + [InlineData("[*]*,\t\r\n\t[coverlet]*", "[coverlet.*.tests?]*,\t\r\n\t[coverlet.*.tests.*]*", "E:\\temp,\t\r\n\t/var/tmp", "module1.cs,\t\r\n\tmodule2.cs", "Obsolete,\t\r\n\tGeneratedCodeAttribute,\t\r\n\tCompilerGeneratedAttribute", "DoesNotReturnAttribute,\t\r\n\tThrowsAttribute")] + [InlineData("[*]*, \t \r\n \t [coverlet]*", "[coverlet.*.tests?]*, \t \r\n \t [coverlet.*.tests.*]*", "E:\\temp, \t \r\n \t /var/tmp", "module1.cs, \t \r\n \t module2.cs", "Obsolete, \t \r\n \t GeneratedCodeAttribute, \t \r\n \t CompilerGeneratedAttribute", "DoesNotReturnAttribute, \t \r\n \t ThrowsAttribute")] + [InlineData(" [*]* , [coverlet]* ", " [coverlet.*.tests?]* , [coverlet.*.tests.*]* ", " E:\\temp , /var/tmp ", " module1.cs , module2.cs ", " Obsolete , GeneratedCodeAttribute , CompilerGeneratedAttribute ", "DoesNotReturnAttribute , ThrowsAttribute")] public void ParseShouldCorrectlyParseConfigurationElement(string includeFilters, string excludeFilters, string includeDirectories, string excludeSourceFiles, - string excludeAttributes) + string excludeAttributes, + string doesNotReturnAttributes) { var testModules = new List { "abc.dll" }; var doc = new XmlDocument(); @@ -74,6 +75,7 @@ public void ParseShouldCorrectlyParseConfigurationElement(string includeFilters, this.CreateCoverletNodes(doc, configElement, CoverletConstants.SingleHitElementName, "true"); this.CreateCoverletNodes(doc, configElement, CoverletConstants.IncludeTestAssemblyElementName, "true"); this.CreateCoverletNodes(doc, configElement, CoverletConstants.SkipAutoProps, "true"); + this.CreateCoverletNodes(doc, configElement, CoverletConstants.DoesNotReturnAttributesElementName, doesNotReturnAttributes); CoverletSettings coverletSettings = _coverletSettingsParser.Parse(configElement, testModules); @@ -91,6 +93,8 @@ public void ParseShouldCorrectlyParseConfigurationElement(string includeFilters, Assert.Equal("[coverlet.*]*", coverletSettings.ExcludeFilters[0]); Assert.Equal("[coverlet.*.tests?]*", coverletSettings.ExcludeFilters[1]); Assert.Equal("[coverlet.*.tests.*]*", coverletSettings.ExcludeFilters[2]); + Assert.Equal("DoesNotReturnAttribute", coverletSettings.DoesNotReturnAttributes[0]); + Assert.Equal("ThrowsAttribute", coverletSettings.DoesNotReturnAttributes[1]); Assert.False(coverletSettings.UseSourceLink); Assert.True(coverletSettings.SingleHit); From 5a16f1b9ed9bf957348af9698984f931cfe9f481 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Fri, 21 Aug 2020 18:37:40 -0400 Subject: [PATCH 15/18] document new settings for attributes --- Documentation/GlobalTool.md | 41 +++++++++++++++-------------- Documentation/MSBuildIntegration.md | 7 +++++ Documentation/VSTestIntegration.md | 1 + 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/Documentation/GlobalTool.md b/Documentation/GlobalTool.md index bb76dc9e5..283afcb6b 100644 --- a/Documentation/GlobalTool.md +++ b/Documentation/GlobalTool.md @@ -17,26 +17,27 @@ Arguments: Path to the test assembly. Options: - -h|--help Show help information - -v|--version Show version information - -t|--target Path to the test runner application. - -a|--targetargs Arguments to be passed to the test runner. - -o|--output Output of the generated coverage report - -v|--verbosity Sets the verbosity level of the command. Allowed values are quiet, minimal, normal, detailed. - -f|--format Format of the generated coverage report[multiple value]. - --threshold Exits with error if the coverage % is below value. - --threshold-type Coverage type to apply the threshold to[multiple value]. - --threshold-stat Coverage statistic used to enforce the threshold value. - --exclude Filter expressions to exclude specific modules and types[multiple value]. - --include Filter expressions to include specific modules and types[multiple value]. - --include-directory Include directories containing additional assemblies to be instrumented[multiple value]. - --exclude-by-file Glob patterns specifying source files to exclude[multiple value]. - --exclude-by-attribute Attributes to exclude from code coverage[multiple value]. - --include-test-assembly Specifies whether to report code coverage of the test assembly. - --single-hit Specifies whether to limit code coverage hit reporting to a single hit for each location. - --merge-with Path to existing coverage result to merge. - --use-source-link Specifies whether to use SourceLink URIs in place of file system paths. - --skipautoprops Neither track nor record auto-implemented properties. + -h|--help Show help information + -v|--version Show version information + -t|--target Path to the test runner application. + -a|--targetargs Arguments to be passed to the test runner. + -o|--output Output of the generated coverage report + -v|--verbosity Sets the verbosity level of the command. Allowed values are quiet, minimal, normal, detailed. + -f|--format Format of the generated coverage report[multiple value]. + --threshold Exits with error if the coverage % is below value. + --threshold-type Coverage type to apply the threshold to[multiple value]. + --threshold-stat Coverage statistic used to enforce the threshold value. + --exclude Filter expressions to exclude specific modules and types[multiple value]. + --include Filter expressions to include specific modules and types[multiple value]. + --include-directory Include directories containing additional assemblies to be instrumented[multiple value]. + --exclude-by-file Glob patterns specifying source files to exclude[multiple value]. + --exclude-by-attribute Attributes to exclude from code coverage[multiple value]. + --include-test-assembly Specifies whether to report code coverage of the test assembly. + --single-hit Specifies whether to limit code coverage hit reporting to a single hit for each location. + --merge-with Path to existing coverage result to merge. + --use-source-link Specifies whether to use SourceLink URIs in place of file system paths. + --skipautoprops Neither track nor record auto-implemented properties. + --does-not-return-attribute Attributes that mark methods that do not return[multiple value]. ``` NB. For a [multiple value] options you have to specify values multiple times i.e. diff --git a/Documentation/MSBuildIntegration.md b/Documentation/MSBuildIntegration.md index cd54a8e9d..7d6631693 100644 --- a/Documentation/MSBuildIntegration.md +++ b/Documentation/MSBuildIntegration.md @@ -162,6 +162,13 @@ You can also include coverage of the test assembly itself by setting `/p:Include Neither track nor record auto-implemented properties. Syntax: `/p:SkipAutoProps=true` +### Methods that do not return + +Methods that do not return can be marked with attributes to cause statements after them to be excluded from coverage. `DoesNotReturnAttribute` is included by default. + +Attributes can be specified with the following syntax. +Syntax: `/p:DoesNotReturnAttribute="DoesNotReturnAttribute,OtherAttribute"` + ### Note for Powershell / VSTS users To exclude or include multiple assemblies when using Powershell scripts or creating a .yaml file for a VSTS build ```%2c``` should be used as a separator. Msbuild will translate this symbol to ```,```. diff --git a/Documentation/VSTestIntegration.md b/Documentation/VSTestIntegration.md index cff5e299a..47fa12f0d 100644 --- a/Documentation/VSTestIntegration.md +++ b/Documentation/VSTestIntegration.md @@ -80,6 +80,7 @@ These are a list of options that are supported by coverlet. These can be specifi |UseSourceLink | Specifies whether to use SourceLink URIs in place of file system paths. | |IncludeTestAssembly | Include coverage of the test assembly. | |SkipAutoProps | Neither track nor record auto-implemented properties. | +|DoesNotReturnAttribute | Methods marked with these attributes are known not to return, statements following them will be excluded from coverage | How to specify these options via runsettings? ``` From b636ab4e204d6fdbd312d79d1d5989e6f8f949f5 Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Sat, 22 Aug 2020 15:05:53 -0400 Subject: [PATCH 16/18] Update src/coverlet.core/Attributes/DoesNotReturnAttribute.cs Co-authored-by: Marco Rossignoli --- src/coverlet.core/Attributes/DoesNotReturnAttribute.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coverlet.core/Attributes/DoesNotReturnAttribute.cs b/src/coverlet.core/Attributes/DoesNotReturnAttribute.cs index 0a5738130..a35be1336 100644 --- a/src/coverlet.core/Attributes/DoesNotReturnAttribute.cs +++ b/src/coverlet.core/Attributes/DoesNotReturnAttribute.cs @@ -3,5 +3,5 @@ namespace Coverlet.Core.Attributes { [AttributeUsage(AttributeTargets.Property | AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class)] - public class DoesNotReturnAttribute : Attribute { } + internal class DoesNotReturnAttribute : Attribute { } } From 5aac0317e66715522f9a8bd71a991b5620ff335e Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Sat, 22 Aug 2020 15:06:05 -0400 Subject: [PATCH 17/18] Update src/coverlet.core/Instrumentation/Instrumenter.cs Co-authored-by: Marco Rossignoli --- src/coverlet.core/Instrumentation/Instrumenter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index d7768bfc3..08e52b56d 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -190,7 +190,7 @@ private bool Is_System_Threading_Interlocked_CoreLib_Type(TypeDefinition type) } // Have to do this before we start writing to a module, as we'll get into file - // locking issues if we do it while writing. + // locking issues if we do it while writing. private void CreateReachabilityHelper() { using (var stream = _fileSystem.NewFileStream(_module, FileMode.Open, FileAccess.Read)) From 4fc520298ae1f0668700fd15b5b10fecd756cafb Mon Sep 17 00:00:00 2001 From: kevin-montrose Date: Sat, 22 Aug 2020 15:06:56 -0400 Subject: [PATCH 18/18] Add a blank line per https://github.com/coverlet-coverage/coverlet/pull/904/files/5a16f1b9ed9bf957348af9698984f931cfe9f481#r475060124 --- src/coverlet.core/Instrumentation/Instrumenter.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index 08e52b56d..e74dc051d 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -5,6 +5,7 @@ using System.IO; using System.Linq; using System.Runtime.CompilerServices; + using Coverlet.Core.Instrumentation.Reachability; using Coverlet.Core.Abstractions; using Coverlet.Core.Attributes;