Skip to content

[cDAC] Dumptest Stackwalker Improvements#124912

Draft
max-charlamb wants to merge 4 commits intodotnet:mainfrom
max-charlamb:cdac-dumptest-improvements
Draft

[cDAC] Dumptest Stackwalker Improvements#124912
max-charlamb wants to merge 4 commits intodotnet:mainfrom
max-charlamb:cdac-dumptest-improvements

Conversation

@max-charlamb
Copy link
Member

Adds tools to better add stack tests

@dotnet-policy-service
Copy link
Contributor

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new DumpTestStackWalker helper class to simplify writing stack frame validation tests in the cDAC dump test suite, and adds three new tests demonstrating its usage.

Changes:

  • Adds DumpTestStackWalker helper class with fluent API for asserting stack frame sequences with support for adjacency checking, predicate matching, and flexible inner-to-outer/outer-to-inner traversal
  • Adds new tests validating expected frame sequences in StackWalk, PInvokeStub, and VarargPInvoke dump tests
  • Updates RunDumpTests.ps1 to compute and pass skip versions to the test build, improving test filtering when running subset of versions

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
DumpTestStackWalker.cs New helper class providing fluent API for stack frame assertions with support for adjacency, predicates, and directional traversal
StackWalkDumpTests.cs Adds test validating the expected frame sequence (MethodC → MethodB → MethodA → Main) using the new helper
VarargPInvokeDumpTests.cs Adds test validating vararg P/Invoke stack frames including ILStub frame detection using predicate matching
PInvokeStubDumpTests.cs Adds test validating P/Invoke stack frames (memcpy → CrashInILStubPInvoke → Main)
RunDumpTests.ps1 Enhances script to compute and pass skipped versions to test build via MSBuild property

Comment on lines +270 to +284
public Expectation(string name, bool Adjacent, Action<ResolvedFrame>? assert)
{
_name = name;
_predicate = null;
_assert = assert;
this.Adjacent = Adjacent;
Description = name;
}

