Backing via [u8; N]

[wrenger]

Using [u8; N] as the backing store instead of integers has the following benefits.

1. Support more unconventional bitfield sizes (like 24bit) without unnecessary padding
2. Support for larger bitfields beyond 128bit
3. No forced alignment to the underlying integer type

Changing the API to something like this, as suggested in https://github.com/wrenger/bitfield-struct-rs/issues/6.

#[bitfield] // no explicit backing type specified
struct Foo {
    foo: u64,
    bar: u64,
    #[bits(2)]
    extra: u8,
    #[bits(6)]
    _reserved: u8 // must include explicit "padding" bits
}

static_assert_eq!(std::mem::size_of::<Foo>(), 8 + 8 + 1);

// no `From`/`Into` for any specific backing integer, but instead have a From/Into `[u8; N]`

The main difficulty is the more complex (and perhaps less efficient) code generation for the accessor functions. However, this might be abstracted into a single bit_copy function used by the accessors.

fn bit_copy(dst: &mut [u8], dst_off: usize, src: &[u8], src_off: usize, len: usize)

[wrenger]

I'm not happy with this bit_copy implementation yet. It has three major problems:

1. It is very complex and fragile due to the number of edge cases.
2. The generated code is far less optimal than the previous implementation.
3. It cannot be made const as long as const_mut_refs is missing (https://github.com/rust-lang/rust/issues/57349)

The first two might be improved by refactoring and optimizing them. The latter is not that easy. Keeping the accessors (<name>(), with_<name>()) const is quite important for me.

[wrenger]

Ok the generated and inlined code is better than expected: https://godbolt.org/z/4n3Mv6fK8

[daprilik]

Yeah, I would've been very surprised if the generated code ended up any worse than that. The lack of any/all panics is especially good to see!

3. It cannot be made const as long as const_mut_refs is missing

Well that's certainly unfortunate.

Here's an idea though: see if you can const-ify it without relying on helper functions (i.e: just emit the required code on a per accessor basis). If that works, you can start streamlining + optimizing that code.

e.g: by pre-computing which if-statements apply to which field, and manually emitting only the case you care about. e.g: by lifting out smaller chunks of shared utility into their own helper functions

Of course, having that feature be stable would be optimal... but I'm sure that with a bit of tasteful code contortions, it should be workable without it as well.

[wrenger]

Ok here is the first workaround, moves instead of mut refs.

The generated code, however, is less optimal. Even with optimizations, it copies a lot more. https://godbolt.org/z/ePG7Yzfve

[daniel5151]

I'll try to poke around this code a bit. Maybe I can spot some easy wins / optimizations.

In the meantime, here's a rewritten bit_print method that is nicer to use in godbolt, since it doesn't bring in any of the noisy Rust formatting machinery (making it easier to focus on the code we actually care about):

#[inline(never)]
fn bit_print(v: &[u8]) {
    fn putchar(c: u8) {
        extern "C" {
            fn putchar(c: u8);
        }
        unsafe { putchar(c) }
    }

    for b in v {
        let mut b = *b;
        for _ in 0..8 {
            putchar(if b & 0x80 == 0 { b'0' } else { b'1' });
            b <<= 1;
        }
        putchar(b' ');
    }
    putchar(b'\n');
}

[daniel5151]

I played around with it a bit, and it seems that you can get pretty dang close to identical by helping the Rust compiler reason about the moves. See https://godbolt.org/z/WTs8boW9f

In a nutshell: the following construct seems to iron out the codegen issues quite nicely:

// new "inner" function
fn set_flag_copy(x: [u8; 4], flag: bool) -> [u8; 4] {
    let src = [flag as u8];
    bit_copy(x, 3, &src, 0, 1)
}

// same API as what you had
fn set_flag(x: &mut [u8; 4], flag: bool) {
    *x = set_flag_copy(*x, flag)
}

Oh, and for posterity: here's a godbolt I was playing around with that compares (streamlined versions of) the non-cost and my new const versions against eachother: https://godbolt.org/z/oaWxT73Yz

[jstarks]

This looks cool. Is the thinking that specifying the backing would become optional, or would it be prohibited?

I'd prefer optional, where bitfield(u32) continues to enforce an alignment of 4 and a backing size of 32 bits, for example, and implicitly a transmutability to u32 for FFI. In my use of this crate, I have a lot of types that should really be naturally aligned u32/u64 values, and the existing attribute makes it really easy to see the underlying type without explicit align or const asserts.

[jstarks]

(Really I think I'd probably avoid the bare bitfield attribute altogether, since if I'm using bitfields, I almost always want to know, specify, and enforce the underlying backing type. Otherwise I imagine that I'll get the size or alignment wrong accidentally and pass an invalid *mut T across some FFI boundary.)

[daprilik]

I agree with @jstarks - when I proposed the bare #[bitfield] syntax, I didn't fully reason though the footgun implicit align(1) would have. Don't get me wrong - having align(1) bitfield structs would still be useful in certain scenarios, but I don't think it makes for the best default.

So, on that note: I would suggest disallowing the bare #[bitfield] syntax, and sticking to the existing syntax of specifying a backing type, while introducing a new bit of syntax for specifying arbitrary [u8; N]-backed bitfield structs (with implicit align(1))

(and of course: keeping the existing syntax would be a good move regardless, in terms of making it easy to upgrade between versions)

---

The only question becomes... what syntax to use for exotically-sized bitfields?

Maybe something like #[bitfield(N)]? Though, #[bitfield(8)] is awfully close to #[bitfield(u8)], which could lead to some annoying typo errors...

Maybe something more explicit like #[bitfield(size = N)] might be better? Though, with syntax highlighting, 8 and u8 would look quite different, so maybe it's not too bad...

Anyways, this is an area ripe for bikeshedding, and ultimately, I'm not too concerned with the specific syntax - just that it is opt-in, rather than implicit (as initially suggested)

EDIT: oh, actually, I don't think the 8 vs. u8 thing would be that big of an issue, since the macro should just error out due to size mismatch when summing up the individual size of the fields. In that case, I have a soft preference for the more concise syntax 😋

[wrenger]

In general, I want to keep this library as simple as possible. Having two entirely different code generation methods, depending on whether the underlying type is an int or a byte array, is not ideal. I would instead fully commit to one of the two: either only integers or byte arrays. The latter probably with a backward compatible way of defining the bitfield's size and alignment using integers (#[bitfield(u32)]).

The decision of which backing type to use is not easy. Integers are limited but very simple, which is important for efficient code generation and proc macros, which often come with compile-time overheads. The byte array backing is more flexible but also quite complex, and const functions are currently so limited that code generation and readability suffer.

[wrenger]

I'd prefer optional, where bitfield(u32) continues to enforce an alignment of 4 and a backing size of 32 bits, for example, and implicitly a transmutability to u32 for FFI. In my use of this crate, I have a lot of types that should really be naturally aligned u32/u64 values, and the existing attribute makes it really easy to see the underlying type without explicit align or const asserts.

This is also the case for me. I primarily use this bitfield crate for x86 register abstractions (which are usually aligned accordingly) and packing data into atomic data types (like AtomicU64 with from/into). So keeping this common case as the default makes sense.

Don't get me wrong - having align(1) bitfield structs would still be useful in certain scenarios, but I don't think it makes for the best default.

Maybe in these cases, #[repr(packed)] that enforces align=1, might be sufficient. Alternatively, a wrapper could be used that copies the bytes internally from a byte array into the bitfields integer. Also, if sizes other than 1, 2, 4, 8, or 16 bytes are needed, one could fallback on two or more bitfield types stored in a packed struct.

[daprilik]

A few thoughts:

• I totally agree that you should pick one repr backend and stick with it - having two different codegen backends seems like a bad idea from a maintainability + feature parity standpoint
• It would be interesting to see the codegen of this new array-backed approach vs. the old integer-backed approach in a non-godbolt context. Any chance you could use something like cargo asm to smoke test the end-to-end proc-macro generated code?
• I would shy away from proclaiming #[repr(packed)] as the "blessed" way to do these sorts of things:
  • In my experience, #[repr(packed)] is a real ergonomic pain-point, as it makes working with references to packed fields incredibly annoying (in the sense that you can't just do stuff like packed.field.method_taking_ref_self(), due to alignment issues). Maybe it's not as bad here (since things are all Copy), but it's worth exploring.
  • Manually decomposing an non 2^n sized bitfield into a set of ad-hoc smaller bitfield structs that get smooshed together into a single #[repr(packed)] field doesn't seem fun (and would mean that field accesses look like packed.inner0.field(), unless there's some hand-rolled getter/setter delegates on the outer struct itself)

Of course, this is your lib, and you're totally free to make whatever complexity vs. featureset vs. codegen tradeoffs you think are best, so if you think this feature falls on the wrong side of those tradeoffs, that's totally reasonable - i'll just have consider some alternatives.

That being said, as someone who's been hacking on emulators and OS code for a while now, I've found that exotically sized bitfield structures - while rare - certainly do come up, and having a single ergonomic and featureful Rust crate to reach for when working with bitfield would be incredibly helpful 😉

[wrenger]

Yes I agree that this general solution would be very cool. I'm just a little bit unsure if I'm able to make the bit copy function as efficient as the current implementation. It seems to me like a more complicated memcpy, which in itself is highly optimized for different architectures.

However, I'm willing to implement the code generation with the current bit copy and we can decide then if we are happy with it's performance.

[wrenger]

After being quite occupied the last few days, I was finally able to implement the code generation for the new bit_copy.

The bitfield attribute has changed a little:

Integer bitfields start with the ty argument followed by an integer type. The size and alignment are copied from the integer type but can be overwritten. The ty argument can be omitted if you want to specify the size and alignment of the bitfield manually.

For example: #[bitfield(ty = u64)].

The other arguments for this macro are:

• bytes: The byte size of the bitfield (default: 1)
• align: The alignment of the bitfield (default: 1)
• debug: Whether or not the fmt::Debug trait should be generated (default: true)

It seems functional so far, but I didn't benchmark it yet. What do you think?

[daniel5151]

In terms of feature-set, I think you've nailed it! AFAICT, this implementation ticks all the boxes I wanted when initially filing this feature request 🎉

The only remaining questions to me are around the API itself, and how we might refine it:

• I strongly prefer the existing bitfield(uX) syntax over the more explicit bitfield(ty = uX) syntax
  • As discussed earlier in the thread, it seems likely that most users of bitfield-struct (including yourself, and indeed, myself) will primarily be modelling integer-backed bitfields, so making that syntax more verbose feels like a step backwards in usability
• What are the precise semantics of using the bytes argument vs. specifying a [u8; X] backing?
• Just to double check (without looking too closely at the proc-macro impl itself): the default align should be alignof(ty), not 1... right?
• I don't think we should allow the bare #[bitfield] syntax, for those aforementioned foot-gun issues
  • i.e: in true Rust fashion, the API should make sure users into the pit of success, which in this case, means specifying a backing type (and therefore getting proper alignment)
  • The fact that the type is backed by an array internally should be just that - an internal implementation detail, quietly serving its purpose

Those are some of my initial thoughts after perusing the code. I'll try to find some time to clone down and play with the code in a more hands-on fashion as well, and see if I can glean any more insights there.

Oh, and thanks again for taking a crack at this! It's starting to shape up really nicely, and I'm super excited to refactor some code using this new functionality 😄

[wrenger]

• I strongly prefer the existing bitfield(uX) syntax over the more explicit bitfield(ty = uX) syntax
  • As discussed earlier in the thread, it seems likely that most users of bitfield-struct (including yourself, and indeed, myself) will primarily be modelling integer-backed bitfields, so making that syntax more verbose feels like a step backwards in usability

It was easier to parse, but using bitfield(uX) and bitfield([u8; N]) should also be possible. In any case, we need a nice semantic for defining byte size and alignment (preferably with backward compatibility).

• Just to double check (without looking too closely at the proc-macro impl itself): the default align should be alignof(ty), not 1... right?

Specifying ty copies alignment (and size) from it. If ty and align are both not specified, it defaults to 1.

• I don't think we should allow the bare #[bitfield] syntax, for those aforementioned foot-gun issues
  • i.e: in true Rust fashion, the API should make sure users into the pit of success, which in this case, means specifying a backing type (and therefore getting proper alignment)

Using #[bitfield] is equivalent to #[bitfield(bytes = 1, align = 1)]. The macro still checks that the members occupy exactly 8 bits. But yes, enforcing an explicit size (via bytes, ty, or the other syntax) might be a good idea.

• The fact that the type is backed by an array internally should be just that - an internal implementation detail, quietly serving its purpose

Yes, that was part of the idea for trying a different semantic other than bitfield([u8; N]).

Maybe we should support specifying either bitfield(uX) or bitfield(bytes = N) with the latter having an optional align parameter (defaulting to 1). The former just uses the alignment from the integer.

[daprilik]

But yes, enforcing an explicit size (via bytes, ty, or the other syntax) might be a good idea.

Indeed. Allowing bare #[bitfield] with align=1, bytes=1 semantics seem like the wrong default to promote, so I think it's best to deny it entirely.

Maybe we should support specifying either bitfield(uX) or bitfield(bytes = N) with the latter having an optional align parameter (defaulting to 1). The former just uses the alignment from the integer.

Well... I think there's still merit in allowing both ty and align = 1, since getting the From<ty> and Into<ty> impls seems useful.

Maybe you could consider something like the following?

// scenario 1 (most common, same as today)
#[bitfield(u32)] // with no support for additional attrs
// scenario 2 (advanced)
#[bitfield(ty = u32, align = 1)] // all key = value pairs
// ...or
#[bitfield(bytes = 5, align = 2)] 

[wrenger]

Well... I think there's still merit in allowing both ty and align = 1, since getting the From and Into impls seems useful.

Yes, indeed.

Maybe you could consider something like the following?

// scenario 1 (most common, same as today)
#[bitfield(u32)] // with no support for additional attrs
// scenario 2 (advanced)
#[bitfield(ty = u32, align = 1)] // all key = value pairs
// ...or
#[bitfield(bytes = 5, align = 2)] 

We also have the debug argument, which is already supported (#[bitfield(u32, debug=false)]). Scenario 1 would also have to support this optional argument, to keep the current functionality. This means that both 1 and 2 have optional arguments, but 1 does not accept all of them...

So, in that case, only two versions seem a lot easier.

#[bitfield(u32, align = 1, debug = false)]
#[bitfield(bytes = 3, align = 2, debug = false)]

PS: Sorry for the late answer.

[daprilik]

Ahh, that's right, you already support debug... Okay, yeah, fair enough! In that case, those two versions you proposed look great to me!

i.e: require either ty or bytes = N, and then zero or more additional optional attrs.

---

And no need to apologize! I'm just happy to see this PR continue to chug along at a reasonably steady pace.

It'll land when it lands :)

[bowdie]

Thanks for working on [u8; N] support - this will really help with embedded projects. I've been testing this branch and it's working well in a std environment, but when I move it to no_std environment on ESP32 it won't build.

I believe it's due to the removal of the this from Cargo.toml

[lib]
proc-macro = true

Are there plans to make it no_std compatible again?

[wrenger]

no_std is definitely a priority for this crate. However, this branch still tests implementations and might contain bugs/oversights in addition to this one.

[heroin-moose]

Do you have an intention to merge this at some point? Really need this one as my struct is 48 bytes. Aside from that this crate looks perfect for my case :(

[wrenger]

@heroin-moose, I would love to, but I'm currently not happy with the design and the (in certain cases) non-optimal code generation. There are also certain nightly features like const mut refs that would make this a lot cleaner. Unfortunately, they are not ready yet.

Also, I currently don't have much time for further experimentation, which is why this PR didn't got much attention lately.

Edit: If you have any ideas for improvements, feel free to contribute them ;)

[HaoboGu]

Any updates?

[wrenger]

Not really, I'm afraid.

[Frostie314159]

I'm probably a bit late to this discussion, but we should probably use an integer type wherever possible and otherwise fallback to an array. Apart from that, we could check if the field we're accessing is byte aligned and the size a power of two and generate code depending on that. I'm afraid, we won't be able to generate more optimal code. In the end, we're probably building something that's close to the code, which calculates the layout for structs in LLVM.

[JohnstonJ]

Would love to see this. I'm writing some code to parse fixed-length 80-byte chunks of data from a legacy binary data file format that will involve use of:

• Individual fields require use of bit fields (thus the interest in this package)
• Unions
• Nested structures / fixed-length arrays of nested structures
• Nested structures with bit fields, where the nested structure is not of integer size (i.e. 3 bytes)

On and on.

This package is almost perfect for me, except the constraint restricting me to only built-in integer types makes it almost totally useless, I think... (I still have to get into it in more depth, but scoping this out in advance is not looking promising at first glance.)