Message-ID: <48BC1B31.1050306@panasas.com>
Date:	Mon, 01 Sep 2008 19:41:21 +0300
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	Jan Beulich <jbeulich@...ell.com>
CC:	"David S. Miller" <davem@...emloft.net>,
	Ingo Molnar <mingo@...e.hu>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Ivo van Doorn <IvDoorn@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Theodore Tso <tytso@....edu>,
	Rusty Russell <rusty@...tcorp.com.au>,
	"John W. Linville" <linville@...driver.com>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const			 expressions

Jan Beulich wrote:
>>>> Boaz Harrosh <bharrosh@...asas.com> 01.09.08 17:00 >>>
>> What is broken with my BUILD_BUG_ON_ZERO(). I tried all tests and
>> it works fine. Do you have a test with unwanted results?
>> (Actually it's the original one I have not touched it).
> 
> That's the problem - it uses the same sizeof(char[]) approach, and hence
> has the same problems that you just try to fix for BUILD_BUG_ON().
> 

No it does not have this problem. Have you tested it? 
I have! It works fine. (Complains on non-const expressions)

The problem with the original BUILD_BUG_ON was the 
(void)() cast and the interaction with the optimizer.

>>>>> Of course you could wrap the whole thing in ({}),
>>>> "do{}while(0)" is effectively an "{}" plus the added bonus
>>>> of demanding an ";" ;-)
>>> An expression likewise demands a terminating ; (or a continuation of the
>>> expression, i.e. by using an operator)
>>>
>> I was not criticizing your approach, I was commenting on:
>> "{}" vs. do{}while(0) 
> 
> I think we're having some mis-understanding here: What I'm concerned
> about is that you can't use statements where-ever expressions can be
> used (the opposite is true, because expressions are statements). My
> main concern is the limitation of its use inside macros, where one could
> try to use comma expressions to combine multiple BUILD_BUG_ON()-s.
> 

So it has not been used and it will not in the future.
In macros just use multiple statements separated by ";"
and surrounded by do{}while(0). To reach the same effect.

Note that BUG_ON is a statement and now BUILD_BUG_ON() is also.
for expressions use BUILD_BUG_ON_ZERO() 

> Jan
> 

OK I have tested with your version of BUILD_BUG_ON{,_ZERO} and I have
two problems with it.

[1] Problem number one is with this sequence of code:

(a)
static inline bool __virtio_has_feature(const struct virtio_device *vdev,
				      unsigned int fbit)
{
	virtio_check_driver_offered_feature(vdev, fbit);
	return test_bit(fbit, vdev->features);
}

/* Did you forget to fix assumptions on max features? */
#define virtio_has_feature(vdev, fbit) \
	((__builtin_constant_p(fbit) ? BUILD_BUG_ON_ZERO(fbit >= 32) : 0) + \
	__virtio_has_feature(vdev, fbit))

...
(b)
static inline int virtio_config_buf(struct virtio_device *vdev,
				    unsigned int fbit,
				    unsigned int offset,
				    void *buf, unsigned len)
{
	if (!virtio_has_feature(vdev, fbit))
		return -ENOENT;
...
(c)
	if (data && !virtio_has_feature(vdev, VIRTIO_NET_F_CSUM))
		return -ENOSYS;

I have not fount a way to code (a) in a way that actually works
for both (b) and (c). That is, good with const expressions and
bypassed with non-const. (See explanations below)

with your code:
> --- linux-2.6.27-rc5/include/linux/virtio_config.h	2008-08-21 14:37:35.000000000 +0200
> +++ 2.6.27-rc5-build-bug-on/include/linux/virtio_config.h	2008-08-28 15:08:47.000000000 +0200
> @@ -96,8 +96,7 @@ static inline bool virtio_has_feature(co
>  				      unsigned int fbit)
>  {
>  	/* Did you forget to fix assumptions on max features? */
> -	if (__builtin_constant_p(fbit))
> -		BUILD_BUG_ON(fbit >= 32);
> +	MAYBE_BUILD_BUG_ON(fbit >= 32);
>  
>  	virtio_check_driver_offered_feature(vdev, fbit);
>  	return test_bit(fbit, vdev->features);

The MAYBE_BUILD_BUG_ON just gets ignored in all cases and
coding (c) as:

	if (data && !virtio_has_feature(vdev, 717))
		return -ENOSYS;

Will not complain at all.

[2] Problem number two is with this sequence of code:

#define is_power_of_two(x)	( !((x) & ((x)-1)) )
#define low_bit_mask(x)		( ((x)-1) & ~(x) )
#define is_valid_mask(x)	is_power_of_two(1 + (x) + \
					low_bit_mask(x))

#define FIELD_CHECK(__mask, __type)			\
	BUILD_BUG_ON(!__builtin_constant_p(__mask) ||	\
		     !(__mask) ||			\
		     !is_valid_mask(__mask) ||		\
		     (__mask) != (__type)(__mask))	\

#define FIELD8(__mask)				\
({						\
	FIELD_CHECK(__mask, u8);		\
	(struct rt2x00_field8) {		\
		compile_ffs8(__mask), (__mask)	\
	};					\
})

I expect the compiler to complain since I'm abusing the
"sign-bit" and need unsigned calculations, but the 
(void)() cast musks the all operation off. Actually the 
all FIELD_CHECK is ignored, I have tried illegal values
and they are let through.



>From what I understand the (void)() cast gets thrown away
by the optimizer very fast before it is fully generated hence
the silence in lots of "bad" cases. The BUILD_BUG_ON_ZERO does
not have this problem because it cannot be optimized out.
On the other hand with your bit field approach the constant-expression
is requested by the compiler very early and cannot be
"optimized out" earlier like in the problem [1] case.

I agree that this is all very ugly in theory. But in practice
it is simple, works(*), and lets me do things like problem[1]
that still produce results.

(*) By works I mean:
 Has no false-positives and no false-negatives. On two accounts:
 constness is enforced, condition is fully checked.

Please show example code that fails my bold (maybe even blunt)
statements, where you would expect one result and the compiler
does something else.

Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists