Skip to content

[cDAC] Move AMD64 Unwinder to managed code #116488

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 7 commits into from
Jun 17, 2025

Conversation

max-charlamb
Copy link
Contributor

@max-charlamb max-charlamb commented Jun 10, 2025

Ports the AMD64 unwinder to managed code. The cDAC uses this managed version instead of deferring to the runtime native copy.

I'm leaving the native building infra in place until the ARM64 unwinder is ported and all of the native linking can be removed.

@max-charlamb
Copy link
Contributor Author

/azp run runtime-diagnostics

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

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

@max-charlamb
Copy link
Contributor Author

/azp run runtime-diagnostics

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@max-charlamb max-charlamb changed the title [cDAC] CI Test Run [cDAC] Move AMD64 Unwinder to managed code Jun 13, 2025
@max-charlamb max-charlamb marked this pull request as ready for review June 13, 2025 18:43
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 transitions the AMD64 unwinding logic from a static native helper call into fully managed code using the new AMD64Unwinder class.

  • Added a using for the AMD64Unwinder namespace.
  • Replaced Unwinder.AMD64Unwind(ref this, target) with instantiation of AMD64Unwinder and its Unwind method.
Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/AMD64Context.cs:54

  • This new managed unwinder path should be covered by unit or integration tests to ensure the behavior matches the previous native implementation under various stack scenarios.
AMD64Unwinder unwinder = new(target);

@@ -50,7 +51,8 @@ public TargetPointer FramePointer

public void Unwind(Target target)
{
Unwinder.AMD64Unwind(ref this, target);
AMD64Unwinder unwinder = new(target);
Copy link
Preview

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

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

Instantiating AMD64Unwinder on each call may introduce heap allocations in hot paths. If performance is critical, consider making AMD64Unwinder a struct or caching a single instance to minimize allocations.

Copilot uses AI. Check for mistakes.

@max-charlamb
Copy link
Contributor Author

/azp run runtime-diagnostics

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@max-charlamb
Copy link
Contributor Author

/ba-g test failures unrelated to cDAC change. Both runtime-diagnostics and CLR_Tools_Tests are passing.

@max-charlamb max-charlamb merged commit 0d628da into dotnet:main Jun 17, 2025
142 of 153 checks passed
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