| Message ID | 20230118142652.53f20593504b.Iaeea0aee77a6493d70e573b4aa55c91c00e01e4b@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | bitfield: add FIELD_PREP_CONST() | expand |
From: Johannes Berg <johannes@sipsolutions.net> Date: Wed, 18 Jan 2023 14:26:53 +0100 > From: Johannes Berg <johannes.berg@intel.com> > > Neither FIELD_PREP() nor *_encode_bits() can be used > in constant contexts (such as initializers), but we > don't want to define shift constants for all masks > just for use in initializers, and having checks that > the values fit is also useful. > > Therefore, add FIELD_PREP_CONST() which is a smaller > version of FIELD_PREP() that can only take constant > arguments and has less friendly (but not less strict) > error checks, and expands to a constant value. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > include/linux/bitfield.h | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index c9be1657f03d..ebfa12f69501 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -115,6 +115,32 @@ > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > }) > > +#define __BF_CHECK_POW2(n) BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0) > + > +/** > + * FIELD_PREP_CONST() - prepare a constant bitfield element > + * @_mask: shifted mask defining the field's length and position > + * @_val: value to put in the field > + * > + * FIELD_PREP_CONST() masks and shifts up the value. The result should > + * be combined with other fields of the bitfield using logical OR. > + * > + * Unlike FIELD_PREP() this is a constant expression and can therefore > + * be used in initializers. Error checking is less comfortable for this > + * version, and non-constant masks cannot be used. > + */ > +#define FIELD_PREP_CONST(_mask, _val) \ Have you tried combining it with FIELD_PREP() using __builtin_choose_expr() + __builtin_is_constexpr() (or __builtin_constant_p() depending on which will satisfy the compiler)? I'm not saying it's 100% possible, but worth trying. > + ( \ > + /* mask must be non-zero */ \ > + BUILD_BUG_ON_ZERO((_mask) == 0) + \ > + /* check if value fits */ \ > + BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \ > + /* check if mask is contiguous */ \ > + __BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) + \ > + /* and create the value */ \ > + (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)) \ > + ) > + > /** > * FIELD_GET() - extract a bitfield element > * @_mask: shifted mask defining the field's length and position Thanks, Olek
On Wed, 2023-01-18 at 17:10 +0100, Alexander Lobakin wrote: > > Have you tried combining it with FIELD_PREP() using > __builtin_choose_expr() + __builtin_is_constexpr() (or > __builtin_constant_p() depending on which will satisfy the compiler)? > I'm not saying it's 100% possible, but worth trying. > I haven't tried it that way, but I tried rewriting FIELD_PREP() itself to be constant-compatible, and as soon as the compiler saw __builtin_constant_p() in the initializer it already complained that it was non-constant... I didn't think of __builtin_choose_expr, but it doesn't work either because it only promises that the unused expression is not *evaluated*, not that it's not "looked at", so it still complains both ways (in the constant case that you can't have ({ }) braced groups, and in the non- constant case that the _CONST version is bad... johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: Wed, 18 Jan 2023 17:22:30 +0100 > On Wed, 2023-01-18 at 17:10 +0100, Alexander Lobakin wrote: >> >> Have you tried combining it with FIELD_PREP() using >> __builtin_choose_expr() + __builtin_is_constexpr() (or >> __builtin_constant_p() depending on which will satisfy the compiler)? >> I'm not saying it's 100% possible, but worth trying. >> > > I haven't tried it that way, but I tried rewriting FIELD_PREP() itself > to be constant-compatible, and as soon as the compiler saw > __builtin_constant_p() in the initializer it already complained that it > was non-constant... > > I didn't think of __builtin_choose_expr, but it doesn't work either > because it only promises that the unused expression is not *evaluated*, > not that it's not "looked at", so it still complains both ways (in the > constant case that you can't have ({ }) braced groups, and in the non- > constant case that the _CONST version is bad... Aaaah right. Seems like we can't avoid introducing a separate macro for that. I like the idea of your patch anyways! One note re __BF_CHECK_POW2(): can't we reuse is_power_of_2() anyhow? Foe example, by deriving the code of the latter into a macro and then using it in both? > > johannes Thanks, Olek
On Wed, 2023-01-18 at 17:49 +0100, Alexander Lobakin wrote: > > Aaaah right. Seems like we can't avoid introducing a separate macro for > that. I like the idea of your patch anyways! :) > One note re __BF_CHECK_POW2(): can't we reuse is_power_of_2() anyhow? > Foe example, by deriving the code of the latter into a macro and then > using it in both? > Well, not directly - for example is_power_of_2() doesn't accept 0, while we want to accept 0 (mask being e.g. "0xfull<<60", we already check for mask != 0). I thought about __BUILD_BUG_ON_NOT_POWER_OF_2 but it uses BUILD_BUG_ON, not BUILD_BUG_ON_ZERO, and BUILD_BUG_ON is nicer in most contexts ... So you could pull out the expression "((n) & ((n) - 1)) != 0" from all four of these, but it doesn't feel worth it. johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: Wed, 18 Jan 2023 19:03:24 +0100 > On Wed, 2023-01-18 at 17:49 +0100, Alexander Lobakin wrote: >> >> Aaaah right. Seems like we can't avoid introducing a separate macro for >> that. I like the idea of your patch anyways! > > :) > >> One note re __BF_CHECK_POW2(): can't we reuse is_power_of_2() anyhow? >> Foe example, by deriving the code of the latter into a macro and then >> using it in both? >> > > Well, not directly - for example is_power_of_2() doesn't accept 0, while > we want to accept 0 (mask being e.g. "0xfull<<60", we already check for > mask != 0). > > I thought about __BUILD_BUG_ON_NOT_POWER_OF_2 but it uses BUILD_BUG_ON, > not BUILD_BUG_ON_ZERO, and BUILD_BUG_ON is nicer in most contexts ... > > So you could pull out the expression "((n) & ((n) - 1)) != 0" from all > four of these, but it doesn't feel worth it. Aaaah I see. __BUILD_BUG_ON_NOT_POWER_OF_2_ZERO() then? :D (mostly joking and not sure it's worth it, up to you) > > johannes Thanks, Olek
On Wed, 2023-01-18 at 19:21 +0100, Alexander Lobakin wrote: > > So you could pull out the expression "((n) & ((n) - 1)) != 0" from all > > four of these, but it doesn't feel worth it. > > Aaaah I see. > > __BUILD_BUG_ON_NOT_POWER_OF_2_ZERO() then? :D > > (mostly joking and not sure it's worth it, up to you) > Yeah exactly! I briefly considered but didn't really want to touch build_bug.h for basically nothing :) johannes
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index c9be1657f03d..ebfa12f69501 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -115,6 +115,32 @@ ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ }) +#define __BF_CHECK_POW2(n) BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0) + +/** + * FIELD_PREP_CONST() - prepare a constant bitfield element + * @_mask: shifted mask defining the field's length and position + * @_val: value to put in the field + * + * FIELD_PREP_CONST() masks and shifts up the value. The result should + * be combined with other fields of the bitfield using logical OR. + * + * Unlike FIELD_PREP() this is a constant expression and can therefore + * be used in initializers. Error checking is less comfortable for this + * version, and non-constant masks cannot be used. + */ +#define FIELD_PREP_CONST(_mask, _val) \ + ( \ + /* mask must be non-zero */ \ + BUILD_BUG_ON_ZERO((_mask) == 0) + \ + /* check if value fits */ \ + BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \ + /* check if mask is contiguous */ \ + __BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) + \ + /* and create the value */ \ + (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)) \ + ) + /** * FIELD_GET() - extract a bitfield element * @_mask: shifted mask defining the field's length and position