search

Lokathor / bytemuck

A crate for mucking around with piles of bytes
https://docs.rs/bytemuck
Apache License 2.0
725 stars 79 forks source link

Crate makes assumptions about bool values #210

Closed Manishearth closed 1 year ago

Manishearth commented 1 year ago

https://github.com/Lokathor/bytemuck/blob/d10fbfc6ffb9a2adf2faa001cd614bd7dfe20f02/src/contiguous.rs#L179-L180

This assumes that a bool takes values 0=false, 1=true, which is true on all platform Rust currently supports but is not necessarily globally true, see https://github.com/rust-lang/unsafe-code-guidelines/issues/53.

This is quite minor but I did notice this crate goes out of its way to handle spirv correctly so it does seem worth at least documenting in code and perhaps adding additional runtime-optimizeable asserts for.

Lokathor commented 1 year ago

https://doc.rust-lang.org/reference/types/boolean.html

The rust reference documents it as being size 1 align 1, and also the exact but patterns of false and true.

I think even if the size and alignment were bigger on a new platform (eg: 4 each perhaps, like a u32 instead of like a u8), then bool would still have to use the bit patterns for 0 or 1 just because so much unsafe code everywhere assumes such by now.

I'm open to bytemuck having additional static assertions or something like that if you'd like to add them somewhere.

Manishearth commented 1 year ago

Aha, good point. Might be worth linking to that spec, but i assumed from skimming that issue that this was still an open issue.

Yeah, I'm aware there's almost no chance rust will change this, this just cropped up during unsafe code review :smile:

Lokathor commented 1 year ago

@MaulingMonkey informed me that on some obscure platforms it really might not be s1a1, in the example he knew about it was OSX for PPC cpus. Not sure if we need to worry or not in the long run.

MaulingMonkey commented 1 year ago

I'll mention abibool in passing for those who need interop with booleanish values which might not match Rust's ≈ #[repr(u8)] enum bool { false=0, true=1 } ABI. That includes e.g. most C ABIs, which typedef an integer type for booleans for C89 interop, and can easily be passed nonzero values other than 1 which should probably be interpreted as truthy instead of immediate undefined behavior. Even if bytemuck tried to relax it's assumptions about bool, rustc would still make assumptions about bool, making it no more useful for interop with C or C++ code.

Lokathor commented 1 year ago

https://docs.rs/bool32/latest/bool32/ i made a bool thing for interacting with vulkan and sdl2