FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[release/9.0-staging] Fix crash during Async Break when APC and CET are enabled by thaystg · Pull Request #114932 · dotnet/runtime · GitHub

  1. dotnet
  2. runtime

Repository navigation

[release/9.0-staging] Fix crash during Async Break when APC and CET are enabled - #114932

Merged
thaystg merged 7 commits into
dotnet:release/9.0-stagingfrom
thaystg:dev/thays/backport_111408
May 19, 2025
ConversationCommits (7)ChecksFiles changed
Merged

[release/9.0-staging] Fix crash during Async Break when APC and CET are enabled#114932
thaystg merged 7 commits into
dotnet:release/9.0-stagingfrom
thaystg:dev/thays/backport_111408

Conversation

thaystg commented Apr 22, 2025 •
edited by tommcdon
Loading

Copy link
Copy Markdown
Member

Backport of #111408 to release/9.0-staging

/cc @thaystg

Customer Impact

When a customer tries to pause an app that is running under the debugger, it may crash the app. This was very easy to reproduce using the sample provided in the issue.
CET + APC enabled caused a lot of issues and this is one of them. I think we should fix it because this is probably affecting a lot of customers that didn't open an issue because it's not easy to share a repro sample.

Regression

  • Yes (Works well on .NET 8)
  • No

Testing

Manually tested.

Risk

Medium risk, we are changing how we pause the threads in an Async Pause asked by the customer while debugging.

Copilot AI review requested due to automatic review settings April 22, 2025 21:29
ghost added the area-VM-coreclr label Apr 22, 2025
Copilot AI reviewed Apr 22, 2025
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull Request Overview

This PR backports changes to support special user mode APC handling in thread suspension and debugging paths.

  • Introduces new thread state flags (TS_SSToExitApcCall and TS_SSToExitApcCallDone) and helper methods to manage APC-related thread suspension.
  • Adds an overload for HandleSuspensionForInterruptedThread and updates debugger and controller logic to handle single-stepping out of APC calls.

Reviewed Changes

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

File Description
src/coreclr/vm/threadsuspend.cpp Added APC handling in thread suspension, new method MarkForSuspensionAndWait, and overload for HandleSuspensionForInterruptedThread.
src/coreclr/vm/threads.h Updated thread state flags, friend declarations, and protected member methods.
src/coreclr/vm/dbginterface.h, debugger.h, debugger.cpp Introduced SingleStepToExitApcCall support for APC callbacks during debugging.
src/coreclr/debug/ee/controller.cpp Modified exception dispatch to manage APC exit using the new thread state flags.
Comments suppressed due to low confidence (1)

src/coreclr/vm/threadsuspend.cpp:4241

  • Ensure that the new FEATURE_SPECIAL_USER_MODE_APC code paths, including the APC exit handling, are adequately covered by regression tests to verify both normal and debugger-assisted scenarios.
#ifdef FEATURE_SPECIAL_USER_MODE_APC

Comment thread src/coreclr/debug/ee/controller.cpp

dotnet-policy-service Bot commented Apr 22, 2025

Copy link
Copy Markdown
Contributor

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

This was referenced Apr 23, 2025
Open
Closed
build-analysis Bot mentioned this pull request Apr 24, 2025
Open
3 tasks
thaystg changed the title backport 111408 [release/9.0-staging] Apr 28, 2025
thaystg changed the title [release/9.0-staging] [release/9.0-staging] Fix crash during Async Break when APC and CET are enabled Apr 28, 2025
jeffschwMSFT approved these changes Apr 28, 2025
View reviewed changes

jeffschwMSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm. we will take for consideration in 9.0.x

jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Apr 28, 2025
jeffschwMSFT added this to the 9.0.x milestone Apr 28, 2025
rbhanda modified the milestones: 9.0.x, 9.0.6 Apr 29, 2025
rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2025
vseanreesermsft added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels May 2, 2025

jozkee commented May 7, 2025

Copy link
Copy Markdown
Member

@thaystg friendly reminder that code complete is on Monday May 12th (2:00 PM Pacific) for the June Release. If you'd like to get this change included in that release, please merge this PR before the deadline.

thaystg commented May 13, 2025

Copy link
Copy Markdown
Member Author

/ba know failures.

thaystg commented May 13, 2025

Copy link
Copy Markdown
Member Author

/ba-g unrelated failures

thaystg commented May 14, 2025

Copy link
Copy Markdown
Member Author

/ba known failures.

thaystg merged commit 753e467 into dotnet:release/9.0-staging May 19, 2025
thaystg mentioned this pull request May 27, 2025
Merged
github-actions Bot locked and limited conversation to collaborators Jun 19, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
jeffschwMSFT jeffschwMSFT approved these changes
tommcdon tommcdon approved these changes

Assignees

thaystg

Labels

area-VM-coreclr Servicing-approved Approved for servicing release

Projects

None yet

Milestone

9.0.6

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL