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

Constify bitfield constructor for Rust 1.57+ #2314

Closed

Conversation

hugwijst
Copy link

@hugwijst hugwijst commented Oct 19, 2022

Allows creating structs with bitfields in a const context, for example:

static FIRST: bindings::bitfields::First = bindings::bitfields::First {
    _bitfield_align_1: [],
    _bitfield_1: bindings::bitfields::First::new_bitfield_1(1, 2, 3),
};

This can be useful when interacting with existing C libraries in a bare-metal environment, as these sometimes expect global structs with specific symbols to exist.

The current workaround is to create a mutable struct with the bitfield assigned to something like __BindgenBitfieldUnit::new([0; 2]), and then call the appropriate field setters before the struct gets used. This has multiple downsides:

  1. It requires hard-coding the size of the storage array of __BindgenBitfieldUnit.
  2. It requires the struct to be mutable, which would require the use of unsafe if the struct also has to be static.
  3. It increases the code size.

@hugwijst hugwijst force-pushed the const_bitfield_constructor branch 2 times, most recently from ddb2f4b to 027bf68 Compare October 20, 2022 04:45
@@ -236,6 +239,9 @@ rust_feature_def!(
Stable_1_47 {
=> larger_arrays;
}
Stable_1_57 {
=> const_panic;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be lowered to 1.56 (gated by constification of core::mem::transmute) if we are okay with removing the debug_asserts! in the generated code.

@hugwijst
Copy link
Author

r? @fitzgen

@kulp
Copy link
Member

kulp commented Oct 22, 2022

@hugwijst I like const fns, but can you explain the motivation? Without a PR description I wonder how to evaluate whether this is worth the added complexity.

@rockboynton
Copy link

@hugwijst I like const fns, but can you explain the motivation? Without a PR description I wonder how to evaluate whether this is worth the added complexity.

@kulp, I can provide an example. A lot of C APIs require there to be global (mutable) struct variables and these structures may have bitfields.

In order to mimic this in Rust, we need to use static mut VAR: Type = Type {}. But since bitfields don't have a const constructor, that definition would need some dummy values, and use a bunch of setter functions later in the code.

This change lets the entire structure to be defined initially, as it is in C.

Hopefully that provides some context 🙂

@hugwijst
Copy link
Author

@hugwijst I like const fns, but can you explain the motivation? Without a PR description I wonder how to evaluate whether this is worth the added complexity.

I've updated the PR description, happy to discuss alternatives too.

@rockboynton
Copy link

bump

@kulp
Copy link
Member

kulp commented Dec 12, 2022

Thanks @rockboynton for more explanation. I am sorry for the long delays.

It feels like a lot of code changes to me for what it yields, but I will not pretend I am equipped to weigh the trade-offs here.

Maybe @pvdrz or @amanjeev or @emilio will have some wisdom.

@pvdrz
Copy link
Contributor

pvdrz commented Dec 12, 2022

👋 I think this is a nice addition!

I'm wondering if there's a way to avoid having both set_bit_const and set_bit available at the same time as it would be a bit confusing having two functions that do basically the same. Sadly, we cannot make the original set_bit a const fn because it uses &mut self and it indexes a slice and not an array from what I see. We could do two changes:

  1. We could change set_bit to do things by-value instead of by-ref and this would make both set_bit and set_bit_const interchangeable almost everywhere. This would be a breaking change so we would need to put that into account.

  2. Another change we could do is change the bitfield struct definition so it uses [u8; N] instead of the generic Storage. But this would break code generation for any rust target that doesn't have const generics.

I would suggest to do change 1 and then do change 2 behind version 1.57 so most people can transition from the non-const version to the const version if they update the rust version. Take this suggestion with a grain of salt as I don't have the last word on this matter 😅

I would merge this without this suggestion anyway.

@krukai
Copy link

krukai commented Apr 13, 2023

Any update on this? We are actually relying on @hugwijst's fork right now and it would be great to see it merged.

@bors-servo
Copy link

☔ The latest upstream changes (presumably 2034a0f) made this pull request unmergeable. Please resolve the merge conflicts.

@pvdrz pvdrz deleted the branch rust-lang:master November 2, 2023 17:50
@pvdrz pvdrz closed this Nov 2, 2023
@kulp
Copy link
Member

kulp commented Nov 4, 2023

FYI, the current PR's closure may or may not have been intended. @pvdrz can comment.

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

Successfully merging this pull request may close these issues.

None yet

6 participants