Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: No (opsem) Magic Boxes #3712

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

chorman0773
Copy link

@chorman0773 chorman0773 commented Oct 15, 2024

Summary

Currently, the operational semantics of the type alloc::boxed::Box<T> is in dispute, but the compiler adds llvm noalias to it. To support it, the current operational semantics models have the type use a special form of the Unique (Stacked Borrows) or Active (Tree Borrows) tag, which has aliasing implications, validity implications, and also presents some unique complications in the model and in improvements to the type (e.g. Custom Allocators). We propose that, for the purposes of the runtime semantics of Rust, Box is treated as no more special than a user-defined smart pointer you can write today1. In particular, it is given similar behaviour on a typed copy to a raw pointer.

Rendered

Footnotes

  1. We maintain some trivial validity invariants (such as alignment and address space limits) that a user cannot define, but these invariants only depend upon the value of the Box itself, rather than on memory.

@chorman0773 chorman0773 added T-lang Relevant to the language team, which will review and decide on the RFC. T-opsem Relevant to the operational semantics team, which will review and decide on the RFC. labels Oct 15, 2024
@programmerjake

This comment was marked as resolved.

Clarify the constraint o the invariant in footnote

Co-authored-by: Jacob Lifshay <programmerjake@gmail.com>
@clarfonthey
Copy link
Contributor

It feels odd that one of the clear options is left out: why not expose a Unique<T> that has the same semantics as Box<T>, so any smart pointer type can use it?

Like, I definitely agree that Box shouldn't have special semantics that you can't reproduce elsewhere. But among the options, it feels pretty limiting to come to the conclusion that we should eliminate those semantics, rather than just making them reproducible elsewhere.

I agree that whatever happens shouldn't be specific to the Global allocator, though.

@scottmcm scottmcm added the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Oct 15, 2024
@scottmcm
Copy link
Member

scottmcm commented Oct 15, 2024

We maintain some trivial validity invariants (such as alignment and address space limits) that a user cannot define

I, at least, fully expect us to eventually have some way of writing alignment-obeying raw pointers in Rust in some way. If nothing else, slice::Iter is less good than it could be because of the lack of them, and optimizing that type is super-important.

(Transmuting between NonNull<T> and unsafe<'a> &'a T is one thing that might work, for example, though it's also possible that the opsem for that will end up saying that it doesn't and that something else is required instead.)

EDIT: added a word to try to communicate that I wasn't expecting this RFC to include such a type.

@scottmcm scottmcm self-assigned this Oct 15, 2024
@chorman0773
Copy link
Author

I, at least, fully expect us to have some way of writing alignment-obeying raw pointers in Rust in some way. If nothing else, slice::Iter is less good than it could be because of the lack of them, and optimizing that type is super-important.

That's my hope for the future as well, but to avoid the RFC becoming too cluttered, I am refraining from defining such a type in this RFC.

@juntyr
Copy link
Contributor

juntyr commented Oct 16, 2024

Is there a list of optimisations that depend on noalias being emitted for Box’es?

@clarfonthey
Copy link
Contributor

The RFC seems pretty clear that noalias hasn't really provided many benefits compared to being an extra burden to uphold for implementers, but maybe it is worth seeing if there are any sources that can provide a bit more detail on that.

* A pointer with an address that is not well-aligned for `T` (or in the case of a DST, the `align_of_val_raw` of the value), or
* A pointer with an address that offsetting that address (as though by `.wrapping_byte_offset`) by `size_of_val_raw` bytes would wrap arround the address space

The [`alloc::boxed::Box<T>`] type shall be laid out as though a `repr(transparent)` struct containing a field of type `WellFormed<T>`. The behaviour of doing a typed copy as type [`alloc::boxed::Box<T>`] shall be the same as though a typed copy of the struct `#[repr(transparent)] struct Box<T>(WellFormed<T>);`.
Copy link
Member

@kennytm kennytm Oct 16, 2024

Choose a reason for hiding this comment

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

sorry but is the term "typed copy" explained somewhere?

(the explanations I could find are from pretty unofficial places like a reddit1 and urlo2 post)

Footnotes

  1. "they are like memcpy, but the copy occurs with a type, giving the compiler some extra power."

  2. "a typed copy consists of essentially decoding the AM-bytes into the abstract value then encoding that abstract value back to AM-bytes at the new location."

