Skip to content

Add GetStressLogMemoryRanges to ISOSDacInterface17#130038

Merged
max-charlamb merged 1 commit into
mainfrom
max-charlamb/extend-isosdacinterface17
Jun 30, 2026
Merged

Add GetStressLogMemoryRanges to ISOSDacInterface17#130038
max-charlamb merged 1 commit into
mainfrom
max-charlamb/extend-isosdacinterface17

Conversation

@max-charlamb

@max-charlamb max-charlamb commented Jun 30, 2026

Copy link
Copy Markdown
Member

Summary

Adds GetStressLogMemoryRanges to ISOSDacInterface17. This interface has not been used by shipping products so I'm okay with this breaking change. The method enumerates the memory ranges (chunks) that back the in-memory stress log and returns them as an ISOSMemoryEnum, so diagnostic tools can discover and read stress-log memory regions. This functionality wasn't required by SOS, but is exposed by CLRMD.

Changes

  • sospriv.idl -- declare HRESULT GetStressLogMemoryRanges([out] ISOSMemoryEnum **ppEnum) on ISOSDacInterface17.
  • IStressLog (Abstractions) -- add a StressLogMemoryRange(TargetPointer Start, ulong Size) record and a GetStressLogMemoryRanges(StressLogData) contract API.
  • StressLog contract -- implement the API by enumerating the stress log chunks from the existing StressLogMemory processed data, sized by the StressLogChunk type; surfaced through both StressLog_1 and StressLog_2.
  • SOSDacImpl -- implement ISOSDacInterface17.GetStressLogMemoryRanges, projecting the contract ranges into SOSMemoryRegion entries; add a SOSMemoryEnum constructor that takes a precomputed region array.

Adds a new GetStressLogMemoryRanges method to ISOSDacInterface17 that
enumerates the memory ranges (chunks) backing the stress log, returned
as an ISOSMemoryEnum.

- sospriv.idl: declare the new interface method.
- IStressLog contract: add StressLogMemoryRange record and
  GetStressLogMemoryRanges contract API.
- StressLog contract implementation: enumerate stress log chunks using
  the existing StressLogMemory processed data, sized by the
  StressLogChunk type, and surface them through StressLog_1/StressLog_2.
- SOSDacImpl: implement ISOSDacInterface17.GetStressLogMemoryRanges,
  projecting the ranges into SOSMemoryRegion entries; add a
  SOSMemoryEnum constructor taking a precomputed region array.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a new SOS DAC entrypoint to enumerate the in-memory StressLog backing chunks as ISOSMemoryEnum, wiring it through the cDAC StressLog contract and exposing the COM signature via sospriv.idl.

Changes:

  • Extend ISOSDacInterface17 with GetStressLogMemoryRanges returning an ISOSMemoryEnum of SOSMemoryRegion ranges.
  • Add a StressLog contract API (StressLogMemoryRange + GetStressLogMemoryRanges) and implement it for both StressLog_1 and StressLog_2 via existing processed StressLog chunk traversal.
  • Add a SOSMemoryEnum constructor that accepts a precomputed SOSMemoryRegion[] to project contract ranges efficiently.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs Implements ISOSDacInterface17.GetStressLogMemoryRanges and adds a SOSMemoryEnum(SOSMemoryRegion[]) constructor to return regions.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ISOSDacInterface.cs Adds the managed COM interface declaration for GetStressLogMemoryRanges.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StressLog.cs Implements GetStressLogMemoryRanges by enumerating StressLog chunks and surfacing it from both contract versions.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IStressLog.cs Introduces StressLogMemoryRange and a new IStressLog.GetStressLogMemoryRanges API.
src/coreclr/inc/sospriv.idl Adds the new ISOSDacInterface17::GetStressLogMemoryRanges method signature.

@github-actions

Copy link
Copy Markdown
Contributor

Copilot Code Review

Holistic Assessment

Motivation: Justified — diagnostic tools need a way to enumerate stress log memory regions, not just check point containment. The existing IsPointerInStressLog validates a pointer is within the log but doesn't expose the backing chunks to readers.

Approach: Sound. The implementation reuses the existing StressLogMemory processed-data infrastructure, follows the established wiring pattern through StressLogTraversalStressLog_1/StressLog_2SOSDacImpl, and correctly appends the new method to the end of the ISOSDacInterface17 vtable.

