checkpatch false positon on EXPORT_SYMBOL

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.

Quoted 9 lines · 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 ! */

Quoted 4 lines

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.

Quoted 29 lines · Joe Perches

Quoted 10 lines · 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 ! */

Quoted 4 lines

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 */
$

Quoted 5 lines

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.

Quoted 36 lines · Daniel Walker

Quoted 34 lines · Joe Perches

Quoted 10 lines · 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 ! */

Quoted 4 lines

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.

Quoted 1 line

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.

From 223fc7ef4ca0134bf64af0a107532dc3e4010c87 Mon Sep 17 00:00:00 2001

Quoted 34 lines · Joe Perches

Quoted 33 lines · Daniel Walker

Quoted 32 lines · Joe Perches

Quoted 9 lines · 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 ! */

Quoted 4 lines

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.

Quoted 7 lines

Quoted 1 line

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 <[email protected]>
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 <[email protected]>

Signed-off-by Andy Whitcroft <[email protected]>

---
  scripts/checkpatch.pl | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

scripts/checkpatch.pl +0 -0

index d574d13..b581529f 100755

scripts/checkpatch.pl +0 -0

@@ -3000,7 +3000,7 @@ sub process {
 
  			$realline_next = $line_nr_next;

  if (defined $realline_next && (!defined $lines[$realline_next - 1] ||

scripts/checkpatch.pl +1 -1

-			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
+			     substr($lines[$realline_next - 1], $off_next) =~ /^($;|\s)*$/)) {

  $realline_next++;
   }

scripts/checkpatch.pl

Quoted 2 lines · Andy Whitcroft

scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 3006

Quoted 9 lines

scripts/checkpatch.pl +1 -1

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

scripts/checkpatch.pl

Quoted 2 lines · Andy Whitcroft

scripts/checkpatch.pl +0 -0

[]

Quoted 9 lines

scripts/checkpatch.pl +0 -0

@@ -3000,7 +3000,7 @@ sub process {
 
  			$realline_next = $line_nr_next;

  if (defined $realline_next && (!defined $lines[$realline_next - 1] ||

scripts/checkpatch.pl +1 -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.

From 4fd261803ca26cc4ff5b5efb04c0bf8b7bb713e9 Mon Sep 17 00:00:00 2001