Allow the global alloc one TLS slot with a destructor

[orlp]

This is an improvement over #116402, which fixed an unsoundness by simply disallowing the global allocator from creating any thread-local variables with destructors. Instead of doing that, with this PR the global allocator is allowed to create exactly one thread-local variable with a destructor.

This is actually already a huge improvement over the status quo, as a single thread-local variable with a destructor is sufficient to store arbitrary amounts of thread-local data while allowing cleanup on thread exit.

[rustbot]

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[orlp]

I've added a test which uses a GlobalAlloc which not only has a thread-local variable with a destructor in its alloc, but also in dealloc. I had to change the drop order in destructors::run slightly for this.

Writing such an allocator only works if we guarantee not to touch the GlobalAlloc anymore in a thread after calling destructors::run. From a quick glance, I believe we mostly already do this on all platforms except two things:

• rt::thread_cleanup calls thread::drop_current which drops an Arc holding the Thread handle:
  

  rust/library/std/src/thread/current.rs

  Line 271 in 175e043

  drop(Thread::from_raw(current));

  The test currently does not catch this as the main thread holds the Thread handle alive in the .join().
• On μITRON we drop a Box after running the thread-local destructors:

  rust/library/std/src/sys/pal/itron/thread.rs

  Lines 114 to 132 in 175e043

  	unsafe { crate::sys::thread_local::destructors::run() };
  	let old_lifecycle = inner
  	.lifecycle
  	.swap(LIFECYCLE_EXITED_OR_FINISHED_OR_JOIN_FINALIZE, Ordering::AcqRel);
  	match old_lifecycle {
  	LIFECYCLE_DETACHED => {
  	// [DETACHED → EXITED]
  	// No one will ever join, so we'll ask the collector task to
  	// delete the task.
  	// In this case, `*p_inner`'s ownership has been moved to
  	// us, and we are responsible for dropping it. The acquire
  	// ordering ensures that the swap operation that wrote
  	// `LIFECYCLE_DETACHED` happens-before `Box::from_raw(
  	// p_inner)`.
  	// Safety: See above.
  	let _ = unsafe { Box::from_raw(p_inner) };

Can we change/mitigate these things so that GlobalAlloc.dealloc may also access the thread-local with a destructor?

Just to clarify, the current behavior of this PR is safe, you just get a LocalKey panic-during-panic into an abort if you were to write a GlobalAlloc which accesses a thread-local with a destructor in its dealloc.

[ChrisDenton]

cc @joboet, since you've done a lot in this area did you want to take this? I'd be interested in your input even if not.

[joboet]

Sure, I can take this. Though I'm not convinced that the current approach is a good idea – just one TLS variable with a destructor seems like an arbitrary limit and will hinder the compositionality of allocators. And this also doesn't work on platforms like GNU/Windows, where the TLS storage is always allocated. I think there are two better solutions:

• Make the destructor list a single-linked list and store the nodes along with the data in the TLS static (this wouldn't fix the key-based TLS issue though)
• Just switch to the System allocator for all std-internal allocations

[orlp]

just one TLS variable with a destructor seems like an arbitrary limit

This PR doesn't prevent better solutions from being adopted later - it's already a huge improvement over the status quo (where an allocator is unable to register any kind of per-thread cleanup natively in Rust).

and will hinder the compositionality of allocators

This solution (and limitation) is only for the global allocator, which is already not composable. Only a single one may be set.

And this also doesn't work on platforms like GNU/Windows, where the TLS storage is always allocated

I don't quite follow, why would this not work on GNU/Windows?

[orlp]

I don't quite follow, why would this not work on GNU/Windows?

It seems that on platforms which go through the library/std/src/sys/thread_local/os.rs implementation always allocate.

It's probably best then indeed if the entire thread-local implementation is allocation-free where possible and uses System to allocate where needed. I think this also includes the std::thread::Thread which currently uses an Arc internally.

@joboet Is there anything else that would need conversion as well?

[orlp]

Closed in favor of #144465 which is more general.