Messages in this thread | | | Subject | Re: checkpatch false positon on EXPORT_SYMBOL | From | Daniel Walker <> | Date | Tue, 12 Apr 2016 06:37:18 -0700 |
| |
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
| |