Copy link
Author

Choose a reason for hiding this comment

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

The urlo definition is probably good.
It's defined in the opsem, but I don't know if we have a very good written record of that other than spread arround zulip threads and github issues.

@scottmcm scottmcm removed the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Oct 16, 2024
Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

I'm a fan of this. I think that people moving from Vec<T> to Box<[T]> having to deal with drastically-different soundness rules is a giant footgun, and getting rid of the special [ST]B behaviour here sounds good to me.

@nikomatsakis
Copy link
Contributor

My general take:

The two "endpoints" here are

  • Efficient end-user abstractions: this allows most safe code to run faster. This would have strong alias requirements and would not expose raw/unsafe details. This permits non-obvious optimizations (e.g., small string optimization or 0-length capacity).
  • Building blocks for unsafe code: this exposes raw/unsafe details.

From what I can tell, we current orient Box as the former but Vec and String as the latter. That seems backwards, since if anything those are far more useful as abstractions than Box is.

If I could go back in time, I think I would favor end-user abstractions and offer different types (e.g., RawVec or RawBuffer or something) that exposed their innards, but I think that ship has sailed, and we might as well embrace the current situation (which is nice in some ways too).

@nikomatsakis
Copy link
Contributor

The RFC would benefit from some attempt to quantify the impact on performance, though our lack of standardized runtime benchmarks makes that hard.

@traviscross
Copy link
Contributor

@rust-lang/opsem: We were curious in our discussions, does this RFC represent an existing T-opsem consensus?

@chorman0773
Copy link
Author

chorman0773 commented Oct 16, 2024

We were curious in our discussions, does this RFC represent an existing T-opsem consensus?

It does not represent any FCP done by T-opsem, which is why I've included them here. The claims I make, including those about the impact on the operation semantics, are included in the request for comment and consensus.

The RFC would benefit from some attempt to quantify the impact on performance, though our lack of standardized runtime benchmarks makes that hard.

I recall some perf PR's (using the default rustc-perf suite) being done to determine the impact, which showed negligible impact. I can probably pull them up at some point in the RFC's lifecycle.


(Note that we do not define this type in the public standard library interface, though an implementation of the standard library could define the type locally)

The following are not valid values of type `WellFormed<T>`, and a typed copy that would produce such a value is undefined behaviour:
Copy link
Member

Choose a reason for hiding this comment

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

The Reference has been adjusted a while ago to state validity invariants positively, i..e by listing what must be true, instead of listing what must not be false. IMO that's more understandable, and the RFC should be updated to also do that.

@RalfJung
Copy link
Member

RalfJung commented Oct 17, 2024

I agree that whatever happens shouldn't be specific to the Global allocator, though.

There are patterns of using a custom per-Box allocator that are incompatible with the aliasing requirements, at least under our current aliasing models. See rust-lang/miri#3341 for an example. So if we always make Box be unique, we have to declare those allocators to be UB.

Is there a list of optimisations that depend on noalias being emitted for Box’es?

It's "every LLVM optimization that looks at alias information". The question is how much that matters in practice, which is hard to evaluate.

We were curious in our discussions, does this RFC represent an existing T-opsem consensus?

As Connor said, not in any formal sense. Several opsem members have expressed repeatedly that they want to see noalias on Box go, but I don't know whether we have team consensus on this.

My own position is that I love how this simplifies the model and Miri, I am just slightly concerned about this being an irreversible loss of optimization potential that we might regret later. Absence of evidence of optimization benefit is not evidence of absence. Our benchmarks likely just don't have a lot of functions that take Box<T> by value. However, that in itself is an indication that the optimization benefit is likely limited.

Is there a way we can query the ecosystem for functions taking Box<T> by value?

- While the easiest alternative is to do nothing and maintain the status quo, as mentioned this has suprisingly implications both for the operational semantics of Rust
- Alternative 2: Introduce a new type `AlisableBox<T>` which has the same interface as `Box<T>` but lacks the opsem implications that `Box<T>` has.
- This also does not help remove the impact on the opsem model that the current `Box<T>` has, though provides an ergonomically equivalent option for `unsafe` code.
- Alternative 3: We maintain the behaviour only for the unparameterized `Box<T>` type using the `Global` allocator, and remove it for `Box<T,A>` (for any allocator other than `A`), allowing unsafe code to use `Box<T, CustomAllocator>`
Copy link
Member

Choose a reason for hiding this comment

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

This is actually the status quo, since rust-lang/rust#122018

@clarfonthey
Copy link
Contributor

clarfonthey commented Oct 17, 2024

Just to follow up on some of the discussion, it wasn't immediately clear to me that types similar to Box, like Vec and Arc, genuinely don't have these semantics, even though it is implied by the fact that Box is unique in this regard. I think this is worth emphasising more in the text of the RFC, since in a very real sense, we've been going without these semantics for most[citation needed] parts of Rust totally fine, which further emphasises less will be lost by removing it for Box.

I would still love if there's more data showing the lack of returns on noalias optimisations, since it feels wrong that something with a lot of history and usage isn't helping that much, but the fact that Vec and other types don't have this optimisation at least helps us understand that it's not going to cause any performance issues if removed.

…C++ counterpointer `std::unique_ptr`, in the prior art section
@chorman0773
Copy link
Author

chorman0773 commented Oct 17, 2024

I am just slightly concerned about this being an irreversible loss of optimization potential that we might regret later

FTR, speaking right now as one of the main developers of lccc, my opinion is that the best way to mitigate any loss of future optimization potential is to just be far more granular with &mut. I don't think spamming extra noalias on parameters is necessary if we just emit more metadata on derefs (llvm's scoped noalias and dereferenceable, and lccc's unique and dereferenceable type attributes). If there are problems with doing that, I don't think this is a place where we should necessarily bend the whole language to this one attribute on one backend, especially given the complexity of maintaining the feature as-is, where we keep running into fundamental issues with the special treatment of Box. If there are legitimate optimizations lost as a result of the function level noalias, that can't be made up with more granular scoping on dereferences, that may be a different consideration, but my view is that the language after this change has the semantics necessary to justify at the very least a majority of the optimizations that may ultimately be lost, and its up to rustc and llvm (and other compilers) to make use of those semantics if they wish to.

Just to follow up on some of the discussion, it wasn't immediately clear to me that types similar to Box, like Vec and Arc, genuinely don't have these semantics, even though it is implied by the fact that Box is unique in this regard. I think this is worth emphasising more in the text of the RFC, since in a very real sense, we've been going without these semantics for most parts of Rust totally fine, which further emphasises less will be lost by removing it for Box.

I mentioned Vec<T> in particular, and also mentioned std::unique_ptr<T> from C++, which is the most closely equivalent type, and lacks the same semantic implications (and also optimizations).

@scottmcm
Copy link
Member

Just to follow up on some of the discussion, it wasn't immediately clear to me that types similar to Box, like Vec and Arc, genuinely don't have these semantics

And more than them just not having them, IIRC someone tried to implement Vec<T> as the obvious wrapper around Box<[MaybeUninit<T>]>, and in the process found out that lots of people are depending on Vec not having them.

@RalfJung
Copy link
Member

RalfJung commented Oct 18, 2024 via email

text/3712-box-yesalias.md Outdated Show resolved Hide resolved
Co-authored-by: Ralf Jung <post@ralfj.de>
@ZuseZ4
Copy link

ZuseZ4 commented Oct 26, 2024

Just catching up with things. I wanted to share that for automatic differentiation (and likely GPU Kernels written in pure Rust) noalias is very relevant. At the llvm-dev meeting I've been presenting 5 benchmark that showed Performance Penalties in 4/5 Benchmarks ranging from 30% - 10x by removing noalias from the Rust code. Removing noalias from the C++ version of the benchmark also had large performance impacts (up to 4x), so it's not a pure Rust problem, and noalias is just generally very valuable for AutoDiff and GPU support. The only benchmark not suffering didn't had enough pointers/references for noalias to be valuable.

For now I have to admit that I have written all benchmarks using slices or arrays and not box, but both autodiff and GPU offload work on llvm-ir, and there are logical reasons about why noalias has such a big performance impact. So I don't see any reason why the penalty for noalias should be smaller for box. I will try to get at least some of these Benchmarks ported to use a Box instead of slices to prove my point, but for now I can share the existing slice/array versions that I used for my talk: EnzymeAD/Enzyme#1797

