Skip to content

[cDAC] IXCLRDataStackWalk::GetContext returns proper HResult #115157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2025

Conversation

max-charlamb
Copy link
Contributor

@max-charlamb max-charlamb commented Apr 29, 2025

IXCLRDataStackWalk::GetContext should return HResult.S_FALSE when the current Frame is invalid. Previously, this method would return the initial state with HResult.S_OK, triggering a Debug Assert when comparing to the DAC implementation.

@Copilot Copilot AI review requested due to automatic review settings April 29, 2025 18:44
@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 29, 2025
Copy link
Contributor

@Copilot 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 updates the behavior of IXCLRDataStackWalk::GetContext so that it returns HResult.S_FALSE when the current frame is invalid. Key changes include:

  • Introducing a new field (_currentFrameIsValid) to track frame validity.
  • Adjusting the GetContext method to conditionally return S_FALSE when no valid frame is present.
  • Updating the StackWalk contract to yield no result if the frame is invalid.

Reviewed Changes

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

File Description
src/native/managed/cdac/mscordaccore_universal/Legacy/ClrDataStackWalk.cs Added _currentFrameIsValid and modified GetContext and Next methods to determine and use frame validity.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/StackWalk_1.cs Switched to a FrameIterator and added an early exit (yield break) when the frame is invalid.

@max-charlamb max-charlamb requested review from mikem8361, davidwrighton and a team April 29, 2025 18:44
@max-charlamb max-charlamb added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 29, 2025
Copy link
Contributor

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

Copy link
Contributor

@davmason davmason left a comment

Choose a reason for hiding this comment

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

LGTM

@max-charlamb
Copy link
Contributor Author

/ba-g Test failures are unrelated to changes in this PR. Bypassing due to known issue with Build Analysis hanging.

@max-charlamb max-charlamb merged commit 30cf770 into dotnet:main May 1, 2025
152 of 155 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants