Subject: Re: checkpatch false positon on EXPORT_SYMBOL
To: Andy Whitcroft <apw@canonical.com>, Joe Perches <joe@perches.com>
References: <56FD3BAE.1070209@cisco.com>
 <1459452096.1744.12.camel@perches.com> <570C1C62.1060008@cisco.com>
 <1460412582.1800.96.camel@perches.com> <20160412125904.GB5107@brain>
Cc: open list <linux-kernel@vger.kernel.org>,
        Daniel Walker <dwalker@fifo99.com>,
        "xe-kernel@external.cisco.com" <xe-kernel@external.cisco.com>
From: Daniel Walker <danielwa@cisco.com>
Message-ID: <570CFA0E.2000106@cisco.com>
Date: Tue, 12 Apr 2016 06:37:18 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160412125904.GB5107@brain>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4383
Lines: 113

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