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

Run TLS destructors for main thread on UNIX platforms #133819

Closed
wants to merge 4 commits into from

Conversation

surban
Copy link
Contributor

@surban surban commented Dec 3, 2024

This calls TLS destructors on UNIX platforms other than Linux when the main thread exits. This is done by registering a process-wide atexit handler.

r? joboet

This calls TLS destructors on UNIX platforms other than Linux
when the main thread exits. This is done by registering a
process-wide atexit handler.
@rustbot rustbot added 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 Dec 3, 2024
@rust-log-analyzer

This comment has been minimized.

@joboet
Copy link
Member

joboet commented Dec 4, 2024

Hi, T-libs! Are you okay with this behaviour change? This makes platforms like musl behave the same way as glibc; they will now run TLS destructors both when main returns and when exit is called instead of only running them when a thread exits.

@rustbot label +I-libs-nominated

@rustbot rustbot added the I-libs-nominated Nominated for discussion during a libs team meeting. label Dec 4, 2024
@Amanieu
Copy link
Member

Amanieu commented Dec 4, 2024

This is currently documented as platform-specific behavior here.

The main benefit of this PR would be to make the behavior more consistent across platforms so that we can remove the warning in the documentation and make it a guarantee. However for that to happen we need to be able to ensure that this works on all platforms. Could you have a look at other (non-UNIX) platforms to see what the current behavior is and whether they need a similar workaround?

@Amanieu Amanieu removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Dec 4, 2024
@surban
Copy link
Contributor Author

surban commented Dec 5, 2024

However for that to happen we need to be able to ensure that this works on all platforms. Could you have a look at other (non-UNIX) platforms to see what the current behavior is and whether they need a similar workaround?

I can have a look, however I certainly don't have access to all platforms supported by Rust. I can test on Windows, Linux, FreeBSD and WASI preview 1.

Is there a way to run test programs on all Tier 1 and Tier 2 targets via CI, so that I could run automatic tests?

@surban surban requested a review from m-ou-se December 5, 2024 17:05
@surban
Copy link
Contributor Author

surban commented Dec 5, 2024

@Amanieu

So this what I was able to gather by looking at the code and doing some testing where possible.

  • Linux (and most likely Android, Fuchsia, Redox, Hurd, NetBSD, Dragonfly) (tier 1)

    • recent glibc with symbol __cxa_thread_atexit_impl
      • ✅ TLS destructors did run for thread exits and process exit
    • old glibc or musl or other libc without symbol __cxa_thread_atexit_impl
      • 👍 TLS destructors did not run for process exit, but do run with this PR
    • tested with glibc and musl libc on Ubuntu 22.04
  • Windows (tier 1)

    • ✅ TLS destructors did run for thread exits and process exit
    • tested using both MSVC and GNU toolchains on Windows 11 x64
  • Apple platforms (tier 1)

    • ✅ TLS destructors did run for thread exits and process exit
    • tested using macOS 14.4
  • other UNIX (tier 2)

    • 👍 TLS destructors did not run for process exit, but do run with this PR
    • tested using FreeBSD 14.2
  • targets without threads (wasm32-wasip1 without atomics feature and UEFI) (tier 2)

    • ❌ TLS destructors are not tracked
    • currently there is no bookkeeping of TLS values that need to be dropped
    • would require adding destructors::register calls to the statik module
  • wasm32-unknown-unknown with atomics feature (tier 2)

    • 🚫 has no concept of process and thread exit
  • Intel/Fortanix SGX (tier 2)

    • 🚫 has no concept of process exit
  • Risc0 zkvm (tier 3)

    • 🚫 has no concept of process exit
  • Xous (tier 3)

    • 🚫 has no concept of process exit
  • Hermit (tier 3)

    • ✅ calls thread_local::destructors::run() explicitly after main() and thread exit
    • should work, but not tested
  • Solid (tier 3)

    • ❓ depends on whether the system calls functions registered through SOLID_TLS_AddDestructor at process exit
    • I did not find any API documentation.

Summary

Targets that do not have support for threads (i.e. wasm32-wasip1 without atomics and UEFI) require the statik module to be extended to track TLS values for destruction at process exit. For wasm32-wasip1 this can be invoked from an atexit handler, while UEFI might be more challenging.

The remaining non-UNIX targets should either work (Hermit, Solid) or do not have the concept of process exit (std::process:exit is unimplemented).

@joboet
Copy link
Member

joboet commented Dec 6, 2024

That's a great summary, thank you! One other thing to note though is that not every UNIX platform has native thread local storage, and that the solution proposed in this PR will not work for them since std does not manage the destructors itself; this is left to the pthread TLS key implementation.

@surban
Copy link
Contributor Author

surban commented Dec 6, 2024

One other thing to note though is that not every UNIX platform has native thread local storage, and that the solution proposed in this PR will not work for them since std does not manage the destructors itself; this is left to the pthread TLS key implementation.

Could you name such a platform so that I can test? Preferably something that can be run in a VM.

@joboet
Copy link
Member

joboet commented Dec 6, 2024

Oh wow, they've become quite rare since last time I checked. But Solaris (e.g. x86_64_pc_solaris) is one for instance.

@surban
Copy link
Contributor Author

surban commented Dec 9, 2024

Closing in favor of #134085

@surban surban closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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