Re: Question on FIELD_PREP() for static array

* Re: Question on FIELD_PREP() for static array
       [not found] ` <40fd3d963820bf96547fa9b5e8c171c6a339674e.camel@perches.com>
@ 2018-10-10 18:13   ` Johannes Berg
  2018-10-11 14:24     ` John Garry
  0 siblings, 1 reply; 7+ messages in thread
From: Johannes Berg @ 2018-10-10 18:13 UTC (permalink / raw)
  To: Joe Perches, John Garry, Andrew Morton, Andy Shevchenko,
	Kalle Valo, jakub.kicinski, yamada.masahiro, Arnd Bergmann, viro,
	linux-kernel
  Cc: linux-wireless, nbd

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread