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

Layout of repr(C) unions has padding #156

Closed
gnzlbg opened this issue Jul 3, 2019 · 17 comments
Closed

Layout of repr(C) unions has padding #156

gnzlbg opened this issue Jul 3, 2019 · 17 comments
Labels
A-padding Topic: Related to padding A-unions Topic: Related to unions C-open-question Category: An open question that we should revisit S-pending-design Status: Resolving this issue requires addressing some open design questions

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 3, 2019

In this thread and the comments that follow, the following new information was discovered.

repr(C) unions have padding bits of the form:

  • trailing padding: if the union size is larger than the size of its largest field, all union_size - largest_field_size trailing bits are padding bits.
  • interior padding: if the bit i of all union fields is a padding bit, the bit i of the inion is a padding bit

Note: because all repr(C) union fields are at offset 0, there is no padding before any field.

The content of padding bits of repr(C) unions is always uninitialized. That is, they are not required to be preserved on copy / move / pass by value, etc. The implementation of the call ABI can exploit this knowledge.

For example, Rust, clang, and GCC all implement the SysV64 ABI, and when passing a #[repr(C)] union U { x: (u8, u32) } around by value, @eddyb mentioned that Rust and GCC pass the bottom 32-bits (where the u8 is stored) while clang passes the bottom 8-bits. Both implementations are allowed. @comex also mentioned that in some ABIs like RISC-V ELF "appears to require callers to zero- or sign-extend
arguments in registers in a particular way. In other words, it requires the upper bits (which correspond to padding bytes) to have a specific value, and the callee can assume that they do have that value". That would be incompatible with allowing users to use the padding bits.

That is, repr(C) unions are not and cannot be just "bags of bits" where one could write to any bit, and that bit value would need to be preserved on copy / move / pass-by-value.

We should document this for repr(C) unions in the Unsafe Code Guidelines, so I'm re-opening this issue until that is resolved.

@gnzlbg gnzlbg added the A-layout Topic: Related to data structure layout (`#[repr]`) label Jul 3, 2019
@BurntSushi
Copy link
Member

I think you meant to ping @comex here. :-) (Not comes.)

@RalfJung
Copy link
Member

That is, repr(C) unions are not and cannot be just "bags of bits" where one could write to any bit, and that bit value would need to be preserved on copy / move / pass-by-value.

I do not understand why this is a consequence. Your summary (and thanks for the writeup!) says that "Rust and GCC pass the bottom 32-bits". So it is possible to implement this in a way that preserves all bits. Correct?

Note: I am not concerned about bits getting preserved when Rust calls a C function and then C passes data back to Rust. At that point, C rules apply. But I am concerned about the case where Rust code calls an extern "C" function that is also implemented in Rust.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 16, 2019

I do not understand why this is a consequence. Your summary (and thanks for the writeup!) says that "Rust and GCC pass the bottom 32-bits". So it is possible to implement this in a way that preserves all bits. Correct?

So that's a good question. Consider this:

#[repr(C)] union U { x: (u8, u32 ) }
extern "C" {
      fn foo() -> U;
}

If the foo implementation is C code compiled with a C compiler, then the padding bytes of the returned U will probably be undef because the C compiler does not need to put anything initialized in there.

OTOH, if we were to somehow make Rust to always copy padding bytes for repr(C) unions, and enforce that in extern "C" fn foo() -> U { ... } function definitions, then those padding bytes could be initialized to something meaningful.

We can't tell whether the code at the other side is Rust or C, so if we want to be able to treat a repr(C) union as a bag of bytes here, we would always need to freeze the union when doing FFI.

I tend to think that an extern "C" declaration should be assumed to follow C rules (e.g. no panics), and that most people use them for interfacing with C, and that therefore we should aim to optimize according to those rules. In this particular case, it would mean that we don't have to copy padding when dealing with these unions, however minor this optimization is for this example, or even in general.

@RalfJung
Copy link
Member

My reading of rust-lang/rust#60405 (comment) is that it is impossible to achieve "raw bit copy" for extern "C" with a repr(C) union, but I might be misinterpreting that.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 16, 2019

@comex mentions there that Rust->Rust could be extended to support this.

@comex
Copy link

comex commented Jul 16, 2019

I said that it would be possible for some ABIs but not all.

@eddyb
Copy link
Member

eddyb commented Jul 17, 2019

@RalfJung If you have 8 bytes or more of padding you might not have where to put them.

