I think this merits some libs-api discussion (unless that's already happened? I didn't quickly find it). Landing this IMO implies at least an implicit guarantee (even if not necessarily stable without actual docs) that we don't introduce global allocator usage in thread local code. I think we should have some discussion and either commit to that or not. And we should discuss where we draw the line (e.g., is other runtime-like code in std supposed to do this? For example, environment variables or other bits that need allocation early, potentially even before main starts -- is that OK to call into the allocator for?)

OTOH, if we can make a weaker guarantee here while still serving the purpose, that may also be good to look at? For example, can we guarantee that a pattern like dhat's for ignoring re-entrant alloc/dealloc calls is safe? i.e., that we don't need allocations for non-Drop thread locals? Or if we can only do that for a limited set, perhaps std could define a public thread local for allocators to use?

@Mark-Simulacrum

I think this merits some libs-api discussion (unless that's already happened? I didn't quickly find it). Landing this IMO implies at least an implicit guarantee (even if not necessarily stable without actual docs) that we don't introduce global allocator usage in thread local code.

Yes, this should probably be properly discussed.

And we should discuss where we draw the line (e.g., is other runtime-like code in std supposed to do this? For example, environment variables

You make a good point here about environment variables. It is very common to allow configuration and debugging of allocators through them. I'd like to at least guarantee that std::env::var_os does not result in allocation through the global allocator, I will look into that to see how feasible that is.

or other bits that need allocation early, potentially even before main starts -- is that OK to call into the allocator for?)

I think it's good to brainstorm a bit what would be needed for a global allocator, although I don't think an allocator needs all that much from std. Environment variables, thread locals, getting a thread handle comes to mind, and I guess spawning a thread is also rather important for creating background cleanup. I will also investigate that.

OTOH, if we can make a weaker guarantee here while still serving the purpose, that may also be good to look at?

I don't think there's much difference in constraint for the stdlib between guaranteeing this for all thread_local instances as opposed to only non-Drop thread locals, especially on platforms where every single thread-local already does an allocation regardless.

For example, can we guarantee that a pattern like dhat's for ignoring re-entrant alloc/dealloc calls is safe?

Not without putting undue burden on the allocator. dhat doesn't actually change how allocation works, so regardless of re-entrance it calls System.alloc(layout) or System.dealloc(ptr, layout). If it was actually changing allocation method depending on re-entrance it would need to keep track of which pointers were allocated normally, and which were allocated re-entrant, and call the appropriate dealloc function for each. This makes all deallocation significantly slower regardless of re-entrance, in addition to adding extra complexity.

I did some investigation into some of the problems of making std::thread::spawn global-allocator-free:

The above seems feasible to me, although I also had another thought: I don't know if std::thread::spawn needs to be global-allocator-free. The only use-case I can think of for spawning thread in the global allocator is for spawning cleanup threads, and I think it's perfectly fine if this happens after first init:

if !alloc_is_init() {
    init_alloc();
    // Now the allocator works and may be re-entrant.
    spawn_background_threads();
}

So I'm perfectly happy if we don't make std::thread::spawn global-allocator-free.

While investigating the above I also found the following additional blocker for making std::thread::current() global-allocator-free I didn't realize before: ThreadId::new on platforms which don't have 64-bit atomics uses a Mutex, so Mutex would have to be global-allocation-free (likely unfeasible), or this implementation changed. Which lead me to ask:

It's possible to do this with a fairly simple spinlock, but are there platforms we support that have threads and Mutex but not an atomic we can use for a spinlock?

To answer my own question: std requires the existence of AtomicBool.

I've added a commit to use a spinlock for ThreadId if 64-bit atomics are unavailable, and with that I believe std::thread::current() should be safe to call in a global allocator.

