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

#[repr(transparent)] for Box<T>? #52976

Closed
RReverser opened this issue Aug 2, 2018 · 39 comments
Closed

#[repr(transparent)] for Box<T>? #52976

RReverser opened this issue Aug 2, 2018 · 39 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@RReverser
Copy link
Contributor

RReverser commented Aug 2, 2018

#43036 added #[repr(transparent)] with one of the goals being safe FFI interaction for newtypes, and added that attribute to bunch of built-in types such as NonZero, Waker, Unique etc.

One use-case that could be really helpful for FFI though is adding it to Box<T>. I haven't found any previous discussions on pros and cons of doing so, so apologies if this is way off and there are obvious reasons why it can't be such, but as far as I can tell, this is not a breaking change neither from API nor from memory representation perspective.

Looking from definition, Box is just a wrapper around Unique which is already #[repr(transparent)] and wraps NonNull which is also #[repr(transparent)] which, in turn, wraps raw pointer which is FFI-safe.

Adding this attribute to Box would make it transparent wrapper all the way down around a raw pointer, and so would allow to do FFI interactions by simply leaking Box as return type in allocator functions and accepting it as-is in deallocator function without manual into_raw and from_raw conversions.

Aside from ergonomics improvement (allocation / deallocation is pretty common operation in cdylibs), this would also allow code to be more self-documenting as just from looking at rustdoc you would be able to tell which pointers are owned and will be allocated or consumed by Rust side.

Counter-argument might be that one can implement custom transparent CBox on top of NonNull, but this involves reimplementing many APIs and guarantees Box already provides e.g. Unique is not exposed at the moment, and then you also have various standard traits that "just work" with Box, so if it would be possible to avoid reimplementing all of that with no obvious downsides, it might be still useful to shared library authors.

@mjbshaw
Copy link
Contributor

mjbshaw commented Aug 5, 2018

How does this interact with Drop? And how are foreign functions expected to allocate (using Rust's allocator) the boxed memory?

@CryZe
Copy link
Contributor

CryZe commented Aug 5, 2018

They don't. I already do this in my C API even though it's not entirely defined behavior at the moment: https://github.com/LiveSplit/livesplit-core/blob/3ac0834f70a3963f1b7e4fec84b0803c51e6349e/capi/src/fuzzy_list.rs#L12-L24

@RReverser
Copy link
Contributor Author

Yeah, what @CryZe said. This is not for importing foreign functions, this is for easily and safely exposing Rust functions that allocate+return+deallocate Box via C API.

@glandium
Copy link
Contributor

glandium commented Aug 5, 2018

How would this interact with #50882?

@RReverser
Copy link
Contributor Author

Sadly, they wouldn't be compatible when allocator is not zero-sized. I'm surprised Box is going to store allocator by value (it seems so from the change?) even though in theory same allocator state might be reused for many allocations and must be shared between them... Also it seems quite inefficient for most libs/apps out there that use just one allocator. I guess I'm going to need to dig into that PR deeper tomorrow when I'm not asleep.

@sfackler
Copy link
Member

sfackler commented Aug 5, 2018

Also it seems quite inefficient for most libs/apps out there that use just one allocator

The allocator value that would be used in most cases is Global, a zero sized type.

@glandium
Copy link
Contributor

glandium commented Aug 5, 2018

Nothing in Box<T, A: Alloc>(Unique<T>, A) says the allocator is stored by value. It all depends what Alloc is impl'ed for. It could be a ZST, like Global, or a reference (impl Alloc for &MyAlloc).

@RReverser
Copy link
Contributor Author

@sfackler The question is more whether this would be guaranteed for any allocators (I suspect not). In that case, repr(transparent) would indeed not work, but I suspect there are other parts of compiler that currently rely on lang item behind Box being just a pointer...

@CryZe
Copy link
Contributor

CryZe commented Aug 6, 2018 via email

@RReverser
Copy link
Contributor Author

@CryZe

hantomData would / should be used instead

Sorry, I didn't get it - instead of what?

@CryZe
Copy link
Contributor

CryZe commented Aug 6, 2018

PhantomData<A> instead of an embedded A, which could not be zero sized.

@RReverser
Copy link
Contributor Author

Agreed. IIUC the motivation for storing A is to give flexibility and allow several instances of the same allocator, each with its own state, but this looks like quite a niche usecase to be covered by stdlib.

@RReverser
Copy link
Contributor Author

RReverser commented Aug 6, 2018

Also, I suppose, if allocator does want to share some extra data, it can always allocate pointer with larger size and store that metadata (or a pointer to one) alongside actual data in the heap.

@sfackler
Copy link
Member

sfackler commented Aug 6, 2018

Why does Box need to differ from unique_ptr in this respect?

@RReverser
Copy link
Contributor Author

Do you mean C++ unique_ptr? Why would Box be similar to it? For one, Rust already has lots of differences with C++, including in memory management, and in particular availability of zero-sized types which is what @CryZe and me are suggesting to use for custom allocators.

@sfackler
Copy link
Member

sfackler commented Aug 6, 2018

Box is the direct analogue of C++'s unique_ptr. If I understand it correctly, you want to prohibit the use of stateful allocators (e.g. an arena) with Box because it will allow you to treat Box the same as a raw pointer for FFI?

@RReverser
Copy link
Contributor Author

RReverser commented Aug 6, 2018

Yes, but not necessarily prohibit - as I said, even for cases where allocator does need to store some unique state (that can't be put into lazy_static! or similar), it can do so on the heap alongside actual data, like e.g. some implementations of malloc do for the size of allocated data.

So this preserves flexibility for allocators while allows to use Box across FFI boundary without transformations, just like references.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 7, 2019

Right now, Box<T, A> is not a thing: we only have Box<T>, which is a wrapper over Unique<T> which is indeed transparent. I think Box<T> not being transparent is a serious bug.

I understand the motivation of extending Box<T> to support Box<T, A> in the future. That's compatible with Box<T> being repr(transparent), it just constraints A to be a ZST.

I understand the desire to allow A's that aren't ZSTs, but it is IMO the job of those extending Box with that to figure out a way of doing so without breaking the guarantee of Box<T> being transparent.

I haven't heard any arguments in support of not making this change now. So are there any? Otherwise we should just make Box<T> transparent now.

@RReverser
Copy link
Contributor Author

I'm absolutely still in favour of this change, but the backpressure was hard when I raised the issue. Did it change / can we agree allocators to be limited to PhantomData<...whatever...> or any other ZST now?

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

FWIW, we don't even have to apply the repr(transparent) attribute to Box<T>, we just need to guarantee that Box<T> is layout compatible with *mut T under certain conditions. That can be simply done in the documentation of Box<T> for the time being.

@RReverser
Copy link
Contributor Author

That can be simply done in the documentation of Box for the time being.

Well, it can't because it's not guaranteed by the compiler at all (even if happens to be compatible on popular platforms).

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

If the Box documentation says that Box is guaranteed to have the same layout as a *mut T, and the compiler violates it, the compiler has a bug.

@RReverser
Copy link
Contributor Author

Yeah, but documentation currently doesn't say that, and merely writing that down won't guarantee that it actually works that way everywhere :)

@RReverser
Copy link
Contributor Author

I'd much rather have a strict #[repr(transparent)] so that this guarantee would derive naturally, and future extensions to Box would be constrained to adhere to it too.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

Yeah, but documentation currently doesn't say that,

Hence this proposal.

and merely writing that down won't guarantee that it actually works that way everywhere :)

Of course not, that would need to come with tests, but AFAIK the compiler currently gives structs with default layout and a single field the same layout as that field on all platforms that Rust currently supports. So beyond adding a test to liballoc checking that this is the case for Box, not much more would need to be done.

I'd much rather have a strict #[repr(transparent)] so that this guarantee would derive naturally,

That would prevent extending Box with an allocator type parameter, which is something that we want to do, and an entire working group is trying to solve. Since repr(transparent) would be part of the Box API, removing it later would be a breaking change, so I don't think we can do that.

@RReverser
Copy link
Contributor Author

That would prevent extending Box with an allocator type parameter, which is something that we want to do, and an entire working group is trying to solve.

It wouldn't prevent it, just constraint to using ZST as an allocator parameter, which is what I suggested originally and you seemed to support above as well?

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

It wouldn't prevent it, just constraint to using ZST as an allocator parameter

We want to support non zero-sized allocator parameters, and this is incompatible with that.

you seemed to support above as well?

I support guaranteeing that Box<T> has the same layout as *mut T. As mentioned, supporting non ZSTs allocators would be a backward incompatible change if Box<T> is made repr(transparent), so we can't make it repr(transparent).

@RReverser
Copy link
Contributor Author

Okay, I guess I misread

That's compatible with Box being repr(transparent), it just constraints A to be a ZST.

as encouragement of supporting only ZST A's.

stephaneyfx added a commit to stephaneyfx/rust that referenced this issue Jul 10, 2019
This officializes what was only shown as a code example in [the unsafe code guidelines](https://rust-lang.github.io/unsafe-code-guidelines/layout/function-pointers.html?highlight=box#use) and follows [the discussion](rust-lang/unsafe-code-guidelines#157) in the corresponding repository.

It is also related to [the issue](rust-lang#52976) regarding marking `Box<T>` `#[repr(transparent)]`.
@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 10, 2019

@RReverser

@Lokathor pointed out, that this "implementation detail" is "documented" in the repr(transparent) RFC:

As a matter of optimisation, eligible #[repr(Rust)] structs behave as if they were #[repr(transparent)] but as an implementation detail that can't be relied upon by users.

Box<T> is not user code, but part of the implementation, so it could rely on this, and document that, at least for Box<T> this will always hold.

@RReverser
Copy link
Contributor Author

Fine by me - not the outcome I initially hoped for, but it does the trick and reaches same goal anyway :)

@SimonSapin
Copy link
Contributor

I understand the desire to allow A's that aren't ZSTs, but it is IMO the job of those extending Box with that to figure out a way of doing so without breaking the guarantee of Box<T> being transparent.

The goal of using standard library containers with different allocators, including non-zero-size handles, is part of an accepted RFC that precedes the new transparency guarantee being proposed here: https://rust-lang.github.io/rfcs/1398-kinds-of-allocators.html#what-about-standard-library-containers

Ideally we would have Box<T, A> is repr(transparent) if and only if A is zero-size, but as far as I know there is no way to express that in the language today.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 25, 2019

@SimonSapin

Ideally we would have Box<T, A> is repr(transparent) if and only if A is zero-size, but as far as I know there is no way to express that in the language today.

This PR solves that: #62514

Once we stabilize Box<T, A> we just need to clarify that that only holds if A is zero-sized, and that's pretty much it AFAICT.

We plan to submit an RFC to guarantee this for all types for which this holds as part of the UCGs RFC. There is a PR in the UCG repo with proposed wording for that as well: rust-lang/unsafe-code-guidelines#164 (is blocked on some nit-picking on how to abbreviate one-aligned ZSTs but there is consensus that we want to guarantee that). So when that happens the documentation comment guaranteeing this wouldn't be technically necessary anymore, since it would be a given, but it would obviously be documentation worth having.

@jonas-schievink jonas-schievink added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 6, 2019
@crlf0710
Copy link
Member

I think #62514 is only adding comments, and there seems to be needing language level mechanism if we want to move forward.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 17, 2019

I think #62514 is only adding comments,

The comment there guarantees that box can be used in C FFI. Isn't that enough to resolve this?

@crlf0710
Copy link
Member

Isn't that enough to resolve this?

Is it? From FFI ABI perspective, adding those comments doesn't turn ABI for Box from representing a struct to representing a pointer, which is all #[repr(transparent)] all about, does it?

@CryZe
Copy link
Contributor

CryZe commented Aug 17, 2019

As long as it is guaranteed and there is some test verifying that it is the case, it doesn't need a special attribute.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 17, 2019

From FFI ABI perspective, adding those comments doesn't turn ABI for Box from representing a struct to representing a pointer, which is all #[repr(transparent)] all about, does it?

Box is as special as &mut T or i32, the compiler already sets the ABI of Box to that of a single pointer. The only thing the comment does is guarantee to users that this will never change (otherwise that would be a breaking change).

@crlf0710
Copy link
Member

ok, i guess that's the magic of lang_item then. thank you for the elaboration.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Dec 12, 2019
Clarify `Box<T>` representation and its use in FFI

This officializes what was only shown as a code example in [the unsafe code guidelines](https://rust-lang.github.io/unsafe-code-guidelines/layout/function-pointers.html?highlight=box#use) and follows [the discussion](rust-lang/unsafe-code-guidelines#157) in the corresponding repository.

It is also related to [the issue](rust-lang#52976) regarding marking `Box<T>` `#[repr(transparent)]`.

If the statement this PR adds is incorrect or a more in-depth discussion is warranted, I apologize. Should it be the case, the example in the unsafe code guidelines should be amended and some document should make it clear that it is not sound/supported.
@Mark-Simulacrum
Copy link
Member

We now have some extensive documentation about use of Box with FFI and generally the properties we guarantee: https://doc.rust-lang.org/nightly/std/boxed/index.html#memory-layout. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. 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

10 participants