[PATCH] [StaticAnalyzer] New checker Sizeof on expression

[David Blaikie]

On Wed, Sep 25, 2013 at 6:39 PM, Jordan Rose <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>
> 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
> > E-mail:                    Anders.Ronnholm at evidente.se
> >
> > www.evidente.se
> >
> > ________________________________________
> > Från: Anders Rönnholm
> > Skickat: den 20 september 2013 11:19
> > Till: 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
> > E-mail:                    Anders.Ronnholm at evidente.se
> >
> > www.evidente.se
> >
> > ________________________________________
> > Från: Anders Rönnholm
> > Skickat: den 20 september 2013 10:23
> > Till: 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
> > E-mail:                    Anders.Ronnholm at evidente.se
> >
> > www.evidente.se
>
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130925/2218ca4d/attachment.html>