That being said, both autodiff and gpu-offloading are of course only experimental project goals, and noalias on box is of course less relevant than noalias on say slices. However, I think especially efficient GPU support is valuable beyond the scientific Computing/HPC/ML crowd and of interest for a lot of people. It might still be worth dropping it if it causes too many compiler issues, I haven't read up on the whole discussion yet. But we should at least be aware then that it likely will have relevant performance impact for these targets. If desired, I can also reach out to some of the LLVM people involved in GPU programming, they surely can provide more numbers/examples on noalias usage there.

Edit: Iirc Nikita also pointed out that people often pass Box behind a reference, which would keep noalias, so that might be enough for our cases (I will need to check the IR).

@GoldsteinE
Copy link

So I don't see any reason why the penalty for noalias should be smaller for box.

The idea is that box doesn’t get passed around that much: you usually just pass a reference, and mutable references have noalias.

(This is distinct from passing a box behind a reference / a reference to a box, which would have noalias on the reference, but not on the box)

@ZuseZ4
Copy link

ZuseZ4 commented Oct 26, 2024

Agree, but in our case the existence of 2+ Box types inside of a function (with at least one write to it) would often force us to cache the data behind all Boxes for autodiff (instead of just the one we modified), so it's not only about passing it to other functions. Vectors also don't have noalias iirc so I think also any combination of 2+ vec/box types existing in one function could then cause unnecessary copies.

I want to be clear that the average IR from Rust is still miles ahead of the average IR we get from C++ (since people rarely use restrict), but the difference would be that people manually could fix C++ by adding restrict, whereas Rust users wouldn't be able to add noalias to Rust's builtin types. It might still be worth to sacrifice performance in some niche cases for easier correctness, I'll try to write up in more detail under which cases lacking noalias would have an impact for the HPC/etc. community so people can decide if it's worth it. I just wanted to share this as an FYI, while I'm working out more details on it.

@chorman0773
Copy link
Author

Within a function you already don't get noalias, due to limitations of llvm and what code rustc emits.

@ZuseZ4
Copy link

ZuseZ4 commented Oct 26, 2024

Are you talking about sret preventing noalias? Yes, we're following the work on the llvm side too, e.g. https://discourse.llvm.org/t/restrict-noalias-for-struct-members/78568, so we hope to increase the number of locations where llvm can use noalias info. I however don't have precise insights on how many of these llvm noalias improvements will later be useable by Rust and where we e.g. already allow unsafe code to alias and thus won't be able to add noalias.

@RalfJung
Copy link
Member

RalfJung commented Oct 27, 2024

Are you talking about sret preventing noalias?

No, he's talking about the fact that syntactically in LLVM IR noalias only exists for function arguments, not inside a function.

Agree, but in our case the existence of 2+ Box types inside of a function (with at least one write to it) would often force us to cache the data behind all Boxes for autodiff (instead of just the one we modified), so it's not only about passing it to other functions.

This can't be happening today since, as Connor said, noalias in LLVM is only for arguments. So are you speculating here about what benefits there might be in a hypothetical future where Box has noalias and also LLVM has a way to represent this inside a function, and Rust uses it?

Would be good to know which parts of your comments are based on concrete data and which are speculative.

@RalfJung
Copy link
Member

and noalias is just generally very valuable for AutoDiff and GPU support

That's a good point that I had not considered. Would be good to get some data on the GPU side here, too.

@scottmcm
Copy link
Member

I want to be clear that the average IR from Rust is still miles ahead of the average IR we get from C++ (since people rarely use restrict), but the difference would be that people manually could fix C++ by adding restrict, whereas Rust users wouldn't be able to add noalias to Rust's builtin types.

They could delegate to an inner function taking &mut to get noalias though, no?

(Like how autovectorization works better on &mut [T]/&[T] parameters than on Vec<T>, so you do the same thing there.)

@comex
Copy link

comex commented Oct 27, 2024

This can't be happening today since, as Connor said, noalias in LLVM is only for arguments. So are you speculating here about what benefits there might be in a hypothetical future where Box has noalias and also LLVM has a way to represent this inside a function, and Rust uses it?

Just to clarify, LLVM does have something called noalias that is not for arguments, namely noalias metadata. rustc will not directly produce this metadata, but it arguably could and should. Also note that this metadata can already end up in Rust code because LLVM converts function-argument-noalias to scoped-noalias upon inlining. (This does not alter the semantics.)

@RalfJung
Copy link
Member

Yeah noalias metadata exists but it's largely a separate system from noalias arguments, as far as I can tell. If the semantics of noalias arguments are poorly understood, then the semantics of noalias metadata are a complete mystery -- I don't know anyone who has even the faintest clue of what the actual rules for the metadata are (in an operational sense).

@scottmcm
Copy link
Member

I think we should just do this, and make Box like Vec and Rc and friends in this way.

I agree with Niko's post above that ideally we'd have made Box and Vec the super-strict ones from 1.0 -- I imagine things like Vec::pop writing uninitialized over the memory, so you can't use it later even in unsafe -- but that ship has long since sailed.

Thus I think it's better that we not have a big footgun of code that's sound with Vec<T> being unsound with Box<[T]>, even if that will potentially cost us some optimizations. After all, one already should write functions over &[T] and &mut [T] instead of &Vec<T> and &mut Vec<T>, so this would just make that true for Box as well -- if you really need the noalias rules, delegate to something taking references.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 29, 2024

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

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Nov 29, 2024
@traviscross
Copy link
Contributor

traviscross commented Nov 29, 2024

@rustbot labels +I-lang-nominated

Thus I think it's better that we not have a big footgun of code that's sound with Vec<T> being unsound with Box<[T]>

Is this footgun actually big, though? It gave me a lot of pause about doing this when @RalfJung said:

I am not aware of a single real-world usage of Vec that actually breaks the noalias rules.

If that's true, maybe we should consider whether we can in fact go the other way somehow.

As a rule, I would like to make things less magical. But here -- I don't know -- I wonder whether it's an improvement to lose this optimization and to, in some situations, have to suggest that people write inner functions like this:

// Without the inner function, this would take a long long time with
// `-Zbox-noalias=no`.
pub fn f_box(mut x: Box<[u64; 1]>, y: Box<[u64; 1]>) -> u64 {
    fn inner(x: &mut [u64; 1], y: &[u64; 1]) -> u64 {
        let mut z: u64 = 0;
        for i in 0..const { 2u64.pow(40) } {
            x[0] = i;
            z = z.wrapping_add(y[0]);
        }
        z
    }
    inner(&mut *x, &*y)
}

That's its own kind of new footgun that we'd be adding.

It's particularly kind of odd here since there's still of course the library invariant that a Box exposed to safe Rust must not alias other live things. Obviously that constrains potential use cases rather dramatically. And -- it's maybe a minor point, but -- from a pedagogical perspective, it seems unfortunate to increase the gap between the language and library invariants.

The stated motivation for this RFC leans a lot on the ManuallyDrop<Box<T>> case, but we fixed that in a different way in RFC 3336 (MaybeDanging).

So I just wonder whether there's a strong enough reason to do this, given (quoting @RalfJung again):

I am just slightly concerned about this being an irreversible loss of optimization potential that we might regret later. Absence of evidence of optimization benefit is not evidence of absence.

@rustbot rustbot added the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Nov 29, 2024
@saethlin
Copy link
Member

saethlin commented Nov 29, 2024

I am not aware of a single real-world usage of Vec that actually breaks the noalias rules.

What is this based on? I'm not aware of any checker that we have for the noalias rules that would enable a systematic search.

@RalfJung
Copy link
Member

RalfJung commented Nov 30, 2024

It is based on the fact that, to my knowledge, so far we have not discussed even an artificial example of code that violates noalias in Vec.

It is often claimed that examples like this are UB under noalias:

let mut v = Vec::with_capacity(10);
v.push(0);
let ptr0 = &raw mut v[0];
let mut v = v; // move the Vec
v.push(1);
let ptr1 = &raw mut v[1];
ptr::swap(ptr0, ptr1);

That's just not true. Stacked Borrows and Tree Borrows rule out patterns like this, but noalias is less restrictive than that. Specifically, noalias on an argument is only relevant inside the function; once the function has returned, there are no more aliasing restrictions on pointers derived from such an argument. In contrast, Stacked Borrows and Tree Borrows remember "where the returned reference comes from" and keep enforcing non-aliasing.

We don't have any way to do systematic testing. But not even having a single realistic example of such UB does make me wonder how much of a problem it would be. IMO we should be asking: what is the support for the claim that adding noalias to Vec's buffer pointer would cause any UB in code we care about?

@RalfJung
Copy link
Member

RalfJung commented Nov 30, 2024

So, here is such an example:

unsafe fn f(mut v: Vec<i32>, ptr: *mut i32) {
    *ptr = 0;
    v[0] = 0;
}

let mut v = vec![0];
let ptr = &raw mut v[0];
f(v, ptr);

This is UB because inside f, we are accessing v[0] via both v and ptr. We could access other elements in f just fine, and even push_within_capacity to the vector. Only accessing the same element is an issue.

OTOH, also note that we could currently not even compile this to a noalias on the LLVM side since v is passed indirectly. We'd need to do some work on our ABI so that the three Vec fields are passed as three separate arguments.

@GoldsteinE
Copy link

GoldsteinE commented Nov 30, 2024

There’s always an option of having a Box<_> be noalias but not dereferenceable_on_entry. That both doesn’t lose the optimizations in the @traviscross’s example and fixes the ManuallyDrop<_> issue. I am guessing (but I don’t have a proof) that actually dereferencing the Box<_> inside of a function body would lead LLVM to infer dereferencability anyway. (In any case, dereferenceable_on_entry doesn’t even exist, so we’re only losing potential optimizations if it’s ever added)

@RalfJung
Copy link
Member

RalfJung commented Nov 30, 2024

There’s always an option of having a Box<_> be noalias but not dereferenceable_on_entry.

That would have to be a weaker noalias than SB/TB, since their noalias requires dereferenceable_on_entry. SB doesn't support such a weaker noalias at all, would TB support it by treating Box<T> almost like a ZST reference.

@chorman0773
Copy link
Author

I am not aware of a single real-world usage of Vec that actually breaks the noalias rules.

FTR, I don't like this argument - whether or not its true, it has no bearing on what is undefined behaviour in Rust. None of the proposed aliasing models for Rust that I've seen are "Exactly noalias, no more, no less". Whenever rustc emits llvm noalias, that must be justified by undefined behaviour at the rust level.

Part of the point of this RFC is removing special cases in the memory model, so IMO it's completely against the proposal to add even more special-cased rules to SB or TB to handle something closer to what noalias does specifically for Box<T> and Vec<T>.

@RalfJung
Copy link
Member

Rust doesn't yet have an aliasing model, only several WIP proposals. If there's some good benefit from having noalias on Vec arguments, it would be reasonable to say that the proposals should be extended to be able to support that.

Comment on lines +40 to +47
# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

For the remainder of this section, let `WellFormed<T>` designate a type for *exposition purposes only* defined as follows:
```rust
#[repr(transparent)]
struct WellFormed<T: ?Sized>(core::ptr::NonNull<T>);
```
Copy link
Contributor

@traviscross traviscross Dec 1, 2024

Choose a reason for hiding this comment

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

Are there any differences semantically between what is proposed for Box<_> in this RFC and what would be true of MaybeDangling<Box<_>> today?

That is, since we already accepted RFC 3336, if there is no daylight between these, then it should say that normatively, and it perhaps could even lean into that for defining the semantics.

cc @RalfJung

Copy link
Member

Choose a reason for hiding this comment

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

We haven't spelled out whether MaybeDangling<Box<_>> would allow pointers too close to the end of the address space, but it seems reasonable to say "no" to that. In that case the answer to your question is yes, this RFC proposes to weaken Box so that its validity requirements are equivalent to MaybeDangling<Box<_>>.

Copy link
Author

Choose a reason for hiding this comment

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

Are there any differences semantically between what is proposed for Box<_> in this RFC and what would be true of MaybeDangling<Box<_>> today?

Restating Ralf's answer to this, MaybeDangling only removes aliasing invariants, but preserves the normal validity invariants of the type. This RFC proposes to remove the aliasing invariants from Box<_> as a whole, so it would naturally leave it in an identical state to MaybeDangling<Box<_>>.

However, this also spells out that validity invariant in full, which we cannot rely on existing types yet to do.

