[PATCH] [StaticAnalyzer] New checker Sizeof on expression

[Anders Rönnholm]

I have run it against LLVM and Libreoffice with no false positives. No real bugs found either.

//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: metafoo at gmail.com [metafoo at gmail.com] för Richard Smith [richard at metafoo.co.uk]
Skickat: den 16 maj 2014 20:07
Till: Jordan Rose
Cc: Anders Rönnholm; cfe-commits at cs.uiuc.edu; Daniel Marjamäki
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression

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<mailto: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<mailto: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<mailto:jordan_rose at apple.com>]
> Skickat: den 13 maj 2014 18:38
> Till: Anders Rönnholm
> Cc: cfe-commits at cs.uiuc.edu<mailto: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><mailto: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><mailto: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<tel:%2B46%20%280%2970%20912%2042%2054>
> E-mail:                    Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se><mailto:Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>>
>
> www.evidente.se<http://www.evidente.se><http://www.evidente.se>
>
> ________________________________________
> Från: Jordan Rose [jordan_rose at apple.com<mailto:jordan_rose at apple.com>]
> Skickat: den 31 januari 2014 18:50
> Till: Anders Rönnholm
> Cc: cfe-commits at cs.uiuc.edu<mailto: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><mailto: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><mailto: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><mailto: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<mailto: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>