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

Document interaction between std::alloc and Box::{into_raw, from_raw} #53039

Closed
FenrirWolf opened this issue Aug 3, 2018 · 18 comments
Closed
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@FenrirWolf
Copy link
Contributor

FenrirWolf commented Aug 3, 2018

The current documentation for Box::from_raw states that Boxes can only be made from raw pointers previously derived from Box::into_raw, but now that there are stable functions to directly access the allocator, one can imagine writing code like this:

use std::alloc::{alloc, Layout};
use std::ptr;

fn main() {
    let boxed = unsafe {
        let raw = alloc(Layout::from_size_align(4, 4).unwrap()) as *mut i32;

        ptr::write(raw, 42);

        Box::from_raw(raw)
    };

    println!("{}", boxed); // prints `42` with `Box`'s `Display` impl

    // `boxed` gets dropped here instead of deallocating directly with `std::alloc::dealloc`
}

Is such code sound? Or is Box's allocator still considered unspecified?

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 3, 2018
@sfackler
Copy link
Member

sfackler commented Aug 3, 2018

I think we probably want to guarantee that Box uses the global allocator (until we add an allocator type parameter) and that this kind of thing is okay. @rust-lang/libs thoughts?

@FenrirWolf
Copy link
Contributor Author

Same thing for Vec I imagine? And also for going the other way around? (i.e. Box::into_raw and then calling alloc::dealloc on the pointer with the appropriate Layout)

@steveklabnik
Copy link
Member

steveklabnik commented Aug 4, 2018 via email

@FenrirWolf
Copy link
Contributor Author

FenrirWolf commented Aug 4, 2018

Would the stdlib ever be using two system allocators at once? I can see that being a problem if you were, say calling a function in a C dependency that in turn calls out to an allocator and hands you the resulting pointer, or something like that. But if std itself ever called out to multiple global allocators, then even Box::into_raw and Box::from_raw wouldn't necessarily be compatible either right?

@steveklabnik
Copy link
Member

steveklabnik commented Aug 4, 2018 via email

@sfackler
Copy link
Member

sfackler commented Aug 4, 2018

@steveklabnik You're right that memory allocated with malloc or HeapAlloc etc is not necessarily compatible with Box and Vec, but memory allocated with std::alloc::{alloc, alloc_zeroed} etc should be.

@vitalyd
Copy link

vitalyd commented Aug 5, 2018

Is there a guarantee how Box interacts with the allocator? In other words, what if Box were to align the allocations to, say, a cacheline size and not to the natural size/alignment of the value inside? Or maybe it overallocates and uses portions of the allocation for some (future) metadata. These are contrived examples, but the gist seems reasonable - to me, Box is a pseudo-allocator itself, at least as far as today’s docs go.

Or is that not the case?

@sfackler
Copy link
Member

sfackler commented Aug 5, 2018

I don't think any of those things are the case.

@vitalyd
Copy link

vitalyd commented Aug 5, 2018

@sfackler, I’m willing to buy that but what leads you to that conclusion? Is there some part of the documentation that I overlooked?

@FenrirWolf
Copy link
Contributor Author

FenrirWolf commented Aug 5, 2018

Sure, the docs might imply that currently, hence the issue to have that changed =P

@sfackler
Copy link
Member

sfackler commented Aug 5, 2018

@vitalyd It's how Box is implemented, but beyond that the "job" of Box is just to be a smart pointer around a heap allocation. If you want to cache align or whatever you'd pick a type that's built around doing that.

@SimonSapin
Copy link
Contributor

Today’s Box and Vec implementation do not do anything clever with over-alignment for example, so this proposal would be factually correct. But it is a real question how much of that we want to guarantee in docs v.s. what optimizations we might want to do in the future. For example we’ve talked about giving Vec<T> the semantics of Cow<'static, [T]> (or possibly only String / Cow<'static, str>).

But we’re already constrained by the existence of Vec::from_raw_parts, so maybe this docs proposal doesn’t add additional constraints.

@alexcrichton
Copy link
Member

I suspect that this is defacto true today in the sense that it's "as if" we've documented this anyway, likely with unsafe code relying on it. That, plus what @SimonSapin was mentioning, I think we should go ahead and document this.

@FenrirWolf
Copy link
Contributor Author

Also it looks like the interaction between alignment and deallocation isn't settled yet (see #32838), so if this does get documented there would need to be discussion about the ability or inability to put specially aligned memory in a Box or Vec (i.e. if the example code would still be valid if the layout were declared as, say, Layout::from_size_align(4, 4096))

@vitalyd
Copy link

vitalyd commented Aug 7, 2018

I realize that cache aligning inside Box is dubious - that was meant as a (probably bad) example of Box internally using the allocator not as one would expect. Put another way, my only point is that knowing Box uses std::alloc doesn't seem enough - we would also need a guarantee on how it uses the allocator so we can accept/reject @FenrirWolf's example in the first post as being the same as what Box would do.

@FenrirWolf
Copy link
Contributor Author

I don't think anyone misunderstands what you're getting at. I just don't see "making a guarantee" and "documenting the behavior" as separate things. What use is a guarantee that's not documented? Right now the implementation happens to line up such that the example is valid, but it's not guaranteed. So the only question is whether it's desirable to guarantee that in documentation, or to not so that the implementations of Box and Vec can retain extra flexibility.

@SimonSapin
Copy link
Contributor

Although the issue description is only about Box, some of the code for Box<str> and Box<[T]> uses RawVec internally. So if we’re going to document such guarantees, we probably should do it for Box and Vec at the same time.

@FenrirWolf
Copy link
Contributor Author

Looks like this was addressed by #58183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants