Question on FIELD_PREP() for static array

Question on FIELD_PREP() for static array

[John Garry]

(to: get_maintainers -f include/linux/bitfield.h)

Hi,

I would like to use FIELD_PREP() macro for assigning a static array, 
like this:
function()
{
	static u32 val[2] = {FIELD_PREP(GENMASK_ULL(10, 0), 5), 0};

}

However the compiler complains of non-const expression:
./include/linux/bitfield.h:88:2: error: initializer element is not constant
    ({        \

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 in advance,
John

Re: Question on FIELD_PREP() for static array

[Joe Perches]

On Wed, 2018-10-10 at 18:16 +0100, John Garry wrote:
> (to: get_maintainers -f include/linux/bitfield.h)
> 
> Hi,
> 
> I would like to use FIELD_PREP() macro for assigning a static array, 
> like this:
> function()
> {
> 	static u32 val[2] = {FIELD_PREP(GENMASK_ULL(10, 0), 5), 0};
> 
> }
> 
> However the compiler complains of non-const expression:
> ./include/linux/bitfield.h:88:2: error: initializer element is not constant
>     ({        \
> 
> 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)?

Perhaps __bf_shf should not use __builtin_ffsll.

As mask is known constant, maybe expand the test
in-place with some switch/case.

Re: Question on FIELD_PREP() for static array

[Johannes Berg]

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)?
> 
> 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...

johannes

Re: Question on FIELD_PREP() for static array

[John Garry]

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
>
> .
>

Re: Question on FIELD_PREP() for static array

[Johannes Berg]

On Thu, 2018-10-11 at 15:24 +0100, John Garry wrote:
> 
> > +#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)

> 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),

Hmm, what's the code there?

I don't see why the compiler should think it's a variable length?

> 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.

Yeah, I have a patch now to reference it, but I don't see anything from
-Wvla with gcc 8.1?

See
https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/commit/?id=3d7af878357acd9e37fc156928106f1a969c8942
and its parent.

Do you see -Wvla warnings there? Any idea how I could reproduce them?

johannes

Re: Question on FIELD_PREP() for static array

[John Garry]

On 11/10/2018 16:23, Johannes Berg wrote:

Hi

> On Thu, 2018-10-11 at 15:24 +0100, John Garry wrote:
>>
>>> +#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)
>
>> 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),
>
> Hmm, what's the code there?

Nothing special, it was just a sample. Here'e the code:
     regval = FIELD_PREP(MESON_SAR_ADC_CHAN_LIST_ENTRY_MASK(0),
                 chan->address);

So val is a variable, and I find if remove both of the 
BUILD_BUG_ON_RET_ZERO()' which use __bf_shf() then it goes away.

>
> I don't see why the compiler should think it's a variable length?
>
>> 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.
>
> Yeah, I have a patch now to reference it, but I don't see anything from
> -Wvla with gcc 8.1?

I'm using a 7.3.1-based toolchain

>
> See
> https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/commit/?id=3d7af878357acd9e37fc156928106f1a969c8942
> and its parent.
>
> Do you see -Wvla warnings there? Any idea how I could reproduce them?

I'll try it, thanks

John

>
> johannes
>
> .
>

Re: Question on FIELD_PREP() for static array

[John Garry]

>> Yeah, I have a patch now to reference it, but I don't see anything from
>> -Wvla with gcc 8.1?
>
> I'm using a 7.3.1-based toolchain
>
>>
>> See
>> https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/commit/?id=3d7af878357acd9e37fc156928106f1a969c8942
>>
>> and its parent.
>>
>> Do you see -Wvla warnings there? Any idea how I could reproduce them?
>
> I'll try it, thanks

3d7af878357acd9e37f builds ok. However I am using 20181010-next (I'm not 
sure what yours is based on), and I just noticed that it includes this 
new guy:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/Makefile?h=next-20181011&id=bc5701d8e27fd8beaf895176982fc6a97878f3b8

When I cherry-pick this onto your codebase@3d7af878357acd9e37f, the 
82011 code has warns.

Thanks,
John


>
> John
>
>>
>> johannes
>>
>> .
>>
>

Re: Question on FIELD_PREP() for static array

[Johannes Berg]

On Thu, 2018-10-11 at 18:26 +0100, John Garry wrote:
> > > Yeah, I have a patch now to reference it, but I don't see anything from
> > > -Wvla with gcc 8.1?
> > 
> > I'm using a 7.3.1-based toolchain
> > 
> > > 
> > > See
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/commit/?id=3d7af878357acd9e37fc156928106f1a969c8942
> > > 
> > > and its parent.
> > > 
> > > Do you see -Wvla warnings there? Any idea how I could reproduce them?
> > 
> > I'll try it, thanks
> 
> 3d7af878357acd9e37f builds ok. However I am using 20181010-next (I'm not 
> sure what yours is based on), and I just noticed that it includes this 
> new guy:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/Makefile?h=next-20181011&id=bc5701d8e27fd8beaf895176982fc6a97878f3b

Right, but I added -Wvla by adding subdir-ccflags-y to my Makefile for
this test, and I don't see the warning.

I tried with gcc 7.3.1 too now (Fedora 27) and it doesn't provoke the
warning, even if apply bc5701d8e27fd (manually). I did ensure with V=1
that it shows up on the compiler command line.

This is on x86-64, are you using something else?

Hmm.

However, I have another trick:

#define __NLA_ENSURE(condition) (0 * sizeof(struct { unsigned int x:1 - 2*!(condition);}))

or, in this context,

#define BUILD_BUG_ON_RET_ZERO(cond) (0 * sizeof(struct { unsigned int x:1 - 2*!(condition);}))


What do you think?

johannes

Re: Question on FIELD_PREP() for static array

[Johannes Berg]

On Thu, 2018-10-11 at 21:13 +0200, Johannes Berg wrote:
> 
> However, I have another trick:
> 
> #define __NLA_ENSURE(condition) (0 * sizeof(struct { unsigned int x:1 - 2*!(condition);}))
> 
> or, in this context,
> 
> #define BUILD_BUG_ON_RET_ZERO(cond) (0 * sizeof(struct { unsigned int x:1 - 2*!(condition);}))

Oops, I forgot to insert the second !, it must be

#define BUILD_BUG_ON_RET_ZERO(cond) (0 * sizeof(struct { unsigned int x:1 - 2*!!(condition);}))

johannes