[PATCH] [StaticAnalyzer] New checker Sizeof on expression
Anders Rönnholm
Anders.Ronnholm at evidente.se
Fri Oct 4 06:09:15 PDT 2013
Hi,
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.
I have added a check to report when sizeof is called inside a sizeof.
sizeof(sizeof(int))
I also made an exception when compared against another sizeof as suggested.
//Anders
.......................................................................................................................
Anders Rönnholm Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
Mobile: +46 (0)70 912 42 54
E-mail: Anders.Ronnholm at evidente.se
www.evidente.se
________________________________________
Från: David Blaikie [dblaikie at gmail.com]
Skickat: den 26 september 2013 04:31
Till: Jordan Rose
Cc: Anders Rönnholm; Richard Smith; Eli Friedman; cfe-commits at cs.uiuc.edu
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
On Wed, Sep 25, 2013 at 6:39 PM, Jordan Rose <jordan_rose at apple.com<mailto:jordan_rose at apple.com>> wrote:
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?
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
Putting it in the analyzer would then seem an unfortunate side-effect of its true/false positive rate.
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.
Comments on the patch itself:
+ return sizeof(2 + 1); // expected-warning{{Sizeof() on an expression with an operator may yield unexpected results}}
The rule that capitalizes analyzer output (unlike compiler warnings) makes this look very strange. Maybe it's worth inserting the word "Using" before "sizeof".
+//==- CheckSizeofOnExpression.cpp - Check expressions in sizeof() *- C++ -*-==//
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 -----==//
+//
+// This file defines a check for expressions made in sizeof()
+//
Please turn this into a Doxygen file comment. http://llvm.org/docs/CodingStandards.html#file-headers
+class WalkAST : public StmtVisitor<WalkAST> {
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.)
+ Expr *Expr = E->getArgumentExpr();
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.
+ //Only check binary operators
+ // is expression based on macro => don't warn
Please use complete sentences for comments, including a space after the //.
+ BR.EmitBasicReport(AC->getDecl(),
+ "Expression with an operation in sizeof()",
+ "Logic",
+ "Sizeof() on an expression with an operator "
+ "may yield unexpected results.",
+ ELoc, &R, 1);
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}.
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.)
+ walker.Visit(D->getBody());
If you do change to RecursiveASTVisitor, make sure you visit D, to include the constructor initializers.
Thanks for working on this!
Jordan
On Sep 20, 2013, at 2:37 , Anders Rönnholm <Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>> wrote:
> Hi,
>
> Sorry I broke a test with a last minute change.
>
> Thanks,
> Anders
>
> .......................................................................................................................
> Anders Rönnholm Senior Engineer
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
>
> Mobile: +46 (0)70 912 42 54<tel:%2B46%20%280%2970%20912%2042%2054>
> E-mail: Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>
>
> www.evidente.se<http://www.evidente.se>
>
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 20 september 2013 11:19
> Till: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
> Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
>
> Hi,
>
> I forgot that there is i line limit of 80, i have corrected that.
>
> //Anders
>
> .......................................................................................................................
> Anders Rönnholm Senior Engineer
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
>
> Mobile: +46 (0)70 912 42 54<tel:%2B46%20%280%2970%20912%2042%2054>
> E-mail: Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>
>
> www.evidente.se<http://www.evidente.se>
>
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 20 september 2013 10:23
> Till: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
> Ämne: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
>
> Hi,
>
> 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.
>
> Example:
> sizeof(2 + 1);
>
> Thanks,
> Anders
>
> .......................................................................................................................
> Anders Rönnholm Senior Engineer
> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
>
> Mobile: +46 (0)70 912 42 54<tel:%2B46%20%280%2970%20912%2042%2054>
> E-mail: Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>
>
> www.evidente.se<http://www.evidente.se>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeofonexpression.diff
Type: text/x-patch
Size: 8142 bytes
Desc: sizeofonexpression.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131004/a03f4f06/attachment.bin>
More information about the cfe-commits
mailing list