Skip to content

Move Windows keyless TLS dtor into TLS callback module #94820

New issue

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

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

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

ChrisDenton
Copy link
Member

This may help ensure that the thread local doesn't end up in a separate module to the code that accesses it.

This may help ensure that the thread local doesn't end up in a separate module to the code that accesses it.
@rust-highfive
Copy link
Contributor

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2022
@ChrisDenton ChrisDenton marked this pull request as ready for review March 11, 2022 18:10
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2022
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 23, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2022
@Mark-Simulacrum
Copy link
Member

This may help ensure that the thread local doesn't end up in a separate module to the code that accesses it.

I assume module here means e.g., CGU or similar? If this is important to ensure, it seems like we ought to have at least a comment on the code, and ideally some kind of test -- otherwise it seems easy for this PR to get lost in a refactoring.

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Jul 31, 2022

Module means "dll" or "exe" here. IIRC I was seeing issues when running stage 1 tests where sometimes TLS access was being inlined across normal boundaries. Though this didn't appear to affect stage 2 builds as far as I could tell. I guess this is ultimately a compiler bug because it shouldn't be doing that ever. The thread_local! macro works around the problem by making get() be #[inline(never)]. That might work here too.

Though having just rerun ui tests now I'm not seeing the failure. Maybe things have changed.

@Mark-Simulacrum
Copy link
Member

I think inline(never) makes me less worried about accidentally reverting it, at least -- it seems like the better change here. I agree that the situation you're describing shouldn't happen though...

@ChrisDenton
Copy link
Member Author

Closing in favour of #100007

@ChrisDenton ChrisDenton closed this Aug 1, 2022
@ChrisDenton ChrisDenton deleted the win-tls-fix branch August 1, 2022 02:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants