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.

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

Unify TLS destructor list implementations #116850

Closed
wants to merge 4 commits into from
Closed

Unify TLS destructor list implementations #116850

wants to merge 4 commits into from

Conversation

joboet
Copy link
Member

@joboet joboet commented Oct 17, 2023 •
edited
Loading

Part of #110897.

Currently, nearly every platform implements its own TLS destructor list. This adds unnecessary code and means that issues like #116390 are harder to find and fix. Therefore, this PR unifies all of these implementations into a shared one inside sys::common::fast_local, leaving behind only the platform calls needed to ensure the list is emptied.

This changes behaviour on some Linux-like platforms, because we now keep our own list. This should however not impact performance too much, as the platform calls would also have needed to allocate.

Because it is only used on UNIX and is now much easier, I've removed the fallback implementation in sys_common. Because StaticKey may now be unused, this resulted in errors for Windows, which is why I have refactored that code to move the at-exit hack to the new thread_local_guard module. If it causes too much review pain, I can split this part out.

Tested on aarch64 macOS, checked on all other platforms. Best reviewed per-commit.

@rustbot label +T-libs +A-thread-locals

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added O-hermit Operating System: Hermit O-itron Operating System: ITRON O-solid Operating System: SOLID O-unix Operating system: Unix-like O-windows Operating system: Windows 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. labels Oct 17, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joboet joboet mentioned this pull request Oct 18, 2023
Closed
@rustbot
Copy link
Collaborator

rustbot commented Oct 18, 2023

The Miri subtree was changed

cc @rust-lang/miri

@bors
Copy link
Contributor

bors commented Oct 19, 2023

☔ The latest upstream changes (presumably #116402) made this pull request unmergeable. Please resolve the merge conflicts.

@joboet joboet mentioned this pull request Oct 22, 2023
Closed
@bors
Copy link
Contributor

bors commented Nov 21, 2023

☔ The latest upstream changes (presumably #118126) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 9, 2023

☔ The latest upstream changes (presumably #117873) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jan 13, 2024

☔ The latest upstream changes (presumably #117285) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 16, 2024

☔ The latest upstream changes (presumably #120486) made this pull request unmergeable. Please resolve the merge conflicts.

@joboet
std: begin unifying TLS destructor lists
ca68d4d
@joboet
std: move UNIX to new destructor list implementation
13cc6af
@joboet
std: refactor Windows TLS destructor support
2e425cf 
Now that the fallback code has been removed, there are no users of `StaticKey` left for targets with native TLS. Therefore, move the at-exit hack to the thread-local guard module, where it can be shared by both implementations, and cfg-out the key-based TLS when it's not needed.
@joboet
std: complete unifying the TLS destructor list implementations
5e1727c
@bors
Copy link
Contributor

bors commented Feb 25, 2024

☔ The latest upstream changes (presumably #121569) made this pull request unmergeable. Please resolve the merge conflicts.

@joboet
Copy link
Member Author

joboet commented Apr 30, 2024

This makes the dylib unloading problem described here even worse. I'll get some other things done before coming back to this.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2024
@joboet joboet marked this pull request as draft April 30, 2024 12:56
@joboet
Copy link
Member Author

joboet commented Jun 15, 2024

Closing in favour of #126523.

@joboet joboet closed this Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@m-ou-se m-ou-se

Labels
O-hermit Operating System: Hermit O-itron Operating System: ITRON O-solid Operating System: SOLID O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joboet @rustbot @rust-log-analyzer @bors @RalfJung @m-ou-se