lkml.org 
[lkml]   [2016]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: checkpatch false positon on EXPORT_SYMBOL
From
Date
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

\
 
 \ /
  Last update: 2016-04-12 16:01    [W:0.104 / U:0.224 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site