public Expectation(Func<ResolvedFrame, bool> predicate, string description, bool Adjacent, Action<ResolvedFrame>? assert)
{
_name = null;
_predicate = predicate;
_assert = assert;
this.Adjacent = Adjacent;
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The parameter name Adjacent should be lowercase adjacent to follow C# naming conventions. Parameter names should use camelCase, not PascalCase.

Suggested change
public Expectation(string name, bool Adjacent, Action<ResolvedFrame>? assert)
{
_name = name;
_predicate = null;
_assert = assert;
this.Adjacent = Adjacent;
Description = name;
}
public Expectation(Func<ResolvedFrame, bool> predicate, string description, bool Adjacent, Action<ResolvedFrame>? assert)
{
_name = null;
_predicate = predicate;
_assert = assert;
this.Adjacent = Adjacent;
public Expectation(string name, bool adjacent, Action<ResolvedFrame>? assert)
{
_name = name;
_predicate = null;
_assert = assert;
this.Adjacent = adjacent;
Description = name;
}
public Expectation(Func<ResolvedFrame, bool> predicate, string description, bool adjacent, Action<ResolvedFrame>? assert)
{
_name = null;
_predicate = predicate;
_assert = assert;
this.Adjacent = adjacent;

Copilot uses AI. Check for mistakes.
Comment on lines +270 to +284
public Expectation(string name, bool Adjacent, Action<ResolvedFrame>? assert)
{
_name = name;
_predicate = null;
_assert = assert;
this.Adjacent = Adjacent;
Description = name;
}

public Expectation(Func<ResolvedFrame, bool> predicate, string description, bool Adjacent, Action<ResolvedFrame>? assert)
{
_name = null;
_predicate = predicate;
_assert = assert;
this.Adjacent = Adjacent;
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The parameter name Adjacent should be lowercase adjacent to follow C# naming conventions. Parameter names should use camelCase, not PascalCase.

Suggested change
public Expectation(string name, bool Adjacent, Action<ResolvedFrame>? assert)
{
_name = name;
_predicate = null;
_assert = assert;
this.Adjacent = Adjacent;
Description = name;
}
public Expectation(Func<ResolvedFrame, bool> predicate, string description, bool Adjacent, Action<ResolvedFrame>? assert)
{
_name = null;
_predicate = predicate;
_assert = assert;
this.Adjacent = Adjacent;
public Expectation(string name, bool adjacent, Action<ResolvedFrame>? assert)
{
_name = name;
_predicate = null;
_assert = assert;
this.Adjacent = adjacent;
Description = name;
}
public Expectation(Func<ResolvedFrame, bool> predicate, string description, bool adjacent, Action<ResolvedFrame>? assert)
{
_name = null;
_predicate = predicate;
_assert = assert;
this.Adjacent = adjacent;

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings March 10, 2026 15:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

return;
}

Assert.Fail("Expected to find an ILStub MethodDesc on the crashing thread stack");
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

Grammar: "a ILStub" should be "an ILStub" in this failure message.

Copilot uses AI. Check for mistakes.
Comment on lines +131 to +142
writer ??= Console.WriteLine;
writer($"Call stack ({_frames.Count} frames, {(_outerToInner ? "outer->inner" : "inner->outer")}):");

for (int i = 0; i < _frames.Count; i++)
{
ResolvedFrame f = _frames[i];
string name = f.Name ?? "<null>";
string md = f.MethodDescPtr != TargetPointer.Null
? $"0x{f.MethodDescPtr:X}"
: "null";
writer($" [{i}] {name} (MethodDesc: {md})");
}
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

Print reports the direction as outer->inner when _outerToInner is set, but it always prints _frames in the original inner-to-outer order. This can be misleading when debugging; consider printing the frames in the selected direction (or adjust the label to reflect the actual order printed).

Copilot uses AI. Check for mistakes.
/// </summary>
protected void InitializeDumpTest(TestConfiguration config, [CallerMemberName] string callerName = "")
=> InitializeDumpTest(config, DebuggeeName, DumpType, callerName);

Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

This new InitializeDumpTest overload is part of the base test API but lacks the XML documentation that the original overload has. Adding matching docs (including parameter meaning for debuggeeName/dumpType) would help keep the base class self-describing.

Suggested change
/// <summary>
/// Loads the dump for the given <paramref name="config"/> using the specified
/// <paramref name="debuggeeName"/> and <paramref name="dumpType"/>, and evaluates
/// skip attributes on the calling test method.
/// </summary>
/// <param name="config">The test configuration that selects the runtime version whose dump should be loaded.</param>
/// <param name="debuggeeName">
/// The name of the debuggee that produced the dump (for example, the test program name such as "BasicThreads").
/// This is used to locate the dump file on disk.
/// </param>
/// <param name="dumpType">
/// The dump type to load (for example, "heap" or "full"). This determines the subdirectory under the dump root
/// where the dump file is located.
/// </param>
/// <param name="callerName">
/// The name of the calling test method. This is automatically supplied by the compiler and is used when
/// evaluating skip attributes.
/// </param>

Copilot uses AI. Check for mistakes.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

using System; appears unused in this file. If warnings are treated as errors (common in this repo), this can break the build; please remove the unused using directive.

Suggested change
using System;

Copilot uses AI. Check for mistakes.
}
}

Assert.True(foundILStub, "Expected to find a ILStub MethodDesc on the crashing thread stack");
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

Grammar: "a ILStub" should be "an ILStub" in the assertion message.

Suggested change
Assert.True(foundILStub, "Expected to find a ILStub MethodDesc on the crashing thread stack");
Assert.True(foundILStub, "Expected to find an ILStub MethodDesc on the crashing thread stack");

Copilot uses AI. Check for mistakes.
return;
}

Assert.Fail("Expected to find an ILStub MethodDesc on the crashing thread stack");
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

Grammar: "a ILStub" should be "an ILStub" in this failure message.

Copilot uses AI. Check for mistakes.
- Add InitializeDumpTest(config, debuggeeName, dumpType) overload to
  DumpTestBase so tests can specify different debuggees per method
- Move PInvokeStub and VarargPInvoke tests into StackWalkDumpTests.cs
- Delete PInvokeStubDumpTests.cs and VarargPInvokeDumpTests.cs

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@max-charlamb max-charlamb force-pushed the cdac-dumptest-improvements branch from b6ddbd9 to 070f184 Compare March 10, 2026 16:14
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.

2 participants