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

Force #[repr(C)] layout to guarantee same offset of union fields. #268

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

anforowicz
Copy link
Contributor

https://rust-lang.github.io/unsafe-code-guidelines/layout/unions.html points out that

> [...] the default layout of Rust unions is, in general,
> unspecified.
>
> That is, there are no general guarantees about the offset of the
> fields, whether all fields have the same offset, what the call ABI
> of the union is, etc.

This commit explicitly asks for #[repr(C)] layout to guarantee that both fields have the same offeset.

https://rust-lang.github.io/unsafe-code-guidelines/layout/unions.html
points out that

    > [...] the default layout of Rust unions is, in general,
    > unspecified.
    >
    > That is, there are no general guarantees about the offset of the
    > fields, whether all fields have the same offset, what the call ABI
    > of the union is, etc.

This commit explicitly asks for `#[repr(C)]` layout to guarantee
that both fields have the same offeset.
@anforowicz
Copy link
Contributor Author

@Lokathor
Copy link
Owner

The offset of the field doesn't matter in this case because we're not accessing the field via any sort of pointer offset math, but instead by using the standard field access syntax.

That said, this doesn't hurt, so whatever.

@Lokathor Lokathor merged commit 3f42bec into Lokathor:main Aug 27, 2024
14 checks passed
@Lokathor Lokathor added the semver-patch semver patch change label Aug 27, 2024
@Lokathor
Copy link
Owner

released in bytemuck-1.17.1

@anforowicz anforowicz deleted the union-repr-c branch August 27, 2024 18:58
@anforowicz
Copy link
Contributor Author

released in bytemuck-1.17.1

Thanks! Much appreciated.

The offset of the field doesn't matter in this case because we're not accessing the field via any sort of pointer offset math, but instead by using the standard field access syntax.

If the offsets were different, then I don't think it would matter that no pointer offset math was used. I think that this would be equivalent to expecting that transmuting can work through something like:

// `A` and `B` are `AnyBitPattern` and `NoUninit`
type A = u32;  
type B = i32;

// The compiler can _theorerically_ insert some `_padding` bytes before `A` or `B`.
// (Since there are no guarantees for the default non-`#[repr(C)]` layout.)
#[repr(C)]
struct AWithPadding {
    _padding: [u8; 4],  // 4 bytes of padding
    a: A,
}
#[repr(C)]
struct BWithPadding {
    _padding: [u8; 8],  // Different padding - 8 bytes
    b: B,
}

// Now `u.a_with_padding.a` and `u.b_with_padding.b` don't use pointer offset math,
// but `A` and `B` don't "overlap" in `ABUnion` and therefore the `union` can't be used for transmuting.
#[repr(C)]
union ABUnion {
    a_with_padding: AWithPadding,
    b_with_padding: BWithPadding,
}

That said, this doesn't hurt, so whatever.

Ack. I think that in practice there will be no padding (since A and B have the same size), but since there are no guarantees, it's probably best to explicitly request offset 0 by using #[repr(C)].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch semver patch change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants