Skip to content

[cDAC] Restore Frame-walk fallback in Thread.GetContext#128518

Open
Copilot wants to merge 3 commits into
mainfrom
copilot/reimplement-e-notimpl-path
Open

[cDAC] Restore Frame-walk fallback in Thread.GetContext#128518
Copilot wants to merge 3 commits into
mainfrom
copilot/reimplement-e-notimpl-path

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 23, 2026

The E_NOTIMPL Frame-walk fallback removed in 5391e9e is still needed for data targets that don't implement GetThreadContext. The native side is being restored in a separate PR; this change brings the equivalent path to cDAC.

Changes

  • Thread_1.GetContext: When TryGetThreadContext returns false, walk the thread's Frame chain (modeled after IStackWalk.GetFrames) instead of throwing InvalidOperationException.

    • For an InterpreterFrame, fill the context from the top InterpMethodContextFrame via the existing UpdateContextFromCurrentFrame (which calls SetContextToInterpMethodContextFrame).
    • For other frames, update a cleared context from the frame and accept the first one with both SP and PC non-zero (e.g. RedirectedThreadFrame, InlinedCallFrame, DynamicHelperFrame), setting RawContextFlags = FullContextFlags to mark SP/PC/FP as valid. This mirrors the native DT_CONTEXT_CONTROL [| DT_CONTEXT_INTEGER] intent without per-architecture branching.
    • If no frame yields a usable context, return a zeroed context (thread is not running managed code).
  • docs/design/datacontracts/Thread.md: Updated the GetContext pseudocode to document the new fallback.

…dContext is unavailable

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 23, 2026 05:34
Copilot AI review requested due to automatic review settings May 23, 2026 05:34
@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.

@rcj1 rcj1 marked this pull request as ready for review May 23, 2026 06:14
Copilot AI review requested due to automatic review settings May 23, 2026 06:14
Copy link
Copy Markdown
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 changes the cDAC IThread.GetContext implementation to fall back to deriving a thread context from the thread’s explicit Frame chain when the data target can’t provide an OS thread context, and updates the Thread data contract design doc accordingly.

Changes:

  • Update Thread_1.GetContext to return the OS context when TryGetThreadContext succeeds, otherwise derive a context by walking the thread’s Frame chain.
  • Add a helper (GetContextFromFrames) that selects an InterpreterFrame context (via InterpMethodContextFrame) or the first frame producing non-zero SP/IP.
  • Update docs/design/datacontracts/Thread.md pseudocode to document the fallback behavior.

Note: I did not run build/tests as part of this review.

Show a summary per file
File Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Thread_1.cs Adds frame-walk fallback path for GetContext when TryGetThreadContext doesn’t provide a context.
docs/design/datacontracts/Thread.md Documents the updated GetContext behavior and fallback selection rules.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 3

Comment thread docs/design/datacontracts/Thread.md Outdated
Copilot AI review requested due to automatic review settings May 23, 2026 06:54
Copy link
Copy Markdown
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.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

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