I also investigated environment variables and... it's hopeless without a new API. The current API returns owned Strings or OsStrings both of which allocate with the global allocator. Currently if the global allocator wants to read environment variables it'll have to use libc::getenv (and thus bypass Rust's lock). I don't think that's the end of the world.

We talked about this in today's @rust-lang/libs-api meeting.

We were supportive of doing this, and we agreed that using TLS in an allocator seems desirable.

We also decided there's no point in doing this if we don't make it a guarantee of support. Thus, labeling this libs-api.

@rfcbot merge

Also, before merging this we'd like to see the documentation for TLS updated to make this guarantee.

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Separately, it would also be useful for the global allocator documentation to provide an explicit safelist of other things you're allowed to use.

🔔 This is now entering its final comment period, as per the review above. 🔔

@joshtriplett

Also, before merging this we'd like to see the documentation for TLS updated to make this guarantee.

Done.

Separately, it would also be useful for the global allocator documentation to provide an explicit safelist of other things you're allowed to use.

Where? On the std::alloc page where the #[global_allocator] attribute is described, or on GlobalAlloc?

So with this, for the first time, a program with a #[global_allocator] might still use the System allocator for some std operations the user has no control over? That seems really surprising and potentially breaking to me. The point of #[global_allocator] is to entirely replace which code std uses for allocations. We even have tests relying on that, using a custom #[global_allocator] to count how many allocations have been created. Those tests will now be less effective.

At the very least, this should be documented with that attribute.

Much like Ralf, I am quite surprised at the "allocator fork" occurring here.

Now, I would like to note that there is precedent for this. One of the early pains I encountered in replacing the system allocator with a custom allocator was that the loader will use the system allocator for loading the code (and constants, etc...) regardless. Thus, to an extent, there are already allocations bypassing the global allocator.

Still, up until now, this was a well-defined set. Loaded sections of library/binary would be allocated with the system allocator, all further allocations would be allocated with the global allocator. The divide is clear. And one can (try to) write their own loader if they wish to change the statu quo.

This changes removes control from the hands of the (allocator) developer. It may be fine. It may be the beginning of a slippery slope.

For the point at hand, is there any reason that the thread-local destructors could not be registered in an intrusive linked list instead?

That is, each thread-local variable requiring a destructor would be accompanied by a thread-local:

struct Node {
    pointer: *mut u8,
    destructor: unsafe extern "C" fn(*mut u8),
    next: *mut Node,
}

And destruction would simply consist of popping the first destructor of the linked-list and executing it in a loop.

(This doesn't solve all problems identified in this PR, but it may solve the most salient one)

So with this, for the first time, a program with a #[global_allocator] might still use the System allocator for some std operations the user has no control over? That seems really surprising and potentially breaking to me.

Well, yes and no. I think technically yes, this is the first time specifically System is explicitly called from the Rust standard library.

However, System essentially maps to libc::malloc, which is still called all the time by other libc functions and other things we use internally in std. Is there a reason you specifically want to single out this usage of malloc from the others?
For example if I currently set a breakpoint on malloc, on MacOS we call tlv_get_addr in our thread-local implementation, which calls malloc internally. Why was that okay, but would this be breaking?

Note that this doesn't affect no_std applications which absolutely rely on #[global_allocator] and for which there is no system allocator available. The thread_local! and std::thread implementation is firmly in std.

For the point at hand, is there any reason that the thread-local destructors could not be registered in an intrusive linked list instead?

On some platforms we can only install a key (that is, a pointer) into thread-local storage, which requires boxing of the thread-local variable. We must allocate on these platforms. See here:

We also require allocation for std::thread::current() as the thread handle may survive the thread it refers to, so it can't live on the thread's stack.

This doesn't solve all problems identified in this PR, but it may solve the most salient one.

Well, considering there are platforms which hard require allocation for thread-locals, and every platform requiring allocation for std::thread::current(), I don't particularly agree.

@RalfJung @matthieu-m Sorry, forgot to tag you earlier, awaiting your response.

@Mark-Simulacrum Squashed.

@bors r+ rollup=never

Thanks!

📌 Commit f01e7aa has been approved by Mark-Simulacrum

It is now in the queue for this repository.

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

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Mark-Simulacrum We got sniped by a typo commit, could you r+ again?

@bors r+

📌 Commit db1eda9 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

⌛ Testing commit db1eda9 with merge ffe818a...

The job dist-i586-gnu-i586-i686-musl failed! Check out the build log: (web) (plain enhanced) (plain)

failures:

---- [ui] tests/ui/threads-sendsync/tls-in-global-alloc.rs stdout ----

error: test did not exit with success! code=None so test would pass with `run-crash`
status: signal: 6 (SIGABRT) (core dumped)
command: cd "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/threads-sendsync/tls-in-global-alloc" && RUSTC="/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" RUST_TEST_THREADS="4" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/threads-sendsync/tls-in-global-alloc/a"
stdout: none
--- stderr -------------------------------
fatal runtime error: something here is badly broken!, aborting
------------------------------------------

💔 Test failed - checks-actions

Seems like we hit this assert on i586:

My guess is that the following happens:

1. something allocates before main runs in the spawned thread,
2. this access the #[global_allocator],
3. which calls std::thread::current() in this test,
4. which default-initializes the current thread symbol to a new one (since it wasn't set yet),
5. which makes set_current fail at the start of main.

I'm not sure what the best way to fix this is. The first thing I could think of was to pass the current thread symbol from the newly spawned thread back to the thread which called spawn, but this is a performance regression for everyone as it means the spawning thread is blocked until main of the spawned thread has started running.

Another alternative is that we move the responsibility of calling set_current to the imp::Thread::new implementation, requiring it to be called before any allocations are done.

Another alternative is that we move the responsibility of calling set_current to the imp::Thread::new implementation, requiring it to be called before any allocations are done.

I think that's a good idea, as it would also help to clean up the logic introduced by #144500 – as a matter of fact, I wanted to do something like this as a follow-up to that PR anyway.