only set noalias on Box with the global allocator #122018

RalfJung · 2024-03-05T10:36:52Z

As discovered in rust-lang/miri#3341, noalias and custom allocators don't go well together.

rustc can now check whether a Box uses the global allocator. This replaces the previous ad-hoc and rather unprincipled check for a zero-sized allocator.

This is the rustc part of fixing that; Miri will also need a patch.

rustbot · 2024-03-05T10:36:59Z

r? @estebank

rustbot has assigned @estebank.
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

rustbot · 2024-03-05T10:37:01Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

RalfJung · 2024-03-05T10:38:11Z

compiler/rustc_codegen_cranelift/src/unsize.rs

-        (&ty::Adt(def_a, _), &ty::Adt(def_b, _)) if def_a.is_box() && def_b.is_box() => {
-            let (a, b) = (src_layout.ty.boxed_ty(), dst_layout.ty.boxed_ty());
-            (src, unsized_info(fx, a, b, old_info))
-        }


@bjorn3 the equivalent function in codegen_ssa does not have this Box special case, so I assume it is not needed here either.

compiler/rustc_hir/src/lang_items.rs

oli-obk · 2024-03-05T11:57:38Z

I don't think we should do this. This makes box more special, when we're trying to make it less special. The FIXME you removed was specifically for making box less special and instead making Unique be the signal for noalias.

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

RalfJung · 2024-03-05T12:21:31Z

I don't think we should do this. This makes box more special, when we're trying to make it less special.

It doesn't make Box any more special than it already is. It works around the fact that Box is both a lang primitive and a libs type, causing a big mess. I didn't create that mess, I am trying to clean it up. This PR is part of the experimentation with the nightly feature that is custom allocators.

We could still have hacks to move the semantics to Unique in the future, e.g. making it Unique<T, A> with the semantics of "it's a unique pointer if A = Global but not otherwise". But without such hacks, Unique is just fundamentally incompatible with custom allocators.

If you have another idea for how to fix rust-lang/miri#3341, I'd like to hear it. :)

The FIXME you removed was specifically for making box less special and instead making Unique be the signal for noalias.

That FIXME is many, many years old and nobody even made an attempt at implementing it. I don't think a random FIXME somewhere in the sources is the right way to track such a fundamental aliasing model question. We have issues tracking that question (rust-lang/unsafe-code-guidelines#384, rust-lang/unsafe-code-guidelines#326).

rustbot · 2024-03-05T12:50:02Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

RalfJung · 2024-03-05T12:50:22Z

