<div dir="ltr">On Mon, Oct 7, 2013 at 9:55 AM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>I'm fine with this staying in the analyzer for now, unless David, Richard, or Eli feel it should be a warning right away.</div>
</div></blockquote><div><br></div><div>Do we have evidence that we want this? Does it catch bugs? If so, what do they look like? It seems like this would trigger on legitimate code; how does a user suppress the warning in that case, and does that suppression make their code clearer?<br>
</div><div><br></div><div>What is the false/true positive ratio for bug finding here?</div><div><br></div><div>sizeof(expression) is a common idiom in SFINAE contexts. Is that covered here?</div><div><br></div><div>sizeof(sizeof(int)) is a "traditional" way to get sizeof(size_t). Why should we warn on that?</div>
<div><br></div><div>And more as a general question than something specific to this patch: Is there a region in the space of false positive ratios where we think a syntactic warning should go into the static analyzer? If so, why? And what is that region? I would have thought that the static analyzer, like the clang warnings, would be aimed at (eventually) having a false positive ratio of near zero. If so, then should we ever put a warning in the static analyzer if it doesn't require the static analyzer's technology (or have a high runtime cost)?<br>
</div><div><br></div><div>There is a cost to putting a useful check in the static analyzer rather than behind a warning flag: it reduces the accessibility and automatability of the check. It's easier for people to turn on a compiler warning as part of their build than it is to turn on the static analyzer, especially if they want to enforce a policy of no regressions.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>+ if (isa<BinaryOperator>(ArgExpr->IgnoreParens())) {</div>
<div>+ const BinaryOperator *Binop =</div><div>+ dyn_cast<BinaryOperator>(ArgExpr->IgnoreParens());</div></div><div><br></div><div>Conventionally we'd combine these two in the if-condition itself (see the <a href="http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates" target="_blank">LLVM Programmer's Manual</a>). You never need to follow isa<> with a checked dyn_cast<>, either.</div>
<div><br></div><div><br></div><div><div class="im"><div>+ BR.EmitBasicReport(AC->getDecl(),</div><div>+ "Expression with an operation in sizeof()",</div></div><div>+ categories::LogicError,</div>
<div>+ "Using sizeof() on an expression with an operator "</div><div>+ "may yield unexpected results.", ELoc, &R, 1);</div></div><div><br></div><div>
There's already an overload that takes a single range; please use that instead of a count of 1.</div><div><br></div><div><br></div><div><div>+ if (LeftSizeof && RightSizeof) {</div><div>+ if (LeftSizeof->getKind() == UETT_SizeOf &&</div>
<div>+ RightSizeof->getKind() == UETT_SizeOf)</div><div>+ return false;</div><div>+ }</div></div><div><br></div><div>I haven't written a RecursiveASTVisitor in a while, but the documentation seems to say that returning false will abort the <i>entire</i> traversal, not just skip the current node's children. Is that correct? (Please add a test case.)</div>
<div><br></div><div>Have you run this over real code? Has it found any bugs?</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Jordan</div></font></span><div><div class="h5"><div><br></div><br><div><div>
On Oct 4, 2013, at 6:09 , Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>> wrote:</div><br><blockquote type="cite">Hi,<br><br>As i have understood it you aren't sure whether to put the checker in the compiler right away or analyzer so i let it stay in the analyzer to be evaluated. I guess it should be implemented as a warning eventually.<br>
<br>I have added a check to report when sizeof is called inside a sizeof.<br>sizeof(sizeof(int))<br><br>I also made an exception when compared against another sizeof as suggested.<br><br>//Anders<br><br>.......................................................................................................................<br>
Anders Rönnholm Senior Engineer<br>Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br><br>Mobile: <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254" target="_blank">+46 (0)70 912 42 54</a><br>
E-mail: <a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a><br><br><a href="http://www.evidente.se" target="_blank">www.evidente.se</a><br><br>________________________________________<br>
Från: David Blaikie [<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]<br>Skickat: den 26 september 2013 04:31<br>Till: Jordan Rose<br>Cc: Anders Rönnholm; Richard Smith; Eli Friedman; <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br><br>On Wed, Sep 25, 2013 at 6:39 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a><mailto:<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>>> wrote:<br>
Seems reasonable. This is something that we could also implement as a compiler warning, though. David, Richard, Eli, what do you think: compiler or analyzer?<br><br>personally I think anything cheap enough to do at compile time probably should be, but Doug's general attitude has been that the other prerequisite is bug-finding capability; Essentially if a warning would be too noisy to be on by default in an existing codebase (because it caused more busy work than bug fixing to cleanup the codebase to be warning-free) then it doesn't go in the compiler<br>
<br>Putting it in the analyzer would then seem an unfortunate side-effect of its true/false positive rate.<br><br>Of course we're still missing the middle ground: things that don't have the true positive rate to be intrinsic features of the compiler, but a codebase may want to opt-in to if the bug-finding is acceptable to them. This would come in the form of a Clang plugin, ideally, but we don't have an authoritative/configurable plugin for this purpose yet.<br>
<br><br><br>Comments on the patch itself:<br><br>+ return sizeof(2 + 1); // expected-warning{{Sizeof() on an expression with an operator may yield unexpected results}}<br><br>The rule that capitalizes analyzer output (unlike compiler warnings) makes this look very strange. Maybe it's worth inserting the word "Using" before "sizeof".<br>
<br><br>+//==- CheckSizeofOnExpression.cpp - Check expressions in sizeof() *- C++ -*-==//<br><br>The format header "-*- C++ -*-" needs all those dashes and stars to be recognized. However, since this is a .cpp file, you should just leave it out anyway and uses dashes to fill the empty space -----==//<br>
<br><br>+//<br>+// This file defines a check for expressions made in sizeof()<br>+//<br><br>Please turn this into a Doxygen file comment. <a href="http://llvm.org/docs/CodingStandards.html#file-headers" target="_blank">http://llvm.org/docs/CodingStandards.html#file-headers</a><br>
<br><br>+class WalkAST : public StmtVisitor<WalkAST> {<br><br>Other checkers predate RecursiveASTVisitor, but you might as well use RecursiveASTVisitor and avoid writing the VisitChildren boilerplate. This would also handle checking C++ constructor initializers, which aren't actually part of the body of the constructor. (We should probably migrate all the old walkers to RecursiveASTVisitor for this reason.)<br>
<br><br>+ Expr *Expr = E->getArgumentExpr();<br><br>We try not to shadow type names with local variable names, which I know is a bit trickier when variable names are also capitalized. Also, there's no reason not to make this a pointer to const.<br>
<br><br>+ //Only check binary operators<br><br>+ // is expression based on macro => don't warn<br><br>Please use complete sentences for comments, including a space after the //.<br><br><br>+ BR.EmitBasicReport(AC->getDecl(),<br>
+ "Expression with an operation in sizeof()",<br>+ "Logic",<br>+ "Sizeof() on an expression with an operator "<br>+ "may yield unexpected results.",<br>
+ ELoc, &R, 1);<br><br>The bug categories aren't well-defined, but there's already one called "Logic error" that's used by some of the analyzer core checkers. It might be nice to refactor this, SizeofPointerChecker, and BuiltinBug to share a single new constant in CommonBugCategories.{h,cpp}.<br>
<br>Also, there's an overload that takes a single SourceRange. (Double-also, BugReporter should just be using ArrayRef, which autoconverts from a single element anyway.)<br><br><br>+ walker.Visit(D->getBody());<br>
<br>If you do change to RecursiveASTVisitor, make sure you visit D, to include the constructor initializers.<br><br>Thanks for working on this!<br>Jordan<br><br><br>On Sep 20, 2013, at 2:37 , Anders Rönnholm <<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>>> wrote:<br>
<br><blockquote type="cite">Hi,<br><br>Sorry I broke a test with a last minute change.<br><br>Thanks,<br>Anders<br><br>.......................................................................................................................<br>
Anders Rönnholm Senior Engineer<br>Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br><br>Mobile: <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254" target="_blank">+46 (0)70 912 42 54</a><tel:%2B46%20%280%2970%20912%2042%2054><br>
E-mail: <a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>><br>
<br><a href="http://www.evidente.se" target="_blank">www.evidente.se</a><<a href="http://www.evidente.se" target="_blank">http://www.evidente.se</a>><br><br>________________________________________<br>Från: Anders Rönnholm<br>
Skickat: den 20 september 2013 11:19<br>Till: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>><br>
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br><br>Hi,<br><br>I forgot that there is i line limit of 80, i have corrected that.<br><br>//Anders<br><br>.......................................................................................................................<br>
Anders Rönnholm Senior Engineer<br>Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br><br>Mobile: <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254" target="_blank">+46 (0)70 912 42 54</a><tel:%2B46%20%280%2970%20912%2042%2054><br>
E-mail: <a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>><br>
<br><a href="http://www.evidente.se" target="_blank">www.evidente.se</a><<a href="http://www.evidente.se" target="_blank">http://www.evidente.se</a>><br><br>________________________________________<br>Från: Anders Rönnholm<br>
Skickat: den 20 september 2013 10:23<br>Till: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>><br>
Ämne: [PATCH] [StaticAnalyzer] New checker Sizeof on expression<br><br>Hi,<br><br>I have another checker i would like to get reviewed. It checks for usage of sizeof on an expression. For now it only checks binary operators. Sizeof on defines does not generate a warning.<br>
<br>Example:<br>sizeof(2 + 1);<br><br>Thanks,<br>Anders<br><br>.......................................................................................................................<br>Anders Rönnholm Senior Engineer<br>
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden<br><br>Mobile: <a href="tel:%2B46%20%280%2970%20912%2042%2054" value="+46709124254" target="_blank">+46 (0)70 912 42 54</a><tel:%2B46%20%280%2970%20912%2042%2054><br>
E-mail: <a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a><mailto:<a href="mailto:Anders.Ronnholm@evidente.se" target="_blank">Anders.Ronnholm@evidente.se</a>><br>
<br><a href="http://www.evidente.se" target="_blank">www.evidente.se</a><<a href="http://www.evidente.se" target="_blank">http://www.evidente.se</a>><br></blockquote><br><blockquote type="cite">_______________________________________________<br>
cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote><br></blockquote></div></div></div></div><br><div style="word-wrap:break-word"><div><blockquote type="cite"></blockquote></div><br></div>
<br></blockquote></div><br></div></div>