Skip to content

Remove reliance on s_isTracking to detect recursion #297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 16, 2019
Merged

Remove reliance on s_isTracking to detect recursion #297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92b26a4
Remove reliance on s_isTracking to detect recursion
sharwell Jan 15, 2019
96b1594
Remove the need to register events when instrumenting System.Private.…
sharwell Jan 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 45 additions & 7 deletions src/coverlet.core/Instrumentation/Instrumenter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ internal class Instrumenter
private FieldDefinition _customTrackerHitsFilePath;
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)
Expand Down Expand Up @@ -76,6 +77,7 @@ private void InstrumentModule()

using (var module = ModuleDefinition.ReadModule(stream, parameters))
{
var containsAppContext = module.GetType(nameof(System), nameof(AppContext)) != null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 This could be modified to also check the name of the containing assembly, but it didn't seem necessary.

var types = module.GetTypes();
AddCustomModuleTrackerToModule(module);

Expand All @@ -95,13 +97,49 @@ private void InstrumentModule()
}

// Fixup the custom tracker class constructor, according to all instrumented types
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));

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);
}
}
Expand Down Expand Up @@ -135,12 +173,9 @@ private void AddCustomModuleTrackerToModule(ModuleDefinition module)
{
MethodDefinition methodOnCustomType = new MethodDefinition(methodDef.Name, methodDef.Attributes, methodDef.ReturnType);

if (methodDef.Name == "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)
Expand All @@ -166,8 +201,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)
Expand Down
39 changes: 19 additions & 20 deletions src/coverlet.template/ModuleTrackerTemplate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,33 +20,36 @@ public static class ModuleTrackerTemplate
public static string HitsFilePath;
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.
Expand All @@ -61,8 +64,8 @@ public static void UnloadModule(object sender, EventArgs e)
using (var fs = new FileStream(HitsFilePath, FileMode.CreateNew))
using (var bw = new BinaryWriter(fs))
{
bw.Write(HitsArray.Length);
foreach (int hitCount in HitsArray)
bw.Write(hitsArray.Length);
foreach (int hitCount in hitsArray)
{
bw.Write(hitCount);
}
Expand All @@ -82,25 +85,21 @@ public static void UnloadModule(object sender, EventArgs e)
using (var bw = new BinaryWriter(fs))
{
int hitsLength = br.ReadInt32();
if (hitsLength != HitsArray.Length)
if (hitsLength != hitsArray.Length)
{
throw new InvalidOperationException(
$"{HitsFilePath} has {hitsLength} entries but on memory {nameof(HitsArray)} has {HitsArray.Length}");
$"{HitsFilePath} has {hitsLength} entries but on memory {nameof(HitsArray)} has {hitsArray.Length}");
}

for (int i = 0; i < hitsLength; ++i)
{
int oldHitCount = br.ReadInt32();
bw.Seek(-sizeof(int), SeekOrigin.Current);
bw.Write(HitsArray[i] + oldHitCount);
bw.Write(hitsArray[i] + oldHitCount);
}
}
}

// Prevent any double counting scenario, i.e.: UnloadModule called twice (not sure if this can happen in practice ...)
// Only an issue if DomainUnload and ProcessExit can both happens, perhaps can be removed...
Array.Clear(HitsArray, 0, HitsArray.Length);

// On purpose this is not under a try-finally: it is better to have an exception if there was any error writing the hits file
// this case is relevant when instrumenting corelib since multiple processes can be running against the same instrumented dll.
mutex.ReleaseMutex();
Expand Down