Skip to content
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

Moved a static field initialization from Thread to ProcessorIdCache #114227

Merged
merged 2 commits into from
Apr 4, 2025

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Apr 3, 2025

Fixes: #113949

Presence of .cctor in Thread can cause circular dependency if Lock needs to block while Thread .cctor has not run yet.

  1. Lock needs to wait on a WaitHandle
  2. WaitHandle needs Thread.CurrentThread
  3. if Thread's .cctor has not run yet, it needs to run.
    (it is unusual for this to be the first use of Thread, but the activation pattern in Stack overflow from static constructors taking locks on Native AOT #113949 made it possible)
  4. .cctor needs to take a Lock, so we go to #1

@Copilot Copilot bot review requested due to automatic review settings April 3, 2025 17:47
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 moves the static field initialization for processor speed checking from Thread to ProcessorIdCache to resolve a circular dependency issue during static initialization.

  • Moved the speed check static field from Thread.cs to ProcessorIdCache.cs.
  • Updated GetCurrentProcessorId methods to use the new initialization logic directly from ProcessorIdCache.

Reviewed Changes

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

File Description
src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs Removed static field initialization and modified GetCurrentProcessorId to call ProcessorIdCache.
src/libraries/System.Private.CoreLib/src/System/Threading/ProcessorIdCache.cs Added static field initialization and embedded fast-path logic within GetCurrentProcessorId.

Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@jkotas
Copy link
Member

jkotas commented Apr 3, 2025

I assume that this is backport candidate.

@VSadov
Copy link
Member Author

VSadov commented Apr 3, 2025

I assume that this is backport candidate.

I think so. The fix is low-risk and we see actual WER buckets due to this.

@jkotas
Copy link
Member

jkotas commented Apr 4, 2025

/backport to release/9.0-staging

Copy link
Contributor

github-actions bot commented Apr 4, 2025

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/14270864630

@VSadov
Copy link
Member Author

VSadov commented Apr 4, 2025

Thanks!! I was just waiting for PR to get green as that leaves fewer doubts, but it looks like the failures are not specific to the PR.

@VSadov VSadov deleted the procIdMove branch April 4, 2025 17:36
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.

Stack overflow from static constructors taking locks on Native AOT
2 participants