Bug in checkpatch.pl

Bug in checkpatch.pl

[Audun Hoem]

I have stumbled about a bug in checkpatch.pl while working on some
code in drivers/staging. It seems to get confused when confronted with
asterisks. For example, this snippe:

kmalloc(sizeof(struct alphatrack_ocmd) * true_size, GFP_KERNEL);

Here the asterisk is in it's binary form, obviously, and performs a
multiplication, however checkpatch reports this:

drivers/staging/frontier/alphatrack.c:772: ERROR: space prohibited
after that '*' (ctx:WxW)

So it's obviously thinking it's the unary operator, which should only
be preceded by a variable name or another unary operator such as ++.

[PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling

[Florian Mickler]

Casts have to be handled after the last type that is followed by an
opening parenthesis is handled.

On Tue, 2 Nov 2010 23:57:36 +0100
Audun Hoem <audun.hoem@gmail.com> wrote:

> I have stumbled about a bug in checkpatch.pl while working on some
> code in drivers/staging. It seems to get confused when confronted with
> asterisks. For example, this snippe:
>
> kmalloc(sizeof(struct alphatrack_ocmd) * true_size, GFP_KERNEL);
>
> Here the asterisk is in it's binary form, obviously, and performs a
> multiplication, however checkpatch reports this:
>
> drivers/staging/frontier/alphatrack.c:772: ERROR: space prohibited
> after that '*' (ctx:WxW)
>
> So it's obviously thinking it's the unary operator, which should only
> be preceded by a variable name or another unary operator such as ++.

Reported-By: Audun Hoem <audun.hoem@gmail.com>

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 scripts/checkpatch.pl |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 90b54d4..c1cbb09 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -859,10 +859,6 @@ sub annotate_values {
 				$av_preprocessor = 0;
 			}
 
-		} elsif ($cur =~ /^(\(\s*$Type\s*)\)/) {
-			print "CAST($1)\n" if ($dbg_values > 1);
-			push(@av_paren_type, $type);
-			$type = 'C';
 
 		} elsif ($cur =~ /^($Type)\s*(?:$Ident|,|\)|\(|\s*$)/) {
 			print "DECLARE($1)\n" if ($dbg_values > 1);
@@ -963,6 +959,13 @@ sub annotate_values {
 			$type = 'V';
 			$av_pending = 'V';
 
+		} elsif ($cur =~ /^(\(\s*$Type\s*)\)/) {
+			#casts handled after last type that opens a brace
+			#is handled, else it screws up the parens handling
+			print "CAST($1)\n" if ($dbg_values > 1);
+			push(@av_paren_type, $type);
+			$type = 'C';
+
 		} elsif ($cur =~ /^($Ident\s*):(?:\s*\d+\s*(,|=|;))?/) {
 			if (defined $2 && $type eq 'C' || $type eq 'T') {
 				$av_pend_colon = 'B';
-- 
1.7.3.1

Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling

[Florian Mickler]

On Wed,  3 Nov 2010 13:04:33 +0100
Florian Mickler <florian@mickler.org> wrote:

> Casts have to be handled after the last type that is followed by an
> opening parenthesis is handled.

That is the wrong fix. I realized now that with that patch we would
not claim anything as a CAST anymore. Better is probably to only claim
a CAST if av_pending is not set. Andy, would that work? It seems to be
alright... Do you have some tests for checkpatch?

Testing it with the reported line and some other random samples it
seems to be alright.

Regards,
Flo

>8------------------------------------------------------------------------
commit 11ed611c647420ea27124faf269d724b4fd3ade4
Author: Florian Mickler <florian@mickler.org>
Date:   Wed Nov 3 15:54:26 2010 +0100

    checkpatch.pl: fix CAST detection
    
    We should only claim that something is a cast if we did not encouter a
    token before, that did set av_pending.
    
    This fixes the operator * in the line below  to be detected as
    binary (vs unary).
    
     kmalloc(sizeof(struct alphatrack_ocmd) * true_size, GFP_KERNEL);
    
    Reported-by: Audun Hoem <audun.hoem@gmail.com>
    Signed-off-by: Florian Mickler <florian@mickler.org>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 90b54d4..06f5c44 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -859,7 +859,7 @@ sub annotate_values {
 				$av_preprocessor = 0;
 			}
 
-		} elsif ($cur =~ /^(\(\s*$Type\s*)\)/) {
+		} elsif ($cur =~ /^(\(\s*$Type\s*)\)/ && $av_pending eq '_') {
 			print "CAST($1)\n" if ($dbg_values > 1);
 			push(@av_paren_type, $type);
 			$type = 'C';

Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling

[Audun Hoem]

On 11/3/10, Florian Mickler <florian@mickler.org> wrote:
> On Wed,  3 Nov 2010 13:04:33 +0100
> Florian Mickler <florian@mickler.org> wrote:
>
>> Casts have to be handled after the last type that is followed by an
>> opening parenthesis is handled.
>
> That is the wrong fix. I realized now that with that patch we would
> not claim anything as a CAST anymore. Better is probably to only claim
> a CAST if av_pending is not set. Andy, would that work? It seems to be
> alright... Do you have some tests for checkpatch?
>
> Testing it with the reported line and some other random samples it
> seems to be alright.
>

Probably good enough of a test to try it on random kernel code.

Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling

[Florian Mickler]

On Wed, 3 Nov 2010 16:26:44 +0100
Audun Hoem <audun.hoem@gmail.com> wrote:

> On 11/3/10, Florian Mickler <florian@mickler.org> wrote:
> > On Wed,  3 Nov 2010 13:04:33 +0100
> > Florian Mickler <florian@mickler.org> wrote:
> >
> >> Casts have to be handled after the last type that is followed by an
> >> opening parenthesis is handled.
> >
> > That is the wrong fix. I realized now that with that patch we would
> > not claim anything as a CAST anymore. Better is probably to only claim
> > a CAST if av_pending is not set. Andy, would that work? It seems to be
> > alright... Do you have some tests for checkpatch?
> >
> > Testing it with the reported line and some other random samples it
> > seems to be alright.
> >
> 
> Probably good enough of a test to try it on random kernel code.

Yes, on random inspection of kernel code the new output looks good. 
(I've set dbg_values to 2 and diffed the output for some files of
the patched version with the output of the broken version)

I speculated on Andy to have a special set of problematic test
cases, but I looked through the git-log of checkpatch and tested some of
the stuff there. It all seems ok. 

Regards,
Flo

Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling

[Florian Mickler]

Hi Andy, 

did you get this?

Regards,
Flo

On Wed, 3 Nov 2010 16:20:42 +0100
Florian Mickler <florian@mickler.org> wrote:

> On Wed,  3 Nov 2010 13:04:33 +0100
> Florian Mickler <florian@mickler.org> wrote:
> 
> > Casts have to be handled after the last type that is followed by an
> > opening parenthesis is handled.
> 
> That is the wrong fix. I realized now that with that patch we would
> not claim anything as a CAST anymore. Better is probably to only claim
> a CAST if av_pending is not set. Andy, would that work? It seems to be
> alright... Do you have some tests for checkpatch?
> 
> Testing it with the reported line and some other random samples it
> seems to be alright.
> 
> Regards,
> Flo
> 
> >8------------------------------------------------------------------------
> commit 11ed611c647420ea27124faf269d724b4fd3ade4
> Author: Florian Mickler <florian@mickler.org>
> Date:   Wed Nov 3 15:54:26 2010 +0100
> 
>     checkpatch.pl: fix CAST detection
>     
>     We should only claim that something is a cast if we did not encouter a
>     token before, that did set av_pending.
>     
>     This fixes the operator * in the line below  to be detected as
>     binary (vs unary).
>     
>      kmalloc(sizeof(struct alphatrack_ocmd) * true_size, GFP_KERNEL);
>     
>     Reported-by: Audun Hoem <audun.hoem@gmail.com>
>     Signed-off-by: Florian Mickler <florian@mickler.org>
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 90b54d4..06f5c44 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -859,7 +859,7 @@ sub annotate_values {
>  				$av_preprocessor = 0;
>  			}
>  
> -		} elsif ($cur =~ /^(\(\s*$Type\s*)\)/) {
> +		} elsif ($cur =~ /^(\(\s*$Type\s*)\)/ && $av_pending eq '_') {
>  			print "CAST($1)\n" if ($dbg_values > 1);
>  			push(@av_paren_type, $type);
>  			$type = 'C';

Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling

[Andy Whitcroft]

On Wed, Nov 17, 2010 at 08:30:31AM +0100, Florian Mickler wrote:
> 
> Hi Andy, 
> 
> did you get this?

Yep sorry, need to get these hooverered up into my tree.

-apw