[PATCH] [StaticAnalyzer] New checker Sizeof on expression
Richard Smith
richard at metafoo.co.uk
Mon Oct 7 13:58:38 PDT 2013
On Mon, Oct 7, 2013 at 9:55 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> I'm fine with this staying in the analyzer for now, unless David, Richard,
> or Eli feel it should be a warning right away.
>
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?
What is the false/true positive ratio for bug finding here?
sizeof(expression) is a common idiom in SFINAE contexts. Is that covered
here?
sizeof(sizeof(int)) is a "traditional" way to get sizeof(size_t). Why
should we warn on that?
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)?
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.
> + if (isa<BinaryOperator>(ArgExpr->IgnoreParens())) {
> + const BinaryOperator *Binop =
> + dyn_cast<BinaryOperator>(ArgExpr->IgnoreParens());
>
> Conventionally we'd combine these two in the if-condition itself (see the LLVM
> Programmer's Manual<http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates>).
> You never need to follow isa<> with a checked dyn_cast<>, either.
>
>
> + BR.EmitBasicReport(AC->getDecl(),
> + "Expression with an operation in sizeof()",
> + categories::LogicError,
> + "Using sizeof() on an expression with an
> operator "
> + "may yield unexpected results.", ELoc, &R, 1);
>
> There's already an overload that takes a single range; please use that
> instead of a count of 1.
>
>
> + if (LeftSizeof && RightSizeof) {
> + if (LeftSizeof->getKind() == UETT_SizeOf &&
> + RightSizeof->getKind() == UETT_SizeOf)
> + return false;
> + }
>
> I haven't written a RecursiveASTVisitor in a while, but the documentation
> seems to say that returning false will abort the *entire* traversal, not
> just skip the current node's children. Is that correct? (Please add a test
> case.)
>
> Have you run this over real code? Has it found any bugs?
>
> Jordan
>
>
> On Oct 4, 2013, at 6:09 , Anders Rönnholm <Anders.Ronnholm at evidente.se>
> wrote:
>
> 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131007/d8eba253/attachment.html>
More information about the cfe-commits
mailing list