Comment on lines +25 to +27
In the case of allocators[^3], without special handling of them in the language as well, the protectors assigned to `Box<T>` were violated by (almost) any non-trivial allocator that provides the storage itself (without delegating to something like `malloc` or `alloc::alloc::alloc`). This is because the allocators access the same memory that the `Box` stores to mark it as deallocated and available again. In an extreme example, the same memory could even be yielded back to another `Allocator::allocate` call. Solving this requires special casing `Allocator`s, which is a heavily unresolved discussion, only applying the special opsem behaviour to `Global`, which is opaque via the global allocator functions, or forgoing custom allocators for `Box` entirely (thus depriving anyone needing to use a custom allocator from the user-visible language features `Box` alone provides). With the exception of the former, which is desired for other optimization reasons though [heavily debated and not resolved](https://github.com/rust-lang/unsafe-code-guidelines/issues/442), these solutions are merely solving the symptom, not the problem.

Any `unsafe` code that may want to temporarily maintain aliased `Box<T>`s for various reasons (such as low-level copy operations), or may want to use something like `ManuallyDrop<Box<T>>`, is put into an interesting position: While they can use a user-defined smart pointer, this requires both care on the part of the smart pointer implementor, but also affects the ergonomics and expressiveness of that code, as `Box<T>` has many special language features that surface at the syntax level, which cannot be replicated today by a user-defined type.
Copy link
Contributor

Choose a reason for hiding this comment

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

The problems described in these motivation items are also solved by MaybeDangling, no?

Either way, the motivation should be adjusted to describe this. That is, it's confusing for the motivation to be written as though we did not already cover this ground and accept RFC 3336. If that RFC did solve the problem, but the idea is that the present proposal solves it better for Box<_> somehow, then that should be described here. Or alternatively, if there's some way in which RFC 3336 did not solve the problem, then that should be detailed specifically.

Copy link
Author

@chorman0773 chorman0773 Dec 1, 2024

Choose a reason for hiding this comment

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

Only the last one. unless we say that in order to use a custom allocator with Box, you have always have to wrap the box in MaybeDangling (in which case, I'm not sure how to create one in the first instance).


In the case of `ManuallyDrop<T>`, because `Box<T>` asserts aliasing validity on a typed copy, and is invalidated on drop, it introduces unique behaviour - `ManuallyDrop<Box<T>>` *cannot* be moved after calling `ManuallyDrop::drop` *even* to trusted code known not to access or double-drop the `Box`. No other type in the language or library has the same behaviour[^2], as primitive references do not have any behaviour on drop (let alone behaviour that includes invalidating themselves), and only `Box<T>`, references, and aggregates containing references are retagged on move. There are proposed solutions on the `ManuallyDrop<T>` type, such as treating specially by supressing retags on move, but this is a novel idea (as `ManuallyDrop<T>` asserts non-aliasing validity invariants on move), and it would interfere with retags of references without justification. The proposed complexity is only required because of `Box<T>`.

In the case of allocators[^3], without special handling of them in the language as well, the protectors assigned to `Box<T>` were violated by (almost) any non-trivial allocator that provides the storage itself (without delegating to something like `malloc` or `alloc::alloc::alloc`). This is because the allocators access the same memory that the `Box` stores to mark it as deallocated and available again. In an extreme example, the same memory could even be yielded back to another `Allocator::allocate` call. Solving this requires special casing `Allocator`s, which is a heavily unresolved discussion, only applying the special opsem behaviour to `Global`, which is opaque via the global allocator functions, or forgoing custom allocators for `Box` entirely (thus depriving anyone needing to use a custom allocator from the user-visible language features `Box` alone provides). With the exception of the former, which is desired for other optimization reasons though [heavily debated and not resolved](https://github.com/rust-lang/unsafe-code-guidelines/issues/442), these solutions are merely solving the symptom, not the problem.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't properly explain why "(almost) any non-trivial allocator" violates noalias.

There is a class of non-trivial custom allocators that violates Stacked Borrows, specifically if it accesses "metadata memory" that is stored outside the region returned by the allocator, using the pointer that was passed in to deallocate. If that's what you mean, it should be stated explicitly. Is that really "almost any non-trivial allocator"? That seems like a strong claim. It took a while for Miri to run into this issue.

With Tree Borrows, at least some of these cases are not UB any more, since Tree Borrows supports the &Header pattern.

Copy link
Member

Choose a reason for hiding this comment

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

It took a while for Miri to run into this issue.

Most people likely don't use custom allocators with miri.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. T-lang Relevant to the language team, which will review and decide on the RFC. T-opsem Relevant to the operational semantics team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.