When only some bytes of an u64 group (passed as one register) are padding, you can pass them alongside the non-padding bytes, but if they're all padding, they won't get a register at all.

@RalfJung RalfJung added C-open-question Category: An open question that we should revisit A-unions Topic: Related to unions A-padding Topic: Related to padding and removed A-layout Topic: Related to data structure layout (`#[repr]`) labels Aug 14, 2019
This was referenced Aug 16, 2019
@comes
Copy link

comes commented Jan 28, 2022

I think you meant to ping @comex here. :-) (Not comes.)

enjoy what you're doing guys and have a super great time while that.

comes

@RalfJung
Copy link
Member

In MiniRust I am now handling this by making unions "chunked": a union is not simply a bag of bytes as large as its size, there are multiple chunks with that size with possible gaps between them. The data within the chunks is preserved verbatim, data between the chunks is lost as padding.

Actually computing the chunks is left to the frontend. I hope we can make it so that repr(Rust) unions only ever have a single chunk that matches their full size, but this also lets us represent C unions that have "gaps" in them that are not being preserved. (We could have also encoded those as structs of unions. But that would probably just be unnecessarily confusing.)

@alercah
Copy link

alercah commented Jul 13, 2022

Apologies for the poorly organized long post.

I think @comex misread the RISC-V calling convention spec (which appears to have moved) slightly, but I don't think it really changes much:

Aggregates whose total size is no more than XLEN bits are passed in a register, with the fields laid out as though they were passed in memory. If no register is available, the aggregate is passed on the stack. Aggregates whose total size is no more than 2×XLEN bits are passed in a pair of registers; if only one register is available, the first XLEN bits are passed in a register and the remaining bits are passed on the stack. If no registers are available, the aggregate is passed on the stack. Bits unused due to padding, and bits past the end of an aggregate whose size in bits is not divisible by XLEN, are undefined.

So while I think it means there's no obligation to sign-extend when an integer followed by padding occupies an entire register, it is also extremely clear that padding is not preserved.

The ARM ABI, on the other hand, appears to me to require aggregates to simply be copied, which might imply preservation of padding, but description of the way that arguments are obtained actually clarifies this (section 8.2):

For C, each argument is formed from the value specified in the source code, except that an array is passed by passing the address of its first element.

So this means that the value is copied, meaning padding can take an unspecified value. Unlike what Apple claims, the caller does seem to be responsible for sign- or zero-extension in the latest platform ABI, at least for fundamental integral types. But specifically not for aggregate types. So I think the same logic applies and while the caller is not obligated to pass any particular padding values, it is not obligated to preserve them either.

But if we look at the C standard, it's actually even worse, I think. See section 6.2.6.1
(apologies if the numbers don't match up to the latest official version of the standard):

  1. When a value is stored in a member of an object of union type, the bytes of the object
    representation that do not correspond to that member but do correspond to other members
    take unspecified values.
  1. ... Where a value is stored in an object using a type that has more than one object representation for
    that value, it is unspecified which representation is used, but a trap representation shall
    not be generated.

and section 6.2.7.1:

  1. ... The value of at most one of the members can be stored in a union object at any time.

Thus we arrive at the classic "you can't use a union to cast" problem: the actual value space of a C union, in the most pedantic and hostile interpretation for the programmer, is a true sum type, and copying a union copies the value, not the object representation, which can clobber any bytes that are padding for that particular value. So the example of union U { (u8, u32), (u64) } isn't even guaranteed to preserve padding of the first variant—not even in functions taking an arbitrary U as an argument, because the variant might be known to the compiler through inlining or otherwise.

Combined with the particular language in the ARM ABI especially which clearly talks about passing the value, rather than the object representation, I think that a maximally defensive definition of #[repr(C)] union semantics would need to take this into account.

(C++, interestingly, is much more clear about the implicit sum type nature of a union, but also requires padding to be preserved if I read it correctly.)

tl;dr I'm not convinced that any ABI requires padding bytes to take specific values. But I think that, in C, which bytes of a union are padding are based on the last value properly stored into the union, which is not represented in memory at all.

@alercah
Copy link

alercah commented Jul 14, 2022

Actually, I am apparently wrong about C++, according to a note on this page:

For a union with bits that participate in the value representations of some members but not the others, compare-and-exchange might always fail because such padding bits have indeterminate values when they do not participate in the value representation of the active member.

@RalfJung
Copy link
Member

RalfJung commented Jul 14, 2022

But if we look at the C standard, it's actually even worse, I think.

I don't think those parts of the C standard are relevant for ABI discussions. ABIs are unavoidably defined on the assembly level.
The C standard was not intended to also specify ABIs (even though it is now heavily involved in that business), and large parts of it IMO make little sense for that purpose.

So I don't think that the actual ABI of a union is ever any worse than "copy this range of bytes but possibly leave some gaps where data gets lost". I have recently adjusted MiniRust to support such "chunked" unions, and I think this is good enough for the ABIs out there. Or do you know of a counterexample?

@RalfJung
Copy link
Member

So the example of union U { (u8, u32), (u64) } isn't even guaranteed to preserve padding of the first variant—not even in functions taking an arbitrary U as an argument, because the variant might be known to the compiler through inlining or otherwise.

This is true if that code is written in C. But I don't see how that has any consequence on Rust code calling Rust code via the C ABI -- and that is the case we are concerned about here.

Combined with the particular language in the ARM ABI especially which clearly talks about passing the value, rather than the object representation, I think that a maximally defensive definition of #[repr(C)] union semantics would need to take this into account.

repr(C) on a struct doesn't mean that all the value representation nonsense works exactly like it does in C. It means that we use the same layout algorithm as C. I think the same is true of unions. We don't promise "everything that is UB in C is also UB in Rust", that would be a terrible promise to make.

tl;dr I'm not convinced that any ABI requires padding bytes to take specific values. But I think that, in C, which bytes of a union are padding are based on the last value properly stored into the union, which is not represented in memory at all.

Yeah I could believe that. Lucky enough I think it has no relevance for Rust.

@alercah
Copy link

alercah commented Jul 15, 2022

I think, in the end, I agree with everything you said here.

alercah added a commit to alercah/unsafe-code-guidelines that referenced this issue Oct 8, 2022
I have tried to document everything I believe we have consensus on. I've
left some things open that I possibly could have closed, but because
this PR is very big, I would like to focus on getting it in as quickly
as possible and worrying about whatever's left aftwards.

I strongly encourage others to submit follow up PRs to close out the
other open issues.

Closes rust-lang#156.
Closes rust-lang#298.
Closes rust-lang#352.
alercah added a commit to alercah/unsafe-code-guidelines that referenced this issue Oct 8, 2022
I have tried to document everything I believe we have consensus on. I've
left some things open that I possibly could have closed, but because
this PR is very big, I would like to focus on getting it in as quickly
as possible and worrying about whatever's left aftwards.

I strongly encourage others to submit follow up PRs to close out the
other open issues.

Closes rust-lang#156.
Closes rust-lang#298.
Closes rust-lang#352.
alercah added a commit to alercah/unsafe-code-guidelines that referenced this issue Oct 15, 2022
This isn't intended to create controversy, but document where discussion
has settled. Please feel free to open more PRs to clear up additional
items.

Closes rust-lang#156.
alercah added a commit to alercah/unsafe-code-guidelines that referenced this issue Oct 30, 2022
This isn't intended to create controversy, but document where discussion
has settled. Please feel free to open more PRs to clear up additional
items.

Closes rust-lang#156.
@JakobDegen JakobDegen added the S-pending-design Status: Resolving this issue requires addressing some open design questions label Aug 1, 2023
@JakobDegen
Copy link
Contributor

It's not entirely clear if this question is just asking for documentation or for a resolution to the Rust level issue about what the semantics of #[repr(C)] unions are, but assuming it's the second

@RalfJung
Copy link
Member

RalfJung commented Aug 1, 2023

In MiniRust this issue is resolved by saying that the value of a union consists of several "chunks" of raw memory, that will have Uninit but in between them. There is logic here that computes these chunks for Rust union types, when translating MIR to MiniRust.

As far as I know, this is sufficient to encode the behavior of these repr(C) unions.

I think all that remains to be done here is somehow documenting this as our consensus solution to the problem. But of course this interacts with other questions around the value representation of unions, of which there are a lot. In particular, do we make union bytes "noundef" if they are "noundef" in each variant?

It might be worth opening a new issue that specifically tracks these questions.

@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2023

Closing in favor of #438.

@RalfJung RalfJung closed this as completed Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-padding Topic: Related to padding A-unions Topic: Related to unions C-open-question Category: An open question that we should revisit S-pending-design Status: Resolving this issue requires addressing some open design questions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants