checkpatch false positon on EXPORT_SYMBOL

checkpatch false positon on EXPORT_SYMBOL

[Daniel Walker]

The below looks like normal code but the last export symbol gets the 
warning,


WARNING:EXPORT_SYMBOL: EXPORT_SYMBOL(foo); should immediately follw its 
function/variable
#16: FILE: kernel/acct.c:70:
+EXPORT_SYMBOL(test_export);    /* Error ! */

It seems to have to do with the comments at the end of the line. The 
first two examples don't have warnings because I removed the comments on 
different lines. comments on the variable and export symbol lines gets 
the error tho.

(warning may not be a proper patch)

diff --git a/kernel/acct.c b/kernel/acct.c
index 8d6e145..a0cc002 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -60,6 +60,15 @@
  #include <linux/blkdev.h> /* sector_div */
  #include <linux/pid_namespace.h>

+int test_export;
+EXPORT_SYMBOL(test_export);    /* No Error ! */
+
+int test_export;    /* No Error below */
+EXPORT_SYMBOL(test_export);
+
+int test_export;    /* Error below */
+EXPORT_SYMBOL(test_export);    /* Error ! */
+
  /*
   * These constants control the amount of freespace that suspend and
   * resume the process accounting system, and the time delay between

Re: checkpatch false positon on EXPORT_SYMBOL

[Joe Perches]

On Thu, 2016-03-31 at 08:01 -0700, Daniel Walker wrote:
> The below looks like normal code but the last export symbol gets the 
> warning,
> 
> 
> WARNING:EXPORT_SYMBOL: EXPORT_SYMBOL(foo); should immediately follw its 
> function/variable
> #16: FILE: kernel/acct.c:70:
> +EXPORT_SYMBOL(test_export);    /* Error ! */
> 
> It seems to have to do with the comments at the end of the line. The 
> first two examples don't have warnings because I removed the comments on 
> different lines. comments on the variable and export symbol lines gets 
> the error tho.

That looks like a false positive I'll leave for Andy.

$ cat ~/export_symbol.c
int test_export_no_comment;
EXPORT_SYMBOL(test_export_no_comment);
int test_export_comment_int;		/* comment int */
EXPORT_SYMBOL(test_export_int);
int test_export_comment_symbol;
EXPORT_SYMBOL(test_export_symbol);	/* comment symbol */
int test_export_both;			/* comment both 1 */
EXPORT_SYMBOL(test_export_both);	/* comment both 2 */
$

Something's a bit off with the $stat variable:

test_export_int doesn't match the EXPORT_SYMBOL test.
test_export_symbol and test_export_both get warnings.

Re: checkpatch false positon on EXPORT_SYMBOL

[Daniel Walker]

On 03/31/2016 12:21 PM, Joe Perches wrote:
> On Thu, 2016-03-31 at 08:01 -0700, Daniel Walker wrote:
>> The below looks like normal code but the last export symbol gets the
>> warning,
>>
>>
>> WARNING:EXPORT_SYMBOL: EXPORT_SYMBOL(foo); should immediately follw its
>> function/variable
>> #16: FILE: kernel/acct.c:70:
>> +EXPORT_SYMBOL(test_export);    /* Error ! */
>>
>> It seems to have to do with the comments at the end of the line. The
>> first two examples don't have warnings because I removed the comments on
>> different lines. comments on the variable and export symbol lines gets
>> the error tho.
> That looks like a false positive I'll leave for Andy.
>
> $ cat ~/export_symbol.c
> int test_export_no_comment;
> EXPORT_SYMBOL(test_export_no_comment);
> int test_export_comment_int;		/* comment int */
> EXPORT_SYMBOL(test_export_int);
> int test_export_comment_symbol;
> EXPORT_SYMBOL(test_export_symbol);	/* comment symbol */
> int test_export_both;			/* comment both 1 */
> EXPORT_SYMBOL(test_export_both);	/* comment both 2 */
> $
>
> Something's a bit off with the $stat variable:
>
> test_export_int doesn't match the EXPORT_SYMBOL test.
> test_export_symbol and test_export_both get warnings.
>


Did this get solved? I haven't see anything else on it.

Daniel

Re: checkpatch false positon on EXPORT_SYMBOL

[Joe Perches]

On Mon, 2016-04-11 at 14:51 -0700, Daniel Walker wrote:
> On 03/31/2016 12:21 PM, Joe Perches wrote:
> > 
> > On Thu, 2016-03-31 at 08:01 -0700, Daniel Walker wrote:
> > > 
> > > The below looks like normal code but the last export symbol gets the
> > > warning,
> > > 
> > > 
> > > WARNING:EXPORT_SYMBOL: EXPORT_SYMBOL(foo); should immediately follw its
> > > function/variable
> > > #16: FILE: kernel/acct.c:70:
> > > +EXPORT_SYMBOL(test_export);    /* Error ! */
> > > 
> > > It seems to have to do with the comments at the end of the line. The
> > > first two examples don't have warnings because I removed the comments on
> > > different lines. comments on the variable and export symbol lines gets
> > > the error tho.
> > That looks like a false positive I'll leave for Andy.
> > 
> > $ cat ~/export_symbol.c
> > int test_export_no_comment;
> > EXPORT_SYMBOL(test_export_no_comment);
> > int test_export_comment_int;		/* comment int */
> > EXPORT_SYMBOL(test_export_int);
> > int test_export_comment_symbol;
> > EXPORT_SYMBOL(test_export_symbol);	/* comment symbol */
> > int test_export_both;			/* comment both 1 */
> > EXPORT_SYMBOL(test_export_both);	/* comment both 2 */
> > $
> > 
> > Something's a bit off with the $stat variable:
> > 
> > test_export_int doesn't match the EXPORT_SYMBOL test.
> > test_export_symbol and test_export_both get warnings.
> > 
> 
> Did this get solved? I haven't see anything else on it.

Not by me.

I punted to Andy and I haven't heard from him.

There aren't many cases of this defect in the current
kernel tree, so I don't know how much he might care.

Re: checkpatch false positon on EXPORT_SYMBOL

[Andy Whitcroft]

On Mon, Apr 11, 2016 at 03:09:42PM -0700, Joe Perches wrote:
> On Mon, 2016-04-11 at 14:51 -0700, Daniel Walker wrote:
> > On 03/31/2016 12:21 PM, Joe Perches wrote:
> > > 
> > > On Thu, 2016-03-31 at 08:01 -0700, Daniel Walker wrote:
> > > > 
> > > > The below looks like normal code but the last export symbol gets the
> > > > warning,
> > > > 
> > > > 
> > > > WARNING:EXPORT_SYMBOL: EXPORT_SYMBOL(foo); should immediately follw its
> > > > function/variable
> > > > #16: FILE: kernel/acct.c:70:
> > > > +EXPORT_SYMBOL(test_export);    /* Error ! */
> > > > 
> > > > It seems to have to do with the comments at the end of the line. The
> > > > first two examples don't have warnings because I removed the comments on
> > > > different lines. comments on the variable and export symbol lines gets
> > > > the error tho.
> > > That looks like a false positive I'll leave for Andy.
> > > 
> > > $ cat ~/export_symbol.c
> > > int test_export_no_comment;
> > > EXPORT_SYMBOL(test_export_no_comment);
> > > int test_export_comment_int;		/* comment int */
> > > EXPORT_SYMBOL(test_export_int);
> > > int test_export_comment_symbol;
> > > EXPORT_SYMBOL(test_export_symbol);	/* comment symbol */
> > > int test_export_both;			/* comment both 1 */
> > > EXPORT_SYMBOL(test_export_both);	/* comment both 2 */
> > > $
> > > 
> > > Something's a bit off with the $stat variable:
> > > 
> > > test_export_int doesn't match the EXPORT_SYMBOL test.
> > > test_export_symbol and test_export_both get warnings.
> > > 
> > 
> > Did this get solved? I haven't see anything else on it.
> 
> Not by me.
> 
> I punted to Andy and I haven't heard from him.
> 
> There aren't many cases of this defect in the current
> kernel tree, so I don't know how much he might care.

After some debugging it seems we are essentially not finding the
appropriate "next line" when we are parsing either of the second or
third entries.  This leads us to not check the second one at all, and to
check the third one only when think we are parsing the comment.

This all stems from us thinking there are two statements on the same line
as the trailing ; is not actually at the end of line so the next statement
is still on this same line.  Basically inline comments should be considered
as spaces for the purposes of determining the next line for this purpose.

The following patch appears to sort this out.  A quick scan says this
entire next line calculation is still only used for the EXPORT* check so
this should be low risk for other tests.

This works for me on your example, if you have a real world one could
you test it there and let us know.

Thanks.

-apw

>From 223fc7ef4ca0134bf64af0a107532dc3e4010c87 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <apw@canonical.com>
Date: Tue, 12 Apr 2016 13:43:46 +0100
Subject: [PATCH] checkpatch: comments are whitespace for the purposes of
 finding the next line

While parsing statements we are recording the nominal next line for the
purposes of checking that EXPORT* follows exactly on below an appropriate
statement.  Where there is whitespace after a statement end marker (such
as ;) we will move to the next line.  This also needs to apply to inline
comments at the end of a line.

Allows us to more correctly parse:

    +int test_export;
    +EXPORT_SYMBOL(test_export);    /* No Error ! */
    +
    +int test_export2;    /* No Error below */
    +EXPORT_SYMBOL(test_export2);
    +
    +int test_export3;    /* Error below */
    +EXPORT_SYMBOL(test_export3);    /* Error ! */
    +

Reported-by: Daniel Walker <danielwa@cisco.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d574d13..b581529f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3000,7 +3000,7 @@ sub process {
 			$realline_next = $line_nr_next;
 			if (defined $realline_next &&
 			    (!defined $lines[$realline_next - 1] ||
-			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
+			     substr($lines[$realline_next - 1], $off_next) =~ /^($;|\s)*$/)) {
 				$realline_next++;
 			}
 
-- 
2.7.4

Re: checkpatch false positon on EXPORT_SYMBOL

[Daniel Walker]

On 04/12/2016 05:59 AM, Andy Whitcroft wrote:
> On Mon, Apr 11, 2016 at 03:09:42PM -0700, Joe Perches wrote:
>> On Mon, 2016-04-11 at 14:51 -0700, Daniel Walker wrote:
>>> On 03/31/2016 12:21 PM, Joe Perches wrote:
>>>> On Thu, 2016-03-31 at 08:01 -0700, Daniel Walker wrote:
>>>>> The below looks like normal code but the last export symbol gets the
>>>>> warning,
>>>>>
>>>>>
>>>>> WARNING:EXPORT_SYMBOL: EXPORT_SYMBOL(foo); should immediately follw its
>>>>> function/variable
>>>>> #16: FILE: kernel/acct.c:70:
>>>>> +EXPORT_SYMBOL(test_export);    /* Error ! */
>>>>>
>>>>> It seems to have to do with the comments at the end of the line. The
>>>>> first two examples don't have warnings because I removed the comments on
>>>>> different lines. comments on the variable and export symbol lines gets
>>>>> the error tho.
>>>> That looks like a false positive I'll leave for Andy.
>>>>
>>>> $ cat ~/export_symbol.c
>>>> int test_export_no_comment;
>>>> EXPORT_SYMBOL(test_export_no_comment);
>>>> int test_export_comment_int;		/* comment int */
>>>> EXPORT_SYMBOL(test_export_int);
>>>> int test_export_comment_symbol;
>>>> EXPORT_SYMBOL(test_export_symbol);	/* comment symbol */
>>>> int test_export_both;			/* comment both 1 */
>>>> EXPORT_SYMBOL(test_export_both);	/* comment both 2 */
>>>> $
>>>>
>>>> Something's a bit off with the $stat variable:
>>>>
>>>> test_export_int doesn't match the EXPORT_SYMBOL test.
>>>> test_export_symbol and test_export_both get warnings.
>>>>
>>> Did this get solved? I haven't see anything else on it.
>> Not by me.
>>
>> I punted to Andy and I haven't heard from him.
>>
>> There aren't many cases of this defect in the current
>> kernel tree, so I don't know how much he might care.
> After some debugging it seems we are essentially not finding the
> appropriate "next line" when we are parsing either of the second or
> third entries.  This leads us to not check the second one at all, and to
> check the third one only when think we are parsing the comment.
>
> This all stems from us thinking there are two statements on the same line
> as the trailing ; is not actually at the end of line so the next statement
> is still on this same line.  Basically inline comments should be considered
> as spaces for the purposes of determining the next line for this purpose.
>
> The following patch appears to sort this out.  A quick scan says this
> entire next line calculation is still only used for the EXPORT* check so
> this should be low risk for other tests.
>
> This works for me on your example, if you have a real world one could
> you test it there and let us know.
>
> Thanks.
>
> -apw
>
>  From 223fc7ef4ca0134bf64af0a107532dc3e4010c87 Mon Sep 17 00:00:00 2001
> From: Andy Whitcroft <apw@canonical.com>
> Date: Tue, 12 Apr 2016 13:43:46 +0100
> Subject: [PATCH] checkpatch: comments are whitespace for the purposes of
>   finding the next line
>
> While parsing statements we are recording the nominal next line for the
> purposes of checking that EXPORT* follows exactly on below an appropriate
> statement.  Where there is whitespace after a statement end marker (such
> as ;) we will move to the next line.  This also needs to apply to inline
> comments at the end of a line.
>
> Allows us to more correctly parse:
>
>      +int test_export;
>      +EXPORT_SYMBOL(test_export);    /* No Error ! */
>      +
>      +int test_export2;    /* No Error below */
>      +EXPORT_SYMBOL(test_export2);
>      +
>      +int test_export3;    /* Error below */
>      +EXPORT_SYMBOL(test_export3);    /* Error ! */
>      +
>
> Reported-by: Daniel Walker <danielwa@cisco.com>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>   scripts/checkpatch.pl | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d574d13..b581529f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3000,7 +3000,7 @@ sub process {
>   			$realline_next = $line_nr_next;
>   			if (defined $realline_next &&
>   			    (!defined $lines[$realline_next - 1] ||
> -			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
> +			     substr($lines[$realline_next - 1], $off_next) =~ /^($;|\s)*$/)) {
>   				$realline_next++;
>   			}
>   

Tested-By: Daniel Walker <danielwa@cisco.com>

Seems to clear up the error on my real world example.

Daniel

Re: checkpatch false positon on EXPORT_SYMBOL

[Joe Perches]

> On Tue, 2016-04-12 at 13:59 +0100, Andy Whitcroft wrote:
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3000,7 +3000,7 @@ sub process {
> 
>  			$realline_next = $line_nr_next;
>  			if (defined $realline_next &&
>  			    (!defined $lines[$realline_next - 1] ||
> -			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
> +			     substr($lines[$realline_next - 1], $off_next) =~ /^($;|\s)*$/)) {
>  				$realline_next++;
>   			}

This doesn't work with c99 comments like:

int foo;		// comment
EXPORT_SYMBOL(foo);	// comment

but then again, there aren't any uses like that in the
kernel tree so it almost certainly doesn't matter.

Thanks Andy.

Re: checkpatch false positon on EXPORT_SYMBOL

[Daniel Walker]

On 04/12/2016 10:49 AM, Joe Perches wrote:
>> On Tue, 2016-04-12 at 13:59 +0100, Andy Whitcroft wrote:
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -3000,7 +3000,7 @@ sub process {
>>
>>   			$realline_next = $line_nr_next;
>>   			if (defined $realline_next &&
>>   			    (!defined $lines[$realline_next - 1] ||
>> -			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
>> +			     substr($lines[$realline_next - 1], $off_next) =~ /^($;|\s)*$/)) {
>>   				$realline_next++;
>>    			}
> This doesn't work with c99 comments like:
>
> int foo;		// comment
> EXPORT_SYMBOL(foo);	// comment
>
> but then again, there aren't any uses like that in the
> kernel tree so it almost certainly doesn't matter.
>
> Thanks Andy.


checkpatch.pl is used prior to sending code to a mailing list. So if 
someone adds code with C99 comments then they would get false warnings.

Daniel

Re: checkpatch false positon on EXPORT_SYMBOL

[Andy Whitcroft]

On Tue, Apr 12, 2016 at 10:49:17AM -0700, Joe Perches wrote:
> > On Tue, 2016-04-12 at 13:59 +0100, Andy Whitcroft wrote:
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -3000,7 +3000,7 @@ sub process {
> > 
> >  			$realline_next = $line_nr_next;
> >  			if (defined $realline_next &&
> >  			    (!defined $lines[$realline_next - 1] ||
> > -			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
> > +			     substr($lines[$realline_next - 1], $off_next) =~ /^($;|\s)*$/)) {
> >  				$realline_next++;
> >   			}
> 
> This doesn't work with c99 comments like:
> 
> int foo;		// comment
> EXPORT_SYMBOL(foo);	// comment
> 
> but then again, there aren't any uses like that in the
> kernel tree so it almost certainly doesn't matter.
> 
> Thanks Andy.

Ok, that makes sense.  Lets try the below.

-apw

>From 4fd261803ca26cc4ff5b5efb04c0bf8b7bb713e9 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <apw@canonical.com>
Date: Tue, 12 Apr 2016 13:43:46 +0100
Subject: [PATCH] checkpatch: comments are whitespace for the purposes of
 finding the next line

While parsing statements we are recording the nominal next line for the
purposes of checking that EXPORT* follows exactly on below an appropriate
statement.  Where there is whitespace after a statement end marker (such
as ;) we will move to the next line.  This also needs to apply to inline
comments at the end of a line.

Allows us to more correctly parse:

    +int test_export;
    +EXPORT_SYMBOL(test_export);    /* No Error ! */
    +
    +int test_export2;    /* No Error below */
    +EXPORT_SYMBOL(test_export2);
    +
    +int test_export3;    /* Error below */
    +EXPORT_SYMBOL(test_export3);    /* Error ! */
    +
    +int test_export4;    // Error below
    +EXPORT_SYMBOL(test_export4);    // Error !
    +

Reported-by: Daniel Walker <danielwa@cisco.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d574d13..c58bc4d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3000,7 +3000,7 @@ sub process {
 			$realline_next = $line_nr_next;
 			if (defined $realline_next &&
 			    (!defined $lines[$realline_next - 1] ||
-			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
+			     substr($lines[$realline_next - 1], $off_next) =~ /^($;|\s|\/\/.*)*$/)) {
 				$realline_next++;
 			}
 
-- 
2.7.4