only set noalias on Box with the global allocator by RalfJung · Pull Request #122018 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation36 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.

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 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

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

(&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))
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This comment has been minimized.

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.

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).

This comment has been minimized.

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

This comment has been minimized.

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.

This comment has been minimized.

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>).

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.

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

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. :)

@bors r+

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

📌 Commit f391c07 has been approved by oli-obk

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Mar 5, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 5, 2024

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.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 5, 2024

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.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 5, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 6, 2024

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 6, 2024

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.

This was referenced

Mar 6, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 9, 2024

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

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 9, 2024

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

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Mar 10, 2024

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

I hate to reawaken a dead thread, but I'm using custom allocators and ran into the following comment added in this PR:

// It is quite crucial that we only allow the `Global` allocator here.
// Handling arbitrary custom allocators (which can affect the `Box` layout heavily!)
// would need a lot of codegen and interpreter adjustments.
#[unstable(feature = "dispatch_from_dyn", issue = "none")]
impl<T: ?Sized + Unsize<U>, U: ?Sized> DispatchFromDyn<Box<U>> for Box<T, Global> {}

Could somebody help enlighten me as to why dynamic dispatch is impossible from a Box<T, CustomAlloc>? I would have thought that the source of the memory for the Box shouldn't matter.
It is quite tough as I am converting a bunch of Box<T> to Box<T, CustomAlloc> and my trait objects no longer compile.
For instance, I would like to have the code

trait Foo {
    fn foo(self: Box<Self, CustomAlloc>;
}

but then a Box<dyn Foo> only compiles when CustomAlloc = Global.

Edit: DispatchFromDyn is also not implemented for Rc<T, CustomAlloc> or Arc<T, CustomAlloc>. I'm wondering if that has the same limitations as Box or not.

Please ask that on Zulip or IRLO; it's not even related to noalias or this PR -- the PR just clarifies what was already an inherent part of the design here.

rustbot added the A-box

Area: Our favorite opsem complication

label

Feb 5, 2025

Labels

A-box

Area: Our favorite opsem complication

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

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.