<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 15, 2020 at 11:02 AM Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com">lukas.bulwahn@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On Tue, 14 Jul 2020, Mrinal Pandey wrote:<br>
<br>
> <br>
> <br>
> On Sat, Jul 11, 2020 at 3:17 AM Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com" target="_blank">lukas.bulwahn@gmail.com</a>> wrote:<br>
> <br>
> >> For Issue 5: Can you provide me (and the CC: the list) the list of<br>
> >> false positives (the commit hashes) you found for issue 5 on<br>
> >> EXPORT_SYMBOL?<br>
> ><br>
> ><br>
> > Here are the commit hashes for which the warning is issued:<br>
> > 54505a1e2083<br>
> > 75d75b7a4d54<br>
> > 8084c99b9af6<br>
> > bfdaf029c9c9<br>
> > dfd402a4c4ba<br>
> ><br>
> >> Can you also provide a short rationale/explanation for<br>
> >> each case that you considered a false positive?<br>
> ><br>
> ><br>
> > In each case the `EXPORT_SYMBOL()` is correctly written and the variable/function to be exported<br>
> > is also inside the parentheses, still, we get the warning. Please let me know if I am wrong here.<br>
> <br>
> I checked those warnings. Some of the patches are good cases to check if we can improve the heuristics on<br>
> EXPORT_SYMBOL().<br>
> <br>
> E.g., commit bfdaf029c9c9 ("ia64: turn csum_partial_copy_from_user() into csum_and_copy_from_user()") looks<br>
> sound to me, and <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> should not really warn about that.<br>
> <br>
> Mrinal, maybe you can find out why <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> believes that this case is wrong and needs to be warned<br>
> about?<br>
> <br>
> <br>
> Sir,<br>
> <br>
> Commit `54505a1e2083`, `8084c99b9af6` and `bfdaf029c9c9` have style issues. The documentation says to use<br>
> `EXPORT_SYMBOL()` just after the closing bracket `}` of the function, it is exporting. In these commits `EXPORT<br>
> SYMBOL()`<br>
> is either defined at a point later or has been defined after leaving a blank line which is totally unneeded.<br>
><br>
<br>
Possibly, a new line is okay; I think we would some statistics on the <br>
overall use of EXPORT_SYMBOL() to see if a new line is generally accepted <br>
or if it is really a rare case (<3%) and <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> should keep <br>
warning about that.<br></blockquote><div><br></div><div>Sir,</div><div><br></div><div>I checked 2000 uses of EXPORT_SYMBOL, the total usages being 15800 approx. and only 74 of them used a blank line.</div><div>That's 0.037% so probably <a href="http://checkpatch.pl">checkpatch.pl</a> is correct there. There are cases where multiple exports are written together but these are also only</div><div>117 out of 2000.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> However, commits `dfd402a4c4ba` and `75d75b7a4d54` look fine to me and have one thing in common, the<br>
> `EXPORT_SYMBOL()` in these is used inside of a macro. checkpatch shouldn't warn about these.<br>
> <br>
<br>
That is probably tricky to handle with <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a>, though.<br></blockquote><div><br></div><div>I will let you know if I find something that works here. This is probably the only case that is worth noticing.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> I have noticed checkpatch denoting wrong line numbers for certain files for several commits.<br>
> For example, for the above commit<br>
> `bfdaf029c9c9`, the following warning is issued:<br>
> <br>
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable<br>
> #74: FILE: arch/ia64/lib/csum_partial_copy.c:122:<br>
> +EXPORT_SYMBOL(csum_and_copy_from_user);<br>
> <br>
> but the file arch/ia64/lib/csum_partial_copy.c is only 113 lines long.<br>
> Please let me know if you can reproduce this.<br>
<br>
Can you find out how the line number is computed?<br>
<br>
Of course, <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> is running on the patch, not on any specific <br>
file. So, in general, you really do not know how long the file was that <br>
the author based the patch on.<br>
<br>
In our case, you need to check the state of the file at the commit above.<br>
<br>
I checked that and it seemed correct to me, line 122 refers to <br>
EXPORT_SYMBOL. So, I do not see the bug here.<br>
<br></blockquote><div>Yes, the file is checked at that point in the commit and everything seems to be correct. I was checking the <br></div><div>the current version of the file and that's why couldn't get line 122.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
Lukas<br>
</blockquote></div></div>