Why do cranelift and gcc have their own copies of almost but not quite the same file. :(

RalfJung · 2024-03-05T13:20:09Z

Maybe we should make the pointer field type an assoc type on allocator, then allocators can choose if they want unique or raw pointer behaviour

That's an interesting alternative, but a lot more work and changes the trait. Might be worth bringing up in rust-lang/wg-allocators#122.

But given that the general vibe is slowly moving towards just not having Box be noalias ever, it's unclear whether we really want custom allocators to have the option of being unique pointers.

eddyb · 2024-03-05T13:54:52Z

But given that the general vibe is slowly moving towards just not having Box be noalias ever

^{(extremely-out-of-the-loop voice)} Is the alternative specifically having all the relevant annotations on allocator/deallocator functions, so e.g. arbitrary -> Box<T> user functions can only get the same annotations as today when the result is guaranteed to come from a call to one of the explicitly annotated allocator functions? _{(and no other copies of that pointer could have escaped - I suppose that's one place where Capstone's "linear capabilities" can express more in hardware than CHERI, but I digress)}

If that (or at least something in that general direction) is what's being considered, I can't think of anything I could have against it (not that it matters, I'm only here because @oli-obk pointed out something about ancient FIXMEs and I wanted to rule it out quickly).

In other words: if any of my old FIXMEs/plans rely on the type being special, and the move is to specific functions being special instead, feel free to throw the old stuff out. And debuginfo is even less critical, as long as any regressions are tracked, and the most common cases don't completely break (like how you kept a special case for Box<T, Global>).

RalfJung · 2024-03-05T13:56:50Z

The alternative is to just not annotate Box arguments with noalias. (We already don't annotate return Box values with noalias any more as that was unsound.) I don't see how the functions being special relates to this question. Some functions need to be special as well to justify the return-position noalias on __rust_allocate but that's an entirely different discussion.

oli-obk · 2024-03-05T14:08:40Z

The alternative is to just not annotate Box arguments with noalias

Does that have performance issues or other issues? That seems like it would be the most straight forward impl

Nilstrieb · 2024-03-05T14:10:21Z

The alternative is to just not annotate Box arguments with noalias

Does that have performance issues or other issues? That seems like it would be the most straight forward impl

We have #99527 with neutral numbers for the compiler.

RalfJung · 2024-03-05T14:22:15Z

That seems like it would be the most straight forward impl

There's a lot of baggage here. Like, years of discussions. ;)
Doing this needs some sort of process involving T-lang I think, to achieve consensus that Box<T> does not have aliasing restrictions.

Meanwhile I just want to fix the problem with custom allocators that Miri found, without affecting stable.

Nilstrieb · 2024-03-05T14:23:37Z

Just removing it doesn't affect stable either as long as we still pretend that it does have that attribute. At worst it may make stable code slightly slower but that seems unlikely to me, and in fact potential performance complaints (or the absence of such) might be useful data for the discussion!

RalfJung · 2024-03-05T14:25:26Z

It affects the code generated on stable. My aim with this PR is not to do that.

Feel free to file a PR that removes the attribute entirely. :)

oli-obk · 2024-03-05T16:15:17Z

@bors r+

yea, let's leave the noalias on default box discussion for another time

bors · 2024-03-05T16:15:22Z

📌 Commit f391c07 has been approved by oli-obk

It is now in the queue for this repository.

only set noalias on Box with the global allocator As discovered in rust-lang/miri#3341, `noalias` and custom allocators don't go well together. rustc can now check whether a Box uses the global allocator. This replaces the previous ad-hoc and rather unprincipled check for a zero-sized allocator. This is the rustc part of fixing that; Miri will also need a patch.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#121280 (Implement MaybeUninit::fill{,_with,_from}) - rust-lang#121438 (std support for wasm32 panic=unwind) - rust-lang#121658 (Hint user to update nightly on ICEs produced from outdated nightly) - rust-lang#121959 (Removing absolute path in proc-macro) - rust-lang#121961 (add test for rust-lang#78894 rust-lang#71450) - rust-lang#121975 (hir_analysis: enums return `None` in `find_field`) - rust-lang#121978 (Fix duplicated path in the "not found dylib" error) - rust-lang#121991 (Merge impl_trait_in_assoc_types_defined_by query back into `opaque_types_defined_by`) - rust-lang#122016 (will_wake tests fail on Miri and that is expected) - rust-lang#122018 (only set noalias on Box with the global allocator) r? `@ghost` `@rustbot` modify labels: rollup

only set noalias on Box with the global allocator As discovered in rust-lang/miri#3341, `noalias` and custom allocators don't go well together. rustc can now check whether a Box uses the global allocator. This replaces the previous ad-hoc and rather unprincipled check for a zero-sized allocator. This is the rustc part of fixing that; Miri will also need a patch.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#121065 (Add basic i18n guidance for `Display`) - rust-lang#121744 (Stop using Bubble in coherence and instead emulate it with an intercrate check) - rust-lang#121829 (Dummy tweaks (attempt 2)) - rust-lang#121832 (Add new Tier-3 target: `loongarch64-unknown-linux-musl`) - rust-lang#121857 (Implement async closure signature deduction) - rust-lang#121894 (const_eval_select: make it safe but be careful with what we expose on stable for now) - rust-lang#122014 (Change some attributes to only_local.) - rust-lang#122016 (will_wake tests fail on Miri and that is expected) - rust-lang#122018 (only set noalias on Box with the global allocator) - rust-lang#122028 (Remove some dead code) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#121065 (Add basic i18n guidance for `Display`) - rust-lang#121744 (Stop using Bubble in coherence and instead emulate it with an intercrate check) - rust-lang#121829 (Dummy tweaks (attempt 2)) - rust-lang#121857 (Implement async closure signature deduction) - rust-lang#121894 (const_eval_select: make it safe but be careful with what we expose on stable for now) - rust-lang#122014 (Change some attributes to only_local.) - rust-lang#122016 (will_wake tests fail on Miri and that is expected) - rust-lang#122018 (only set noalias on Box with the global allocator) - rust-lang#122028 (Remove some dead code) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122018 - RalfJung:box-custom-alloc, r=oli-obk only set noalias on Box with the global allocator As discovered in rust-lang/miri#3341, `noalias` and custom allocators don't go well together. rustc can now check whether a Box uses the global allocator. This replaces the previous ad-hoc and rather unprincipled check for a zero-sized allocator. This is the rustc part of fixing that; Miri will also need a patch.

miri: do not apply aliasing restrictions to Box with custom allocator This is the Miri side of rust-lang#122018. The "intrinsics with body" made this much more pleasant. :) Fixes rust-lang/miri#3341. r? `@oli-obk`

Rollup merge of rust-lang#122233 - RalfJung:custom-alloc-box, r=oli-obk miri: do not apply aliasing restrictions to Box with custom allocator This is the Miri side of rust-lang#122018. The "intrinsics with body" made this much more pleasant. :) Fixes rust-lang/miri#3341. r? `@oli-obk`

miri: do not apply aliasing restrictions to Box with custom allocator This is the Miri side of rust-lang/rust#122018. The "intrinsics with body" made this much more pleasant. :) Fixes #3341. r? `@oli-obk`

rustbot assigned estebank Mar 5, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 5, 2024

RalfJung commented Mar 5, 2024

View reviewed changes

RalfJung force-pushed the box-custom-alloc branch from 1c01472 to 9906745 Compare March 5, 2024 10:39

RalfJung commented Mar 5, 2024

View reviewed changes

compiler/rustc_hir/src/lang_items.rs Show resolved Hide resolved