Re: [PATCH] kmemcheck: fix sparse warning

Re: [PATCH] kmemcheck: fix sparse warning

[Vegard Nossum]

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

Thanks.


Vegard

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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kmemcheck: fix sparse warning

[Bernd Petrovitsch]

[ stripped people from To: and Cc: - far too trivial and well-known
answer ]

On Wed, 2009-07-08 at 23:29 -0700, Josh Triplett wrote:
> [Adding Linus and Chris Li to CC; Linus for further background on
> -Wdo-while, and Chris Li for Sparse.]
[....]
> Much discussion has occurred previously suggesting that Sparse's -Wall
> should match GCC's "all useful warnings" rather than "all possible
this is usually phrased as "all useful warnings in RMS opinion". I'm not
sure how far RMS influence is today on that.

> warnings", and some other option should exist for "all possible
> warnings" (for instance, -Weverything).  This seems very reasonable.
gcc's has "-Wextra" since ages which enables lots of other (IMHO) useful
warnings.
But it's just a name ....

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services