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

Use MaybeUninit<> for generated padding where available. #1606

Open
emilio opened this issue Aug 2, 2019 · 6 comments
Open

Use MaybeUninit<> for generated padding where available. #1606

emilio opened this issue Aug 2, 2019 · 6 comments

Comments

@emilio
Copy link
Contributor

emilio commented Aug 2, 2019

When the rust-target supports MaybeUninit<>, we should technically use that when we inject padding into a struct.

This should be straight-forward and I'm happy to mentor this. See rust-lang/libc#1453.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2019

Note that rust-lang/libc#1453 mentions that, when the rust-target supports MaybeUninit<>, there is probably only a single case for which padding might need to be inserted into a struct, and that case is when the struct has a flexible-array member as its last element. I have no idea how to do FFI from Rust with C libraries whose types use flexible-array members, does rust-bindgen support this? If not, then I don't know of any case in which MaybeUninit would ever be necessary, at least when interfacing with C.

When interfacing with C++, the only case I can imagine in which MaybeUninit might be necessary is when C++ types contain "empty" types like struct A { };. These types are, by default, 1 byte wide (*) and should probably be mapped to Rust types that are also 1 byte wide. When interfacing with C++, these bytes are probably uninitialized, so MaybeUninit<u8> might be required for those.

(*) C++ does have attributes to make struct fields of empty types zero-sized, and other attributes like alignment can actually raise their size, so if those are present the fields might not be 1 byte wide.

@emilio
Copy link
Contributor Author

emilio commented Aug 2, 2019

Yes, bindgen supports both flexible array members and empty C++ structs / classes.

It also supports bitfields, but it's not clear to me what the right way to handle them is, if you have eight bits and only the top one is guaranteed to be initialized, the way we implement bitfields means that you still need to load the whole byte in memory, even if you mask the potentially uninitialized bits away.

@emilio
Copy link
Contributor Author

emilio commented Aug 2, 2019

Though I think we represent flexible array members as wrapper over zero-sized arrays, so I think it should be ok.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2019

It also supports bitfields, but it's not clear to me what the right way to handle them is, if you have eight bits and only the top one is guaranteed to be initialized, the way we implement bitfields means that you still need to load the whole byte in memory, even if you mask the potentially uninitialized bits away.

I think that, for bit fields, rust-bindgen would need to use something like NonZeroU8. For example, given this C++ struct:

struct S {
    int32_t b : 3;
};

The corresponding Rust type (using rustc private attributes) is:

#[rustc_layout_scalar_valid_range_start(0)]
#[rustc_layout_scalar_valid_range_end(8)]
struct Bitfield(u32); 

struct S {
    x: Bitfield
}

where Bitfield only allows values in range [0, 7].

This problem is already solved, but there are no proposals to expose those solutions to Rust users. cc @eddyb @joshtriplett

If I could use a hammer, I'd just expose a single lang item in libcore:

#[lang_item = "int_in_range"]
struct IntInRange<T: {i,u}{8,16,32,64,128,size}, const From: T, const To: T>(T);

That only supports integers as T and sets From: T and To: T as the valid range for IntInRange.

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2019

I have no idea how to map general C++ bitfields to Rust:

struct S {
    // will usually occupy 2 bytes:
    // 3 bits: value of b1
    // 2 bits: unused
    // 6 bits: value of b2
    // 2 bits: value of b3
    // 3 bits: unused
    uint8_t b1 : 3, : 2, b2 : 6, b3 : 2;
};

We can't use IntInRange to expose those, because the fields b1, b2, and b3 are part of the same int, and some parts of the int have holes inside, so a simple "start"-"end" solution like the one used for NonZero does not work there.

@eddyb
Copy link
Member

eddyb commented Aug 9, 2019

@gnzlbg Regarding the validity range thing, the plan has been to use const generics once they're available for this purpose, I guess that could be now?

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

No branches or pull requests

3 participants