lkml.org 
[lkml]   [2018]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: Question on FIELD_PREP() for static array
From
Date
On 10/10/2018 19:13, Johannes Berg wrote:
> On Wed, 2018-10-10 at 10:33 -0700, Joe Perches wrote:
>>
>>> Specifically it doesn't like the __BF_FIELD_CHECK() in FIELD_PREP().
>>>
>>> Any ideas on compiler trickery we could do with the FIELD_PREP()
>>> definition to avoid this issue (i.e. enforce the check but only use the
>>> constant value)?
>>

thanks guys

>> Perhaps __bf_shf should not use __builtin_ffsll.
>
> __bf_shf() is a constant expression, and is fine in this context.
>
> The problem is the use of the compound statement here:
>
> static int x[2] = {
> ({ (void)(0); 1; }),
> 0,
> }
>
> similarly fails to compile.
>
> I've recently run into a similar situation, namely in
> include/net/netlink.h, and the applicable way to solve it here would be
> something like this:
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 3f1ef4450a7c..0680d641923f 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -49,19 +49,16 @@
>
> #define __bf_shf(x) (__builtin_ffsll(x) - 1)
>
> +#define BUILD_BUG_ON_RET_ZERO(cond) (sizeof(char[1 - 2*!!(cond)]) - 1)
> +#define BUILD_BUG_ON_NOT_POW2_RET_ZERO(n) BUILD_BUG_ON_RET_ZERO(((n) & ((n) - 1)) != 0)
> +

> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> - ({ \
> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
> - _pfx "mask is not constant"); \
> - BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
> - BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
> - ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
> - _pfx "value too large for the field"); \
> - BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
> - _pfx "type of reg too small for mask"); \
> - __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
> - (1ULL << __bf_shf(_mask))); \
> - })
> + BUILD_BUG_ON_RET_ZERO(!__builtin_constant_p(_mask)) + \
> + BUILD_BUG_ON_RET_ZERO((_mask) == 0) + \
> + BUILD_BUG_ON_RET_ZERO(__builtin_constant_p(_val) ? \
> + ~((_mask) >> __bf_shf(_mask)) & (_val) : 0) + \
> + BUILD_BUG_ON_RET_ZERO((_mask) > (typeof(_reg))~0ull) + \
> + BUILD_BUG_ON_NOT_POW2_RET_ZERO((_mask) + (1ULL << __bf_shf(_mask)))
>
> /**
> * FIELD_FIT() - check if value fits in the field
> @@ -85,10 +82,8 @@
> * be combined with other fields of the bitfield using logical OR.
> */
> #define FIELD_PREP(_mask, _val) \
> - ({ \
> - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> - ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> - })
> + (__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ") + \
> + (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask)))
>
> /**
> * FIELD_GET() - extract a bitfield element
>
>
> Note that this is an incomplete patch - everything but FIELD_PREP will
> not compile with this.
>
> Also, BUILD_BUG_ON_RET_ZERO and BUILD_BUG_ON_NOT_POW2_RET_ZERO should
> probably have better names, or perhaps do the positive way that I did in
> __NLA_ENSURE, e.g. CONST_ASSERT()/CONST_ASSERT_IS_POWER_OF_2()? I guess
> they should go to build_bug.h as well...

Seems reasonable. However I did try this and was getting compiler
warnings about VLA, from a non-constant being fed into
BUILD_BUG_ON_RET_ZERO(), related to sizeof char[]:
drivers/iio/adc/meson_saradc.c:375:2: warning: ISO C90 forbids variable
length array [-Wvla]
regval = FIELD_PREP(MESON_SAR_ADC_CHAN_LIST_ENTRY_MASK(0),

Surely __NLA_ENSURE is getting a similar issue as it uses a similar
principle, no? I see that this is in -next now, but could not this macro
or derivatives being referenced.

>

Much appreciated,
John

> johannes
>
> .
>


\
 
 \ /
  Last update: 2018-10-11 16:26    [W:0.062 / U:0.236 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site