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 ++.
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 <[email protected]> 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 <[email protected]>
Signed-off-by: Florian Mickler <[email protected]>
---
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
On Wed, 3 Nov 2010 13:04:33 +0100
Florian Mickler <[email protected]> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Florian Mickler <[email protected]>
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';
On 11/3/10, Florian Mickler <[email protected]> wrote:
> On Wed, 3 Nov 2010 13:04:33 +0100
> Florian Mickler <[email protected]> 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.
On Wed, 3 Nov 2010 16:26:44 +0100
Audun Hoem <[email protected]> wrote:
> On 11/3/10, Florian Mickler <[email protected]> wrote:
> > On Wed, 3 Nov 2010 13:04:33 +0100
> > Florian Mickler <[email protected]> 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
Hi Andy,
did you get this?
Regards,
Flo
On Wed, 3 Nov 2010 16:20:42 +0100
Florian Mickler <[email protected]> wrote:
> On Wed, 3 Nov 2010 13:04:33 +0100
> Florian Mickler <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Florian Mickler <[email protected]>
>
> 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';
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