Skip to content

Commit

Permalink
[cdac] Clear cached data as part of IXCLRDataProcess::Flush (#110700)
Browse files Browse the repository at this point in the history
Found this while running the diagnostics repo SOS tests with the cDAC enabled. General sequence for the repro was:
```
!sethostruntime -none
!bpmd <firstLocation>
!bpmd <secondLocation>
g
g
!clrstack
```
Printed stack shows `<unknown>` for some method(s).

Between the first and second breakpoints, more methods were jitted and the corresponding code heap list updated. When a new method in the stack for `<secondLocation>` had the same code heap list as any method from `<firstLocation>`, we'd end up with a stale end address for the heap list and determine that the method was invalid (outside the address range).

The cdac was assuming that the target would be created every time the state changes, but that is not the case (for the repro above, `!sethostruntime -none` resulted in not re-creating the target). We need to handle `IXCLRDataProcess::Flush` calls to clear out any cached data.

With this change, the SOS tests with the cDAC enabled run successfully with both a Release and Debug cdacreader (on a Release runtime).
  • Loading branch information
elinor-fung authored Dec 14, 2024
1 parent ed46aa2 commit d6c034d
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ public interface IDataCache
/// <param name="data">On return, set to the cached data value, or null if the data hasn't been cached yet.</param>
/// <returns>True if a copy of the data is cached, or false otherwise</returns>
bool TryGet<T>(ulong address, [NotNullWhen(true)] out T? data);
/// <summary>
/// Clear all cached data
/// </summary>
void Clear();
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,11 @@ public bool TryGet<T>(ulong address, [NotNullWhen(true)] out T? data)

return false;
}

public void Clear()
{
_readDataByAddress.Clear();
}
}

private readonly struct Reader(ReadFromTargetDelegate readFromTarget)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,15 @@ namespace Microsoft.Diagnostics.DataContractReader.Legacy;
internal sealed unsafe partial class SOSDacImpl : IXCLRDataProcess, IXCLRDataProcess2
{
int IXCLRDataProcess.Flush()
=> _legacyProcess is not null ? _legacyProcess.Flush() : HResults.E_NOTIMPL;
{
_target.ProcessedData.Clear();

// As long as any part of cDAC falls back to the legacy DAC, we need to propagate the Flush call
if (_legacyProcess is not null)
return _legacyProcess.Flush();

return HResults.S_OK;
}

int IXCLRDataProcess.StartEnumTasks(ulong* handle)
=> _legacyProcess is not null ? _legacyProcess.StartEnumTasks(handle) : HResults.E_NOTIMPL;
Expand Down
19 changes: 10 additions & 9 deletions src/native/managed/cdacreader/tests/TestPlaceholderTarget.cs
Original file line number Diff line number Diff line change
Expand Up @@ -230,19 +230,17 @@ public override Target.TypeInfo GetTypeInfo(DataType dataType)
public override ContractRegistry Contracts => _contractRegistry;

// A data cache that stores data in a dictionary and calls IData.Create to construct the data.
private class DefaultDataCache : Target.IDataCache
private sealed class DefaultDataCache : Target.IDataCache
{
protected readonly Target _target;
protected readonly Dictionary<(ulong, Type), object?> _readDataByAddress = [];
private readonly Target _target;
private readonly Dictionary<(ulong, Type), object?> _readDataByAddress = [];

public DefaultDataCache(Target target)
{
_target = target;
}

public virtual T GetOrAdd<T>(TargetPointer address) where T : Data.IData<T> => DefaultGetOrAdd<T>(address);

protected T DefaultGetOrAdd<T>(TargetPointer address) where T : Data.IData<T>
public T GetOrAdd<T>(TargetPointer address) where T : Data.IData<T>
{
if (TryGet(address, out T? result))
return result;
Expand All @@ -258,9 +256,7 @@ protected T DefaultGetOrAdd<T>(TargetPointer address) where T : Data.IData<T>
return result!;
}

public virtual bool TryGet<T>(ulong address, [NotNullWhen(true)] out T? data) => DefaultTryGet<T>(address, out data);

protected bool DefaultTryGet<T>(ulong address, [NotNullWhen(true)] out T? data)
public bool TryGet<T>(ulong address, [NotNullWhen(true)] out T? data)
{
data = default;
if (!_readDataByAddress.TryGetValue((address, typeof(T)), out object? dataObj))
Expand All @@ -273,6 +269,11 @@ protected bool DefaultTryGet<T>(ulong address, [NotNullWhen(true)] out T? data)
}
return false;
}

public void Clear()
{
_readDataByAddress.Clear();
}
}

}

0 comments on commit d6c034d

Please sign in to comment.