[PATCH] kmemcheck: fix sparse warning

[PATCH] kmemcheck: fix sparse warning

[Johannes Berg]

Whether or not the sparse warning

warning: do-while statement is not a compound statement

is justified or not in this case, it is annoying and
trivial to fix.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 include/linux/kmemcheck.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- wireless-testing.orig/include/linux/kmemcheck.h	2009-07-06 11:41:16.000000000 +0200
+++ wireless-testing/include/linux/kmemcheck.h	2009-07-06 11:41:30.000000000 +0200
@@ -137,13 +137,13 @@ static inline void kmemcheck_mark_initia
 	int name##_end[0];
 
 #define kmemcheck_annotate_bitfield(ptr, name)				\
-	do if (ptr) {							\
+	do { if (ptr) {							\
 		int _n = (long) &((ptr)->name##_end)			\
 			- (long) &((ptr)->name##_begin);		\
 		BUILD_BUG_ON(_n < 0);					\
 									\
 		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
-	} while (0)
+	} } while (0)
 
 #define kmemcheck_annotate_variable(var)				\
 	do {								\

Re: [PATCH] kmemcheck: fix sparse warning

[Pekka Enberg]

Johannes Berg wrote:
> Whether or not the sparse warning
> 
> warning: do-while statement is not a compound statement
> 
> is justified or not in this case, it is annoying and
> trivial to fix.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Looks good to me!

Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>

> ---
>  include/linux/kmemcheck.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- wireless-testing.orig/include/linux/kmemcheck.h	2009-07-06 11:41:16.000000000 +0200
> +++ wireless-testing/include/linux/kmemcheck.h	2009-07-06 11:41:30.000000000 +0200
> @@ -137,13 +137,13 @@ static inline void kmemcheck_mark_initia
>  	int name##_end[0];
>  
>  #define kmemcheck_annotate_bitfield(ptr, name)				\
> -	do if (ptr) {							\
> +	do { if (ptr) {							\
>  		int _n = (long) &((ptr)->name##_end)			\
>  			- (long) &((ptr)->name##_begin);		\
>  		BUILD_BUG_ON(_n < 0);					\
>  									\
>  		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
> -	} while (0)
> +	} } while (0)
>  
>  #define kmemcheck_annotate_variable(var)				\
>  	do {								\
> 
> 

Re: [PATCH] kmemcheck: fix sparse warning

[Vegard Nossum]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 2158 bytes --]

2009/7/6 Johannes Berg <johannes@sipsolutions.net>:> Whether or not the sparse warning>> warning: do-while statement is not a compound statement>> is justified or not in this case, it is annoying and> trivial to fix.>> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>> --->  include/linux/kmemcheck.h |    4 ++-->  1 file changed, 2 insertions(+), 2 deletions(-)>> --- wireless-testing.orig/include/linux/kmemcheck.h     2009-07-06 11:41:16.000000000 +0200> +++ wireless-testing/include/linux/kmemcheck.h  2009-07-06 11:41:30.000000000 +0200> @@ -137,13 +137,13 @@ static inline void kmemcheck_mark_initia>        int name##_end[0];>>  #define kmemcheck_annotate_bitfield(ptr, name)                         \> -       do if (ptr) {                                                   \> +       do { if (ptr) {                                                 \>                int _n = (long) &((ptr)->name##_end)                    \>                        - (long) &((ptr)->name##_begin);                \>                BUILD_BUG_ON(_n < 0);                                   \>                                                                        \>                kmemcheck_mark_initialized(&((ptr)->name##_begin), _n); \> -       } while (0)> +       } } while (0)>>  #define kmemcheck_annotate_variable(var)                               \>        do {                                                            \>>>
I'll change the patch title to "kmemcheck: work around bogus sparsewarning" and fix the indentation, sounds ok?
Meanwhile, I Cced sparse mailing list in case somebody else knowsanything else about this warning (what it means, whether it'sjustified in this case, whether it should be fixed in sparse, etc.).
Thanks.

Vegardÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] kmemcheck: fix sparse warning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Wed, 2009-07-08 at 21:28 +0200, Vegard Nossum wrote:

> >  #define kmemcheck_annotate_bitfield(ptr, name)                         \
> > -       do if (ptr) {                                                   \
> > +       do { if (ptr) {                                                 \
> >                int _n = (long) &((ptr)->name##_end)                    \
> >                        - (long) &((ptr)->name##_begin);                \
> >                BUILD_BUG_ON(_n < 0);                                   \
> >                                                                        \
> >                kmemcheck_mark_initialized(&((ptr)->name##_begin), _n); \
> > -       } while (0)
> > +       } } while (0)
> >
> >  #define kmemcheck_annotate_variable(var)                               \
> >        do {                                                            \
> >
> >
> >
> 
> I'll change the patch title to "kmemcheck: work around bogus sparse
> warning" and fix the indentation, sounds ok?

Well, it's debatable whether it's bogus or not, but I don't care as long
as it gets fixed.

> Meanwhile, I Cced sparse mailing list in case somebody else knows
> anything else about this warning (what it means, whether it's
> justified in this case, whether it should be fixed in sparse, etc.).

Everybody take me off the CC please then :) This has been discussed far
too many times already. In this special case, the warning probably isn't
all that useful, but ISTR Linus saying that he wanted it this way.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] kmemcheck: fix sparse warning

[Christopher Li]

On Wed, Jul 8, 2009 at 12:39 PM, Johannes Berg<johannes@sipsolutions.net> wrote:
> Everybody take me off the CC please then :) This has been discussed far
> too many times already. In this special case, the warning probably isn't
> all that useful, but ISTR Linus saying that he wanted it this way.

Right. I would keep the warning in sparse. I think it is better with
the compound statement. It is easier to read which block is which.

> -       do if (ptr) {                                                   \
> +       do { if (ptr) {                                                 \

BTW, if does not hurt to put the "if (ptr)" to a separate line.

Chris

Re: [PATCH] kmemcheck: fix sparse warning

[Josh Triplett]

[Adding Linus and Chris Li to CC; Linus for further background on
-Wdo-while, and Chris Li for Sparse.]

On Wed, Jul 08, 2009 at 09:28:24PM +0200, Vegard Nossum wrote:
> 2009/7/6 Johannes Berg <johannes@sipsolutions.net>:
> > Whether or not the sparse warning
> >
> > warning: do-while statement is not a compound statement
> >
> > is justified or not in this case, it is annoying and
> > trivial to fix.
[...]
> 
> I'll change the patch title to "kmemcheck: work around bogus sparse
> warning" and fix the indentation, sounds ok?
> 
> Meanwhile, I Cced sparse mailing list in case somebody else knows
> anything else about this warning (what it means, whether it's
> justified in this case, whether it should be fixed in sparse, etc.).

-Wdo-while gives a warning if you write:

do
    statement
while (...);

where "statement" does not consist of a compound statement surrounded by
braces.  As far as I know, this warning exists primarily because it
matched Linus's preference for readability.

However, note that Sparse does not have -Wdo-while enabled by default.
Sparse only issues that warning if you use -Wdo-while explicitly, or if
you pass -Wall.  (The latter often happens due to passing the same
warning flags to GCC and Sparse, without using cgcc which filters out
-Wall for Sparse for that reason.)

Much discussion has occurred previously suggesting that Sparse's -Wall
should match GCC's "all useful warnings" rather than "all possible
warnings", and some other option should exist for "all possible
warnings" (for instance, -Weverything).  This seems very reasonable.
Given that Sparse *exists* to give warnings, -Wall's "all useful
warnings" approach seems like it matches Sparse's default behavior;
thus, I think it would make the most sense to make Sparse just interpret
-Wall as "enable all the warnings enabled by default" (which would
normally act as a no-op, but would make a difference if you passed
"-Wno-foo -Wall" for some default warning foo).

This would improve Sparse's behavior for the case where you invoke
sparse $(CFLAGS) directly rather than setting CC=cgcc, if CFLAGS
contains -Wall.

- Josh Triplett

Re: [PATCH] kmemcheck: fix sparse warning

[Hannes Eder]

On Thu, Jul 9, 2009 at 08:29, Josh Triplett<josh@joshtriplett.org> wrote:
> [Adding Linus and Chris Li to CC; Linus for further background on
> -Wdo-while, and Chris Li for Sparse.]
>
> On Wed, Jul 08, 2009 at 09:28:24PM +0200, Vegard Nossum wrote:
>> 2009/7/6 Johannes Berg <johannes@sipsolutions.net>:
>> > Whether or not the sparse warning
>> >
>> > warning: do-while statement is not a compound statement
>> >
>> > is justified or not in this case, it is annoying and
>> > trivial to fix.
> [...]
>>
>> I'll change the patch title to "kmemcheck: work around bogus sparse
>> warning" and fix the indentation, sounds ok?
>>
>> Meanwhile, I Cced sparse mailing list in case somebody else knows
>> anything else about this warning (what it means, whether it's
>> justified in this case, whether it should be fixed in sparse, etc.).
>
> -Wdo-while gives a warning if you write:
>
> do
>    statement
> while (...);
>
> where "statement" does not consist of a compound statement surrounded by
> braces.  As far as I know, this warning exists primarily because it
> matched Linus's preference for readability.

see:
http://lkml.org/lkml/2008/12/23/180

an related messages/threads

Cheers,
-Hannes

Re: [PATCH] kmemcheck: fix sparse warning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]

On Mon, 2009-07-06 at 11:53 +0200, Johannes Berg wrote:
> Whether or not the sparse warning
> 
> warning: do-while statement is not a compound statement
> 
> is justified or not in this case, it is annoying and
> trivial to fix.

Whatever happened to this patch? Did you decide to screw sparse
warnings?

johannes

> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
>  include/linux/kmemcheck.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- wireless-testing.orig/include/linux/kmemcheck.h	2009-07-06 11:41:16.000000000 +0200
> +++ wireless-testing/include/linux/kmemcheck.h	2009-07-06 11:41:30.000000000 +0200
> @@ -137,13 +137,13 @@ static inline void kmemcheck_mark_initia
>  	int name##_end[0];
>  
>  #define kmemcheck_annotate_bitfield(ptr, name)				\
> -	do if (ptr) {							\
> +	do { if (ptr) {							\
>  		int _n = (long) &((ptr)->name##_end)			\
>  			- (long) &((ptr)->name##_begin);		\
>  		BUILD_BUG_ON(_n < 0);					\
>  									\
>  		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
> -	} while (0)
> +	} } while (0)
>  
>  #define kmemcheck_annotate_variable(var)				\
>  	do {								\
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] kmemcheck: fix sparse warning

[Pekka Enberg]

Hi,

Johannes Berg wrote:
> On Mon, 2009-07-06 at 11:53 +0200, Johannes Berg wrote:
>> Whether or not the sparse warning
>>
>> warning: do-while statement is not a compound statement
>>
>> is justified or not in this case, it is annoying and
>> trivial to fix.
> 
> Whatever happened to this patch? Did you decide to screw sparse
> warnings?

I thought Vegard applied the patch but I can't see it in kmemcheck.git 
either. Vegard?

			Pekka

>> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>> ---
>>  include/linux/kmemcheck.h |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> --- wireless-testing.orig/include/linux/kmemcheck.h	2009-07-06 11:41:16.000000000 +0200
>> +++ wireless-testing/include/linux/kmemcheck.h	2009-07-06 11:41:30.000000000 +0200
>> @@ -137,13 +137,13 @@ static inline void kmemcheck_mark_initia
>>  	int name##_end[0];
>>  
>>  #define kmemcheck_annotate_bitfield(ptr, name)				\
>> -	do if (ptr) {							\
>> +	do { if (ptr) {							\
>>  		int _n = (long) &((ptr)->name##_end)			\
>>  			- (long) &((ptr)->name##_begin);		\
>>  		BUILD_BUG_ON(_n < 0);					\
>>  									\
>>  		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
>> -	} while (0)
>> +	} } while (0)
>>  
>>  #define kmemcheck_annotate_variable(var)				\
>>  	do {								\
>>
> 

Re: [PATCH] kmemcheck: fix sparse warning

[Raja R Harinath]

Hi,

Johannes Berg <johannes@sipsolutions.net> writes:

>> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

I know I'm colouring the bike-shed but you can avoid the outer do-while
with:

>> --- wireless-testing.orig/include/linux/kmemcheck.h	2009-07-06 11:41:16.000000000 +0200
>> +++ wireless-testing/include/linux/kmemcheck.h	2009-07-06 11:41:30.000000000 +0200
>> @@ -137,13 +137,13 @@ static inline void kmemcheck_mark_initia
>>  	int name##_end[0];
>>  
>>  #define kmemcheck_annotate_bitfield(ptr, name)				\
>> -	do if (ptr) {							\
>> +	do { if (ptr) {							\

   +    if (ptr) {

>>  		int _n = (long) &((ptr)->name##_end)			\
>>  			- (long) &((ptr)->name##_begin);		\
>>  		BUILD_BUG_ON(_n < 0);					\
>>  									\
>>  		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
>> -	} while (0)
>> +	} } while (0)

   +    } else

Of course, that tailing 'else' may be much too clever to live.  A
slightly less clever but more idiomatic approach would be to use

  +    } else do {} while (0)

but what's the point.  Oh well, ignore me :-) I'm just pained by the
'} }' due to my wierd sense of aesthetics.

- Hari