Skip to content

Add acquire barrier to populate bit reading #117523

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2025
Merged

Add acquire barrier to populate bit reading #117523

merged 1 commit into from
Jul 11, 2025

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Jul 10, 2025 •
edited
Loading

The setting of the NDirectMethodDesc::kNDirectPopulated bit is done via an interlocked operation. This bit is used to indicate if the NDirectMethodDesc has been populated with the needed information to load the P/Invoke. The reading of this bit however was missing an acquire barrier and thus setting of fields between the check and set weren't being observed correctly.

Fixes #112565

This should be considered to backport to .NET 9.0.

@AaronRobinsonMSFT
Add acquire barrier to populate bit reading
0dc96a1 
The setting of the NDirectMethodDesc::kNDirectPopulated bit
is done via an interlocked operation. This bit is used to indicate
if the NDirectMethodDesc has been populated with the needed
information to load the P/Invoke. The reading of this bit however
was missing an acquire barrier and thus setting of fields between
the check and set weren't being observed correctly.
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 10.0.0 milestone Jul 10, 2025
Copilot
Copilot AI reviewed Jul 10, 2025
View reviewed changes
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

Adds an acquire barrier to the IsPopulated check so that any writes performed before setting the kNDirectPopulated bit are correctly observed by readers.

  • Updated IsPopulated to use VolatileLoad when reading m_wFlags

@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Jul 10, 2025
Closed
jkoritzinsky
jkoritzinsky approved these changes Jul 11, 2025
View reviewed changes
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

This must have been an absolute pain to debug and find. LGTM!

This was referenced Jul 11, 2025
Open
Closed
Closed
Closed
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 1f862c8 into dotnet:main Jul 11, 2025
99 of 101 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_112565 branch July 11, 2025 18:24
@elinor-fung
Copy link
Member

Definitely not for this change, but should we have something similar for kDefaultDllImportSearchPathsIsCached (indication of populating another field on NDirectMethodDesc)?

@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

Copilot code review Copilot Copilot left review comments

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@davidwrighton davidwrighton davidwrighton approved these changes

@janvorli janvorli Awaiting requested review from janvorli

Assignees

@AaronRobinsonMSFT AaronRobinsonMSFT

Labels
area-Interop-coreclr
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

Segfault in minipal_resolve_dllimport
4 participants
@AaronRobinsonMSFT @elinor-fung @jkoritzinsky @davidwrighton