Uh oh!
There was an error while loading. Please reload this page.
Consume ISOSDacInterface17 for structured stress-log reading - #1492
Conversation
Pure file rename with no content changes, so git records it as a 100% rename and history/blame follow through to the new path. The subsequent commit turns StressLog into an abstract base and reworks the moved file into the LegacyStressLog subclass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make StressLog an abstract base with two concrete subclasses: * LegacyStressLog -- the original raw-memory parser (the renamed file), used for the legacy DAC / older runtimes and behind the address-based StressLog.TryOpen overloads. * ContractStressLog -- backed by the cDAC's ISOSDacInterface17 via the IAbstractStressLog DAC service; threads, messages, resolved arguments, and chunk memory ranges all come from the structured contract. ClrRuntime.TryGetStressLog prefers the IS17 contract when the cDAC is loaded and transparently falls back to the raw parser otherwise. Adds the SosDac17 COM wrapper + enumerators, the IAbstractStressLog abstraction and its DacStressLog implementation, and an IS17/raw parity test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
42f8211 to
905e8ccCompareThere was a problem hiding this comment.
Pull request overview
This PR updates ClrMD’s stress-log reader to prefer the structured DAC contract (ISOSDacInterface17) when a cDAC is loaded, while keeping the existing raw-memory parser as a fallback for older runtimes / legacy DAC scenarios. It introduces an abstract StressLog base with separate legacy vs contract-backed implementations and adds DAC plumbing + tests to validate parity.
Changes:
- Refactors
StressLoginto an abstract base class and moves the existing raw parser intoLegacyStressLog. - Adds a cDAC-backed stress-log implementation (
ContractStressLog) built on a newIAbstractStressLogservice implemented viaDacStressLog+ newISOSDac17COM wrappers. - Adds a parity test to compare IS17 vs raw parser outputs on the same dump (when a usable cDAC is present).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Diagnostics.Runtime/StressLog/StressLog.cs | Refactors public surface into an abstract base and redirects address-based opens to the legacy parser. |
| src/Microsoft.Diagnostics.Runtime/StressLog/LegacyStressLog.cs | Contains the original raw-memory stress-log parser implementation (moved/renamed). |
| src/Microsoft.Diagnostics.Runtime/StressLog/Internal/StressLogEnumerationContext.cs | Generalizes per-enumeration state to support both iterator-based and contract-provided arguments. |
| src/Microsoft.Diagnostics.Runtime/StressLog/ContractStressLog.cs | Adds the cDAC/IS17-backed stress-log reader that merges per-thread message streams. |
| src/Microsoft.Diagnostics.Runtime/DacInterface/SosDac17.cs | Introduces COM wrappers/enumerators for ISOSDacInterface17 stress-log APIs. |
| src/Microsoft.Diagnostics.Runtime/DacInterface/ISOSDac17.cs | Defines interop structs and the internal ISOSDac17 wrapper interface. |
| src/Microsoft.Diagnostics.Runtime/DacInterface/ClrDataProcess.cs | Adds CreateSOSDacInterface17 to QI for the IS17 interface. |
| src/Microsoft.Diagnostics.Runtime/DacImplementation/DacStressLog.cs | Implements IAbstractStressLog by translating IS17 COM enumerators into managed contract types. |
| src/Microsoft.Diagnostics.Runtime/DacImplementation/DacServiceProvider.cs | Wires up IAbstractStressLog service creation and disposes the IS17 wrapper. |
| src/Microsoft.Diagnostics.Runtime/ClrRuntime.cs | Prefers the contract-backed stress-log path during probing (with fallback considerations). |
| src/Microsoft.Diagnostics.Runtime/AbstractDac/IAbstractStressLog.cs | Adds the public abstract DAC contract for stress-log access and associated data structs. |
| src/Microsoft.Diagnostics.Runtime.Tests/src/StressLogIs17ParityTests.cs | Adds parity tests to validate IS17 vs legacy parser output equivalence (when cDAC available). |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Lee Culver (leculver)
left a comment
There was a problem hiding this comment.
Overall looks fine, a few bits of feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e812c27 to
6201ee8Compare* ContractStressLog.EnumerateMemoryRanges: validate each range and raise a CorruptChunk diagnostic (then skip) when start + size overflows, matching the raw path's ChunkFits guard instead of yielding an invalid range. * ContractStressLog.EnumerateMessages: raise a LimitExceeded diagnostic when the MaxThreads cap is hit, matching the raw path, rather than silently dropping later threads' messages. * DacStressLog.EnumerateMessages: clamp each message's argument count to StressLogConstants.MaxArgumentCount and use a fixed buffer so corrupt data cannot drive an unbounded allocation. * IAbstractStressLog: fix doc typo (used read -> used to read). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
These tests load a Core full dump and compare the IS17 and raw stress-log paths -- nothing Windows-specific, matching the sibling [Fact] CanOpenStressLog_Core_Workstation/_Server integration tests. On platforms without a co-located cDAC they simply take the existing no-op path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Summary
Makes ClrMD consume
ISOSDacInterface17(IS17) for stress-log reading. When the cDAC is loaded, the stress-log header, threads, messages (with resolved arguments), and chunk memory ranges come from the structured DAC contract instead of ClrMD re-parsing raw target memory. Older runtimes / the legacy C++ DAC transparently fall back to the existing raw-memory parser.Design
StressLogbecomes an abstract base (no public ABI break) with two concrete subclasses:LegacyStressLog-- the original raw-memory parser (this is the renamedStressLog.cs; the first commit is a pure rename so history/blame follow through). Used for the legacy DAC / older runtimes and behind the address-basedStressLog.TryOpenoverloads.ContractStressLog-- backed by the cDAC'sISOSDacInterface17via a newIAbstractStressLogDAC service (DacStressLog).ClrRuntime.TryGetStressLogprefers the IS17 contract when the cDAC is available and falls back to the raw parser otherwise.Also adds the
SosDac17COM wrapper + stress-log enumerators and a parity test that verifies the IS17 path and the raw parser produce identical messages and memory ranges for the same dump.Relies on dotnet/runtime#130038 for enumerating stress log memory