diff mbox

checkpatch.pl false positive (was Re: [PATCH v4 0/7] q35: add negotiable broadcast SMI)

Message ID 20161221180155.GI3808@thinpad.lan.raisama.net (mailing list archive)
State New, archived
Headers show

Commit Message

Eduardo Habkost Dec. 21, 2016, 6:01 p.m. UTC 
On Wed, Dec 21, 2016 at 06:47:01PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/12/2016 16:22, Eduardo Habkost wrote:
> > On Tue, Dec 20, 2016 at 03:01:17PM -0800, no-reply@patchew.org wrote:
> > [...]
> >> Checking PATCH 4/7: hw/i386/pc: introduce 2.9 machine types with 0x20 fw_cfg file slots...
> >> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> >> #126: FILE: include/hw/compat.h:4:
> >> +#define HW_COMPAT_2_8 \
> >> +    {\
> >> +        .driver   = "fw_cfg_mem",\
> >> +        .property = "file_slots",\
> >> +        .value    = stringify(0x10),\
> >> +    },{\
> >> +        .driver   = "fw_cfg_io",\
> >> +        .property = "file_slots",\
> >> +        .value    = stringify(0x10),\
> >> +    },
> >>
> >> total: 1 errors, 0 warnings, 119 lines checked
> >>
> >> Your patch has style problems, please review.  If any of these errors
> >> are false positives report them to the maintainer, see
> >> CHECKPATCH in MAINTAINERS.
> > 
> > It is a false positive, but how exactly can we fix it? Should it
> > become a warning instead of an error?
> 
> It should already be treated as an exception:
> 
>                         my $exceptions = qr{
>                                 $Declare|
>                                 module_param_named|
>                                 MODULE_PARAM_DESC|
>                                 DECLARE_PER_CPU|
>                                 DEFINE_PER_CPU|
>                                 __typeof__\(|
>                                 union|
>                                 struct|
>                                 \.$Ident\s*=\s*|       # <---- see here
>                                 ^\"|\"$
>                         }x;
>                         #print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
>                         if ($rest ne '' && $rest ne ',') {
>                                 if ($rest !~ /while\s*\(/ &&
>                                     $dstat !~ /$exceptions/)
>                                 {
>                                         ERROR("Macros with multiple statements should be enclosed in a do - while loop\n" . "$here\n$ctx\n");
>                                 }
> 
> 
> so I guess the first step is debugging it. :)

The following code replaces the whole "{ .driver = ... }" block
with "1":

	# Flatten any parentheses and braces
	while ($dstat =~ s/\([^\(\)]*\)/1/ ||
	       $dstat =~ s/\{[^\{\}]*\}/1/ ||
	       $dstat =~ s/\[[^\{\}]*\]/1/)
	{
	}

The following change fixes the bug, but I don't know if it has
unwanted side-effects:

Comments

Paolo Bonzini Dec. 21, 2016, 6:08 p.m. UTC | #1 
On 21/12/2016 19:01, Eduardo Habkost wrote:
> The following code replaces the whole "{ .driver = ... }" block
> with "1":
> 
> 	# Flatten any parentheses and braces
> 	while ($dstat =~ s/\([^\(\)]*\)/1/ ||
> 	       $dstat =~ s/\{[^\{\}]*\}/1/ ||
> 	       $dstat =~ s/\[[^\{\}]*\]/1/)
> 	{
> 	}

Maybe change it like

- 	       $dstat =~ s/\{[^\{\}]*\}/1/ ||
+ 	       $dstat =~ s/\{[^\{\}]*;[^\{\}]*\}/1;/ ||

so that it requires a statement?  It would have a false positive on
strings containing a semicolon, but that's not a big deal (or could be
fixed by similarly turning strings into just "").

Paolo

> The following change fixes the bug, but I don't know if it has
> unwanted side-effects:
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f084542..0aab3ac 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2200,6 +2200,10 @@ sub process {
>                         $dstat =~ s/^\s*//s;
>                         $dstat =~ s/\s*$//s;
>  
> +                       # remove braces that cover the whole block, if any:
> +                       $dstat =~ s/^\{//;
> +                       $dstat =~ s/\}$//;
> +
>                         # Flatten any parentheses and braces
>                         while ($dstat =~ s/\([^\(\)]*\)/1/ ||
>                                $dstat =~ s/\{[^\{\}]*\}/1/ ||


Paolo
Eduardo Habkost Dec. 21, 2016, 6:19 p.m. UTC | #2 
On Wed, Dec 21, 2016 at 07:08:29PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/12/2016 19:01, Eduardo Habkost wrote:
> > The following code replaces the whole "{ .driver = ... }" block
> > with "1":
> > 
> > 	# Flatten any parentheses and braces
> > 	while ($dstat =~ s/\([^\(\)]*\)/1/ ||
> > 	       $dstat =~ s/\{[^\{\}]*\}/1/ ||
> > 	       $dstat =~ s/\[[^\{\}]*\]/1/)
> > 	{
> > 	}
> 
> Maybe change it like
> 
> - 	       $dstat =~ s/\{[^\{\}]*\}/1/ ||
> + 	       $dstat =~ s/\{[^\{\}]*;[^\{\}]*\}/1;/ ||
> 
> so that it requires a statement?

I am not sure the point of this line is to detect multi-statement
blocks, but to replace other types of expressions containing {}
that could incorrectly match $exceptions, like:

#define FOO { \
   Foo f = { .somefield = 1 }; \
   some_statement(); \
   another_statement();
  }

In this case, $dstat would become:
"{ Foo f = 1; some_statement(); another_statement(); }"


>                                   It would have a false positive on
> strings containing a semicolon, but that's not a big deal (or could be
> fixed by similarly turning strings into just "").

String contents are already replaced with "XXX" by some other
code. These are the contents of $dstat:

 '{        .driver   = "XXXXXXXXXX",        .property = "XXXXXXXXXX",        .value    = stringify1,    }'

> 
> Paolo
> 
> > The following change fixes the bug, but I don't know if it has
> > unwanted side-effects:
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index f084542..0aab3ac 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2200,6 +2200,10 @@ sub process {
> >                         $dstat =~ s/^\s*//s;
> >                         $dstat =~ s/\s*$//s;
> >  
> > +                       # remove braces that cover the whole block, if any:
> > +                       $dstat =~ s/^\{//;
> > +                       $dstat =~ s/\}$//;
> > +
> >                         # Flatten any parentheses and braces
> >                         while ($dstat =~ s/\([^\(\)]*\)/1/ ||
> >                                $dstat =~ s/\{[^\{\}]*\}/1/ ||
> 
> 
> Paolo
diff mbox

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f084542..0aab3ac 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2200,6 +2200,10 @@  sub process {
                        $dstat =~ s/^\s*//s;
                        $dstat =~ s/\s*$//s;
 
+                       # remove braces that cover the whole block, if any:
+                       $dstat =~ s/^\{//;
+                       $dstat =~ s/\}$//;
+
                        # Flatten any parentheses and braces
                        while ($dstat =~ s/\([^\(\)]*\)/1/ ||
                               $dstat =~ s/\{[^\{\}]*\}/1/ ||