Merge branch 'main-master' into memory-mapped-hit-counts

# Conflicts:
#	src/coverlet.core/Instrumentation/Instrumenter.cs
#	src/coverlet.template/ModuleTrackerTemplate.cs

Author: petli

README.md

@@ -440,7 +440,7 @@ There is a simple performance test for the hit counting instrumentation in the t
 
 The duration of the test can be tweaked by changing the number of iterations in the `[InlineData]` in the `PerformanceTest` class.
 
-For more realistic testing it is recommended to try out any changes to the hit counting code paths on large, realistic projects.  If you don't have any handy https://github.com/dotnet/corefx is an excellent candidate.  [This page](https://github.com/dotnet/corefx/blob/master/Documentation/building/code-coverage.md) describes how to run code coverage tests for both the full solution and for individual projects with coverlet from nuget.  Suitable projects (listed in order of escalating test durations):
+For more realistic testing it is recommended to try out any changes to the hit counting code paths on large, realistic projects.  If you don't have any handy https://github.com/dotnet/corefx is an excellent candidate.  [This page](https://github.com/dotnet/corefx/blob/master/Documentation/building/code-coverage.md) describes how to run code coverage tests for both the full solution and for individual projects with coverlet from nuget. Suitable projects (listed in order of escalating test durations):
 
 * System.Collections.Concurrent.Tests
 * System.Collections.Tests
@@ -451,22 +451,10 @@ For more realistic testing it is recommended to try out any changes to the hit c
 Change to the directory of the library and run the msbuild code coverage command:
 
     dotnet msbuild /t:BuildAndTest /p:Coverage=true
-
-Look for a line like this:
-
-    ----- start 18:13:36,59 ===============  To repro directly: =====================================================
-
-It is followed by a `pushd` command into the artefact directory, and a command to run coverlet.  Run these to get to see the coverlet output and especially the test duration. E.g.:
-
-    C:\...\corefx\artifacts\tools\coverlet "System.Collections.Concurrent.Tests.dll" --target ...
-
+    
 To run with a development version of coverlet call `dotnet run` instead of the installed coverlet version, e.g.:
 
-    dotnet run -p C:\...\coverlet\src\coverlet.console\coverlet.console.csproj -c Release -- "System.Collections.Concurrent.Tests.dll" --target ...
-
-
-
-
+    dotnet msbuild /t:BuildAndTest /p:Coverage=true /p:CoverageExecutablePath="dotnet run -p C:\...\coverlet\src\coverlet.console\coverlet.console.csproj"
 
 ## Code of Conduct
 

src/coverlet.core/Instrumentation/Instrumenter.cs

@@ -29,6 +29,7 @@ internal class Instrumenter
         private FieldDefinition _customTrackerHitsMemoryMapName;
         private ILProcessor _customTrackerClassConstructorIl;
         private TypeDefinition _customTrackerTypeDef;
+        private MethodReference _customTrackerRegisterUnloadEventsMethod;
         private MethodReference _customTrackerRecordHitMethod;
 
         public Instrumenter(string module, string identifier, string[] excludeFilters, string[] includeFilters, string[] excludedFiles, string[] excludedAttributes)
@@ -77,6 +78,7 @@ private void InstrumentModule()
 
                 using (var module = ModuleDefinition.ReadModule(stream, parameters))
                 {
+                    var containsAppContext = module.GetType(nameof(System), nameof(AppContext)) != null;
                     var types = module.GetTypes();
                     AddCustomModuleTrackerToModule(module);
 
@@ -96,15 +98,50 @@ private void InstrumentModule()
                     }
 
                     // Fixup the custom tracker class constructor, according to all instrumented types
-                    Instruction firstInstr = _customTrackerClassConstructorIl.Body.Instructions.First();
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Nop));
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsFilePath));
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsFilePath));
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Ldc_I4, _result.HitCandidates.Count));
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Newarr, module.TypeSystem.Int32));
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsArray));
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsResultGuid));
-                    _customTrackerClassConstructorIl.InsertBefore(firstInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsMemoryMapName));
+                    if (_customTrackerRegisterUnloadEventsMethod == null)
+                    {
+                        _customTrackerRegisterUnloadEventsMethod = new MethodReference(
+                            nameof(ModuleTrackerTemplate.RegisterUnloadEvents), module.TypeSystem.Void, _customTrackerTypeDef);
+                    }
+
+                    Instruction lastInstr = _customTrackerClassConstructorIl.Body.Instructions.Last();
+
+                    if (!containsAppContext)
+                    {
+                        // For "normal" cases, where the instrumented assembly is not the core library, we add a call to
+                        // RegisterUnloadEvents to the static constructor of the generated custom tracker. Due to static
+                        // initialization constraints, the core library is handled separately below.
+                        _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Call, _customTrackerRegisterUnloadEventsMethod));
+                    }
+
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldc_I4, _result.HitCandidates.Count));
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Newarr, module.TypeSystem.Int32));
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsArray));
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsFilePath));
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsFilePath));
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldstr, _result.HitsResultGuid));
+                    _customTrackerClassConstructorIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Stsfld, _customTrackerHitsMemoryMapName));
+
+                    if (containsAppContext)
+                    {
+                        // Handle the core library by instrumenting System.AppContext.OnProcessExit to directly call
+                        // the UnloadModule method of the custom tracker type. This avoids loops between the static
+                        // initialization of the custom tracker and the static initialization of the hosting AppDomain
+                        // (which for the core library case will be instrumented code).
+                        var eventArgsType = new TypeReference(nameof(System), nameof(EventArgs), module, module.TypeSystem.CoreLibrary);
+                        var customTrackerUnloadModule = new MethodReference(nameof(ModuleTrackerTemplate.UnloadModule), module.TypeSystem.Void, _customTrackerTypeDef);
+                        customTrackerUnloadModule.Parameters.Add(new ParameterDefinition(module.TypeSystem.Object));
+                        customTrackerUnloadModule.Parameters.Add(new ParameterDefinition(eventArgsType));
+
+                        var appContextType = new TypeReference(nameof(System), nameof(AppContext), module, module.TypeSystem.CoreLibrary);
+                        var onProcessExitMethod = new MethodReference("OnProcessExit", module.TypeSystem.Void, appContextType).Resolve();
+                        var onProcessExitIl = onProcessExitMethod.Body.GetILProcessor();
+
+                        lastInstr = onProcessExitIl.Body.Instructions.Last();
+                        onProcessExitIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldnull));
+                        onProcessExitIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Ldnull));
+                        onProcessExitIl.InsertBefore(lastInstr, Instruction.Create(OpCodes.Call, customTrackerUnloadModule));
+                    }
 
                     module.Write(stream);
                 }
@@ -141,12 +178,9 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module)
                 {
                     MethodDefinition methodOnCustomType = new MethodDefinition(methodDef.Name, methodDef.Attributes, methodDef.ReturnType);
 
-                    if (methodDef.Name == nameof(ModuleTrackerTemplate.RecordHit))
+                    foreach (var parameter in methodDef.Parameters)
                     {
-                        foreach (var parameter in methodDef.Parameters)
-                        {
-                            methodOnCustomType.Parameters.Add(new ParameterDefinition(module.ImportReference(parameter.ParameterType)));
-                        }
+                        methodOnCustomType.Parameters.Add(new ParameterDefinition(module.ImportReference(parameter.ParameterType)));
                     }
 
                     foreach (var variable in methodDef.Body.Variables)
@@ -172,8 +206,11 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module)
                             else
                             {
                                 // Move to the custom type
-                                instr.Operand = new MethodReference(
-                                    methodReference.Name, methodReference.ReturnType, _customTrackerTypeDef);
+                                var updatedMethodReference = new MethodReference(methodReference.Name, methodReference.ReturnType, _customTrackerTypeDef);
+                                foreach (var parameter in methodReference.Parameters)
+                                    updatedMethodReference.Parameters.Add(new ParameterDefinition(parameter.Name, parameter.Attributes, module.ImportReference(parameter.ParameterType)));
+
+                                instr.Operand = updatedMethodReference;
                             }
                         }
                         else if (instr.Operand is FieldReference fieldReference)
@@ -204,6 +241,7 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module)
                 module.Types.Add(_customTrackerTypeDef);
             }
 
+            Debug.Assert(_customTrackerHitsArray != null);
             Debug.Assert(_customTrackerClassConstructorIl != null);
         }
 

src/coverlet.template/ModuleTrackerTemplate.cs

@@ -26,33 +26,36 @@ public static class ModuleTrackerTemplate
         public static string HitsMemoryMapName;
         public static int[] HitsArray;
 
-        // Special case when instrumenting CoreLib, the static below prevents infinite loop in CoreLib
-        // while allowing the tracker template to call any of its types and functions.
-        private static bool s_isTracking;
-
         static ModuleTrackerTemplate()
         {
-            s_isTracking = true;
-            AppDomain.CurrentDomain.ProcessExit += new EventHandler(UnloadModule);
-            AppDomain.CurrentDomain.DomainUnload += new EventHandler(UnloadModule);
-            s_isTracking = false;
-
             // At the end of the instrumentation of a module, the instrumenter needs to add code here
             // to initialize the static fields according to the values derived from the instrumentation of
             // the module.
         }
 
+        // A call to this method will be injected in the static constructor above for most cases. However, if the
+        // current assembly is System.Private.CoreLib (or more specifically, defines System.AppDomain), a call directly
+        // to UnloadModule will be injected in System.AppContext.OnProcessExit.
+        public static void RegisterUnloadEvents()
+        {
+            AppDomain.CurrentDomain.ProcessExit += new EventHandler(UnloadModule);
+            AppDomain.CurrentDomain.DomainUnload += new EventHandler(UnloadModule);
+        }
+
         public static void RecordHit(int hitLocationIndex)
         {
-            if (s_isTracking)
+            // Make sure to avoid recording if this is a call to RecordHit within the AppDomain setup code in an
+            // instrumented build of System.Private.CoreLib.
+            if (HitsArray is null)
                 return;
 
             Interlocked.Increment(ref HitsArray[hitLocationIndex]);
         }
 
         public static void UnloadModule(object sender, EventArgs e)
         {
-            s_isTracking = true;
+            // Claim the current hits array and reset it to prevent double-counting scenarios.
+            var hitsArray = Interlocked.Exchange(ref HitsArray, new int[HitsArray.Length]);
 
             // The same module can be unloaded multiple times in the same process via different app domains.
             // Use a global mutex to ensure no concurrent access.
@@ -91,10 +94,12 @@ public static void UnloadModule(object sender, EventArgs e)
                                 // the shared data.
                                 Interlocked.Increment(ref *(intPointer + HitsResultUnloadStarted));
 
-                                for (var i = 0; i < HitsArray.Length; i++)
+                                for (var i = 0; i < hitsArray.Length; i++)
                                 {
-                                    var count = HitsArray[i];
+                                    var count = hitsArray[i];
 
+                                    // By only modifying the memory map pages where there have been hits
+                                    // unnecessary allocation of all-zero pages is avoided.
                                     if (count > 0)
                                     {
                                         var hitLocationArrayOffset = intPointer + i + HitsResultHeaderSize;
@@ -117,15 +122,10 @@ public static void UnloadModule(object sender, EventArgs e)
                 }
                 finally
                 {
-                    // Avoid double counting if unloaded multiple times (if that can happen?)
-                    HitsArray = new int[HitsArray.Length];
-                    
                     mutex.ReleaseMutex();
                     memoryMap?.Dispose();
                 }
             }
-
-            s_isTracking = false;
         }
     }
 }