Summary: ⚠️ Needs Human Review. The code is correct, consistent with established patterns, and well-scoped. The single concern is the missing dump test for the new ISOSDacInterface17.GetStressLogMemoryRanges method — the test file already covers the other three ISOSDacInterface17 methods with the same pattern and this one is conspicuously absent. A human reviewer should decide whether to require it before merge or accept it as a follow-up.


Detailed Findings

Detailed Findings

✅ Contract Implementation — Correct and consistent

The GetStressLogMemoryRanges implementation in StressLogTraversal mirrors IsPointerInStressLog exactly: both fetch StressLogMemory via ProcessedData.GetOrAdd<StressLogMemory>(stressLog.Logs) and iterate over Chunks. The new method simply yields StressLogMemoryRange(chunk, chunkSize) instead of performing a containment check. This is clean code reuse.

✅ SOSDacImpl wiring — Follows established patterns

  • S_FALSE return when !HasStressLog() matches sibling methods.
  • catch (System.Exception ex) { hr = ex.HResult; } is the standard error-handling pattern.
  • LINQ .Select().ToArray() is already used elsewhere in this file (using System.Linq at line 7).
  • The new SOSMemoryEnum(SOSMemoryRegion[]) constructor is a minimal addition that avoids re-converting already-projected data.

✅ IDL & interface ordering — Correct

The new GetStressLogMemoryRanges is appended as the last method on ISOSDacInterface17 in both sospriv.idl and the managed ISOSDacInterface.cs. This preserves vtable slot ordering as required.

⚠️ Test gap — No dump test for GetStressLogMemoryRanges (advisory, non-blocking)

StressLogDumpTests.cs has matching tests for:

  • ISOSDacInterface17_GetStressLogData
  • ISOSDacInterface17_GetStressLogThreadEnumerator
  • ISOSDacInterface17_GetStressLogMessageEnumerator

A test for GetStressLogMemoryRanges is missing. The expected test would follow the same pattern:

[ConditionalTheory]
[MemberData(nameof(TestConfigurations))]
public unsafe void ISOSDacInterface17_GetStressLogMemoryRanges(TestConfiguration config)
{
    InitializeDumpTest(config);
    ISOSDacInterface17 sosDac = (ISOSDacInterface17)new SOSDacImpl(Target, legacyObj: null);

    DacComNullableByRef<ISOSMemoryEnum> ppEnum = new(isNullRef: false);
    int hr = sosDac.GetStressLogMemoryRanges(ppEnum);
    Assert.Equal(System.HResults.S_OK, hr);

    ISOSMemoryEnum? memEnum = ppEnum.Interface;
    Assert.NotNull(memEnum);

    SOSMemoryRegion[] regions = new SOSMemoryRegion[64];
    uint fetched;
    hr = memEnum.Next(64, regions, &fetched);
    Assert.True(hr == System.HResults.S_OK || hr == System.HResults.S_FALSE);
    Assert.True(fetched > 0, "Expected at least one stress log memory range");
    Assert.NotEqual((ClrDataAddress)0, regions[0].Start);
    Assert.NotEqual((ClrDataAddress)0, regions[0].Size);
}

Since the dump test infrastructure requires a captured dump and existing tests already exercise the underlying data paths, this could be a follow-up — but should be tracked.

💡 Minor observation — ExtraData = default and Heap = 0

The SOSMemoryRegion projection explicitly sets ExtraData = default and Heap = 0. This is correct (stress log chunks aren't heap-specific and have no extra metadata), but if the intent is for callers to distinguish stress log regions from GC regions, consider whether a future version might want to annotate ExtraData. Not actionable now — just noting for design awareness.

Note

This review was generated by GitHub Copilot.

Generated by Code Review for issue #130038 · ● 26.2M ·

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

@max-charlamb max-charlamb enabled auto-merge (squash) June 30, 2026 17:28
@max-charlamb max-charlamb merged commit ea3a1f2 into main Jun 30, 2026
152 checks passed
@max-charlamb max-charlamb deleted the max-charlamb/extend-isosdacinterface17 branch June 30, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants