allow Allocators to be used as #[global_allocator]s

[joboet]

The (hopefully) immanent stabilisation of the Allocator trait raises the question of what is to be done about the older, already-stable GlobalAlloc trait. In my opinion, having two nearly-identical traits for the same purpose is needlessly confusing. Going forward, Allocator as the more modern interface should be the allocator trait.

With Allocator being currently unstable, there is the possibility of implementing GlobalAlloc for all Allocators, thereby allowing them to be used as #[global_allocator] and allowing crates to seamlessly (and semver-compatibly) switch to Allocator. However, unconditionally implementing GlobalAlloc presents a footgun to users, as e.g. using Global as #[global_allocator] will lead to infinite recursion. @nia-e initially tried to resolve this in e1b7097 (#156882) by using weird trait trickery to implement GlobalAlloc for every allocator except Global. But this does not go far enough, e.g. a bump allocator that itself allocates from Global is similarly unsuitable as global allocator.

Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as #[global_allocator]:

// in core::alloc

trait GlobalAllocator: Allocator {}

GlobalAlloc can then be implemented for all GlobalAllocators:

impl<A> GlobalAlloc for A
where
    A: GlobalAllocator
{
    /* ... */
}

This provides a backwards-compatible way for allocator libraries to switch to the new interface and allows deprecating GlobalAlloc (not done here). Over time, I expect that GlobalAlloc will become more and more of an implementation detail of the #[global_allocator] macro (for instance, one might add perma-unstable, hidden methods for things like grow_zeroed that are customised only by the blanket implementation).

With regards to naming, I chose GlobalAllocator to mirror Allocator. GlobalAlloc should probably be deprecated quickly after stabilising GlobalAllocator to avoid confusion. For the same reason, I think it'd be better to add GlobalAllocator before stabilising Allocator – but that is not a necessity.

r? @nia-e
@rustbot label +I-libs-api-nominated

[nia-e]

Should this be an unsafe trait? If so the safety contract can just be that it doesn't use std, and so we can do what was suggested in rust-lang/libs-team#743

View changes since the review

[joboet]

I initially thought that was unnecessary, but it definitely has to be: GlobalAlloc requires the allocator not to panic and allows the allocation-removal optimisation. These two requirements are not on Allocator, so I'll add them here.

[nia-e]

I think we want those requirements on Allocator as well, but I'd like to extend GlobalAllocator in the future with stuff that might be unsafe to set incorrectly (see https://hackmd.io/nNHdKkp1TTK7jat0I-ABqA#Conditional-reentrancy-in-std). Also, it might make sense to make Sync a supertrait too?

[joboet]

I don't think adding Sync is a good idea as it may make it impossible for crates to upgrade from implementing GlobalAlloc to Allocator + GlobalAllocator.

[nia-e]

Seems great! I just had one two comments, but otherwise r=me

[nia-e]

Suggested change

	A: GlobalAllocator,
	A: GlobalAllocator + ?Sized,

View changes since the review

[nia-e]

Test failure may be related to rust-lang/miri#5067, see if setting that flag addresses it

[theemathas]

The following code does not compile on nightly, but compiles with this PR. That is, this PR has insta-stable behavior.

use std::alloc::{GlobalAlloc, System};

fn require_global_alloc<T: GlobalAlloc>() {}

fn main() {
    require_global_alloc::<&System>();
}

[Mark-Simulacrum]

Thus, with this PR, I'd like to propose adding a new marker trait for allocators that can be used as #[global_allocator]:
[...]
allows deprecating GlobalAlloc (not done here).

I think this plan is reasonable, but I would strike marker trait: if we're writing a new trait I think it would also make sense to land rust-lang/libs-team#743 as required for the new trait, forcing implementors to opt in or out of the re-entrancy guarantees. I'd do that for GlobalAlloc too, we just can't do that backwards compatibly today.

We have to have defaults anyway, so it's not too big a deal, but I think it's good for users to think about the configuration of at least the re-entrancy guarantees (not sure if we have other properties we'd put in the same bucket).

cc @orlp

[nia-e]

Agreed with Mark here, this can be an unsafe trait with an associated constant à la REENTRANT_IN_STD: bool. we can just document that it's always ok to set it to true

[nia-e]

We discussed this in the libs-api meeting today; our conclusions were

• Yes, we want this trait
• It should be a subtrait of Allocator + Sync + 'static, aka the people who write #[global_allocator] static mut FOO are wrong (regardless, the global allocator gets reexported as the Global type which is Sync + 'static)
• We probably want an associated constant boolean for std reentrancy (the name needs bikeshedding), so we'd like to see that in this PR
• We should ditch the impl GlobalAllocator for &GlobalAllocator since folks can manually impl this on whatever type they want to actually use as a global allocator, therefore this will have no insta-stable behaviour & needs no FCP

[rustbot]

miri is developed in its own repository. If possible, consider making this change to rust-lang/miri instead.

cc @rust-lang/miri

[rustbot]

This PR was rebased onto a different main 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.

[joboet]

I've added the bounds and removed the reference blanket-implementations, but would prefer not to add the reentrancy constant in this PR for fear of it being bikeshedded to eternity – is it okay to make that change later (obviously still before stabilisation)?

On second thought, GlobalAllocator definitely needs to be an unsafe trait, as implementers also need to uphold the no-unwind and non-observability contracts that are currently imposed on GlobalAlloc.

Also I've added another commit that probably leads to better optimisation of the GlobalAlloc blanket implementation: all global allocator calls are made with non-zero size, which we can inform the optimiser of by doing assert_unchecked. This might be problematic in the unlikely case that a crate defines a GlobalAlloc type that promises to accept zero-size requests, and wants to upgrade that to GlobalAllocator. I suppose the safer option would be to insert the assumptions in the #[global_allocator] expansion instead, but that seems like a lot of hassle for so unlikely a scenario.

@rustbot ready

[rust-log-analyzer]

The job aarch64-gnu-llvm-21-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

test [ui] tests/ui/zero-sized/zero-sized-btreemap-insert.rs ... ok

failures:

---- [ui] tests/ui/allocator/not-an-allocator.rs#u stdout ----
Saved the actual stderr to `/checkout/obj/build/aarch64-unknown-linux-gnu/test/ui/allocator/not-an-allocator.u/not-an-allocator.u.stderr`
diff of stderr:

6 LL | static A: usize = 0;
7    |           ^^^^^ the nightly-only, unstable trait `GlobalAllocator` is not implemented for `usize`
8    |
---
To only update this specific test, also pass `--test-args allocator/not-an-allocator.rs`

error in revision `u`: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/aarch64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/allocator/not-an-allocator.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/aarch64-unknown-linux-gnu/stage2" "--target=aarch64-unknown-linux-gnu" "--cfg" "u" "--check-cfg" "cfg(test,FALSE,u,w)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/aarch64-unknown-linux-gnu/test/ui/allocator/not-an-allocator.u" "-A" "unused" "-W" "unused_attributes" "-A" "internal_features" "-A" "incomplete_features" "-A" "unused_parens" "-A" "unused_braces" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/aarch64-unknown-linux-gnu/native/rust-test-helpers"
stdout: none
--- stderr -------------------------------
error[E0277]: the trait bound `usize: GlobalAlloc` is not satisfied
##[error]  --> /checkout/tests/ui/allocator/not-an-allocator.rs:5:11
   |