From: Johannes Berg <[email protected]>
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 <[email protected]>
---
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) \
+ ( \
+ /* 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
--
2.39.0
From: Johannes Berg <[email protected]>
Date: Wed, 18 Jan 2023 14:26:53 +0100
> From: Johannes Berg <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
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