[PATCH] [StaticAnalyzer] New checker Sizeof on expression
Richard Smith
richard at metafoo.co.uk
Fri May 16 11:07:43 PDT 2014
I'm worried about this having significant numbers of false positives,
especially for an on-by-default warning. Has this been run against any
large codebases (particularly C++ ones)?
+ if (S.isSFINAEContext())
+ return;
+
+ const FunctionDecl *FD = S.getCurFunctionDecl();
+ if (FD && FD->isFunctionTemplateSpecialization())
+ return;
This looks suspicious -- we should suppress this for any template
instantiation, not just function templates. Check
ActiveTemplateInstantiations.empty() instead.
+ if (!Binop->isMultiplicativeOp() &&
+ !Binop->isAdditiveOp())
+ return;
I find this a little surprising. I'd think we should warn on this sort of
thing:
bool b = sizeof(a > 4);
... but I don't want to block the patch on potential new features.
+ << FixItHint::CreateInsertion(Binop->getLHS()->getLocStart(),
"&")
+ <<
FixItHint::CreateReplacement(SourceRange(Binop->getOperatorLoc()), "[")
+ << FixItHint::CreateInsertion(EndLoc, "]");
This fixit looks wrong (both times).
+ if (const ParenExpr *PE = dyn_cast<ParenExpr>(E)) {
//...
+ } else if (const UnaryExprOrTypeTraitExpr *UnaryExpr =
+ dyn_cast<UnaryExprOrTypeTraitExpr>(PE->getSubExpr())) {
+ if (UnaryExpr->getKind() != UETT_SizeOf)
+ return;
Is it intentional that we warn on 'sizeof (sizeof(x))' but not on 'sizeof
sizeof(x)'?
On Fri, May 16, 2014 at 9:06 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> This looks good to me. Richard, anyone else, any additional comments?
>
> Jordan
>
>
> On May 16, 2014, at 2:03 , Anders Rönnholm <Anders.Ronnholm at evidente.se>
> wrote:
>
> > Without HasSideEffects you get lots of warnings in templates. From what
> i remember there were some discussion about not warning in templates but i
> might remember wrong, it's been a while now.
> >
> > I have removed HasSideEffects now and modified the testfiles that
> started to trigg on the warning.
> >
> > Also added extra parens to silence the warning.
> >
> > //Anders
> >
> > ________________________________________
> > Från: Jordan Rose [jordan_rose at apple.com]
> > Skickat: den 13 maj 2014 18:38
> > Till: Anders Rönnholm
> > Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki
> > Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
> >
> > Sorry for letting this slip through the cracks! I know it's now been a
> month and a half, but what were the false positives you saw without the
> HasSideEffects check? For example:
> >
> > +int SizeofFunctionCallExpression() {
> > + return sizeof(SizeofDefine() - 1);
> > +} // no-warning
> >
> > This should have a warning, since the function is not called. If it
> interferes with the VLA thing Aaron brought up, though...
> >
> > I never got a response to this:
> >
> > + if (Binop->getLHS()->getType()->isArrayType() ||
> > + Binop->getLHS()->getType()->isAnyPointerType() ||
> > + Binop->getRHS()->getType()->isArrayType() ||
> > + Binop->getRHS()->getType()->isAnyPointerType())
> > + return;
> >
> > I don't think this is correct...the user is only trying to get ptrdiff_t
> if both the LHS and RHS are pointer-ish.
> >
> > Finally, how about using an extra set of parens to silence the warning?
> It's harder to typo, and we have some precedent for that.
> >
> > Jordan
> >
> >
> > On May 13, 2014, at 3:27 , Anders Rönnholm <Anders.Ronnholm at evidente.se
> <mailto:Anders.Ronnholm at evidente.se>> wrote:
> >
> > Pinging
> > ________________________________________
> > Från: Anders Rönnholm
> > Skickat: den 27 mars 2014 11:09
> > Till: Jordan Rose
> > Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>; Daniel
> Marjamäki
> > Ämne: SV: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
> >
> > New patch with new diagnostic message. I couldn't come up with a better
> wording so i'm using your suggestion. I don't know of a good way to silence
> the warning.
> >
> > I removed the check for HasSideEffects previously but had to take back.
> I noticed that the patch triggered some false positives without it.
> >
> > //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<mailto:
> Anders.Ronnholm at evidente.se>
> >
> > www.evidente.se<http://www.evidente.se>
> >
> > ________________________________________
> > Från: Jordan Rose [jordan_rose at apple.com]
> > Skickat: den 31 januari 2014 18:50
> > Till: Anders Rönnholm
> > Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki
> > Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
> >
> > Sorry to have let this slip! This is looking good, but I had one more
> thought about the diagnostic message. It says "may yield unexpected
> results", but doesn't really explain what those unexpected results are. I
> was wondering if we could work the type into the message for the operator
> case.
> >
> > "operand of sizeof is a binary expression of type %0, which may not be
> intended"
> >
> > I don't like that wording either, but at least this one makes people say
> "what? why isn't it [the type I actually want]?". Also, should there be a
> way to silence the warning?
> >
> > What do you think?
> > Jordan
> >
> >
> > On Jan 23, 2014, at 6:40 , Anders Rönnholm <Anders.Ronnholm at evidente.se
> <mailto:Anders.Ronnholm at evidente.se>> wrote:
> >
> > Hi,
> >
> > New one with comments handled.
> >
> > ________________________________________
> > Från: Jordan Rose [jordan_rose at apple.com<mailto:jordan_rose at apple.com>]
> > Skickat: den 20 december 2013 19:15
> > Till: Anders Rönnholm
> > Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>; Daniel
> Marjamäki; Anna Zaks; David Blaikie; Richard Smith; Matt Calabrese
> > Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
> >
> > On Dec 10, 2013, at 4:38 , Anders Rönnholm <Anders.Ronnholm at evidente.se
> <mailto:Anders.Ronnholm at evidente.se><mailto:Anders.Ronnholm at evidente.se>>
> wrote:
> >
> > Are you OK to commit this patch or do you see more issues?
> >
> > I'm not sure if anyone else has ideological concerns. There's always a
> flag to turn this off, I suppose.
> >
> >
> > + if (S.isSFINAEContext())
> > + return;
> >
> > Code style: extra indent?
> >
> >
> > + if(E->HasSideEffects(S.getASTContext()))
> > + return;
> >
> > sizeof doesn't evaluate its argument, so I'm not sure why you wouldn't
> want to warn here.
> >
> >
> > + const FunctionDecl *FD = S.getCurFunctionDecl();
> > + if(FD && FD->isFunctionTemplateSpecialization())
> > + return;
> >
> > Code style: space after if. (Above too, actually.)
> >
> >
> > + if (Binop->getLHS()->getType()->isArrayType() ||
> > + Binop->getLHS()->getType()->isAnyPointerType() ||
> > + Binop->getRHS()->getType()->isArrayType() ||
> > + Binop->getRHS()->getType()->isAnyPointerType())
> > + return;
> >
> > I don't think this is correct...the user is only trying to get ptrdiff_t
> if both the LHS and RHS are pointer-ish.
> >
> >
> > +def warn_sizeof_bin_op : Warning<
> > + "using sizeof() on an expression with an operator may yield
> unexpected results">,
> > + InGroup<SizeofOnExpression>;
> > +
> > +def warn_sizeof_sizeof : Warning<
> > + "using sizeof() on sizeof() may yield unexpected results.">,
> > + InGroup<SizeofOnExpression>;
> > +
> >
> > sizeof doesn't actually require parens, so we shouldn't put the parens
> in the diagnostics.
> >
> > <sizeofonexpression.diff>
> > <sizeofonexpression.diff>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140516/9418e3f5/attachment.html>
More information about the cfe-commits
mailing list