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: [email protected]
www.evidente.se
________________________________________
Från: David Blaikie [[email protected]]
Skickat: den 26 september 2013 04:31
Till: Jordan Rose
Cc: Anders Rönnholm; Richard Smith; Eli Friedman; [email protected]
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
On Wed, Sep 25, 2013 at 6:39 PM, Jordan Rose
<[email protected]<mailto:[email protected]>> 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
<[email protected]<mailto:[email protected]>> 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:
> [email protected]<mailto:[email protected]>
>
> www.evidente.se<http://www.evidente.se>
>
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 20 september 2013 11:19
> Till: [email protected]<mailto:[email protected]>
> Ä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:
> [email protected]<mailto:[email protected]>
>
> www.evidente.se<http://www.evidente.se>
>
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 20 september 2013 10:23
> Till: [email protected]<mailto:[email protected]>
> Ä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:
> [email protected]<mailto:[email protected]>
>
> www.evidente.se<http://www.evidente.se>
> _______________________________________________
> cfe-commits mailing list
> [email protected]<mailto:[email protected]>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Index: lib/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- lib/StaticAnalyzer/Checkers/Checkers.td (revision 191941)
+++ lib/StaticAnalyzer/Checkers/Checkers.td (working copy)
@@ -112,6 +112,10 @@
HelpText<"Check for pointer subtractions on two pointers pointing to different memory chunks">,
DescFile<"PointerSubChecker">;
+def SizeofOnExpressionChecker : Checker<"SizeofOnExpression">,
+ HelpText<"Check for expression in sizeof()">,
+ DescFile<"CheckSizeofOnExpression.cpp">;
+
def SizeofPointerChecker : Checker<"SizeofPtr">,
HelpText<"Warn about unintended use of sizeof() on pointer expressions">,
DescFile<"CheckSizeofPointer.cpp">;
Index: lib/StaticAnalyzer/Checkers/CheckSizeofOnExpression.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CheckSizeofOnExpression.cpp (revision 0)
+++ lib/StaticAnalyzer/Checkers/CheckSizeofOnExpression.cpp (working copy)
@@ -0,0 +1,123 @@
+//==- CheckSizeofOnExpression.cpp - Check expressions in sizeof()-----------==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// \brief This file defines a check for expressions made in sizeof()
+///
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class SizeofOnExpressionVisitor
+ : public RecursiveASTVisitor<SizeofOnExpressionVisitor> {
+public:
+ explicit SizeofOnExpressionVisitor(BugReporter &br, AnalysisDeclContext *ac)
+ : BR(br), AC(ac) {}
+
+ bool VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E) {
+ if (E->getKind() != UETT_SizeOf)
+ return true;
+
+ if (E->isArgumentType())
+ return true;
+
+ const Expr *ArgExpr = E->getArgumentExpr();
+ if (!ArgExpr)
+ return true;
+
+ if (isa<BinaryOperator>(ArgExpr->IgnoreParens())) {
+ const BinaryOperator *Binop =
+ dyn_cast<BinaryOperator>(ArgExpr->IgnoreParens());
+
+ // Don't warn if expression is based on macro.
+ if ((Binop->getLHS()->getExprLoc().isMacroID()) ||
+ (Binop->getRHS()->getExprLoc().isMacroID()))
+ return true;
+
+ SourceRange R = ArgExpr->getSourceRange();
+ PathDiagnosticLocation ELoc =
+ PathDiagnosticLocation::createBegin(E, BR.getSourceManager(), AC);
+
+ 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);
+ return false;
+ } else if (isa<UnaryExprOrTypeTraitExpr>(ArgExpr->IgnoreParens())) {
+ const UnaryExprOrTypeTraitExpr *UnaryExpr =
+ dyn_cast<UnaryExprOrTypeTraitExpr>(ArgExpr->IgnoreParens());
+ if (UnaryExpr->getKind() != UETT_SizeOf)
+ return true;
+
+ SourceRange R = ArgExpr->getSourceRange();
+ PathDiagnosticLocation ELoc =
+ PathDiagnosticLocation::createBegin(E, BR.getSourceManager(), AC);
+
+ BR.EmitBasicReport(AC->getDecl(), "Sizeof() call in sizeof()",
+ categories::LogicError, "Using sizeof() on sizeof() "
+ "may yield unexpected results.", ELoc, &R, 1);
+ return false;
+ }
+
+ return true;
+ }
+
+ bool VisitBinaryOperator(BinaryOperator *B) {
+ // Don't warn on comparisons between two sizeof.
+
+ if (!BinaryOperator::isComparisonOp(B->getOpcode()))
+ return true;
+
+ const UnaryExprOrTypeTraitExpr *LeftSizeof =
+ dyn_cast<UnaryExprOrTypeTraitExpr>(B->getLHS()->IgnoreImpCasts());
+ const UnaryExprOrTypeTraitExpr *RightSizeof =
+ dyn_cast<UnaryExprOrTypeTraitExpr>(B->getRHS()->IgnoreImpCasts());
+
+ if (LeftSizeof && RightSizeof) {
+ if (LeftSizeof->getKind() == UETT_SizeOf &&
+ RightSizeof->getKind() == UETT_SizeOf)
+ return false;
+ }
+
+ return true;
+ }
+
+private:
+ BugReporter &BR;
+ AnalysisDeclContext *AC;
+};
+}
+
+//===----------------------------------------------------------------------===//
+// SizeofOnExpressionChecker
+//===----------------------------------------------------------------------===//
+
+namespace {
+class SizeofOnExpressionChecker : public Checker<check::ASTCodeBody> {
+public:
+ void checkASTCodeBody(const Decl *D, AnalysisManager &mgr,
+ BugReporter &BR) const {
+ SizeofOnExpressionVisitor Visitor(BR, mgr.getAnalysisDeclContext(D));
+ Visitor.TraverseDecl(const_cast<Decl *>(D));
+ }
+};
+}
+
+void ento::registerSizeofOnExpressionChecker(CheckerManager &mgr) {
+ mgr.registerChecker<SizeofOnExpressionChecker>();
+}
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt (revision 191941)
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt (working copy)
@@ -11,6 +11,7 @@
BasicObjCFoundationChecks.cpp
BoolAssignmentChecker.cpp
BuiltinFunctionChecker.cpp
+ CheckSizeofOnExpression.cpp
CStringChecker.cpp
CStringSyntaxChecker.cpp
CallAndMessageChecker.cpp
Index: test/Analysis/sizeofonexpression.cpp
===================================================================
--- test/Analysis/sizeofonexpression.cpp (revision 0)
+++ test/Analysis/sizeofonexpression.cpp (working copy)
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.SizeofOnExpression -verify %s
+class A {
+public:
+ A(): a(1) { }
+ A(int p) : a(sizeof(p + 1)) { } // expected-warning{{Using sizeof() on an expression with an operator may yield unexpected results}}
+ int a;
+};
Index: test/Analysis/sizeofonexpression.c
===================================================================
--- test/Analysis/sizeofonexpression.c (revision 0)
+++ test/Analysis/sizeofonexpression.c (working copy)
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.SizeofOnExpression -verify %s
+
+#define DEF (1+2)
+
+struct s {
+ int xx;
+ int yy;
+};
+
+int SizeofStructExpression(struct s p) {
+ return sizeof(sizeof(p) + 1); // expected-warning{{Using sizeof() on an expression with an operator may yield unexpected results}}
+}
+
+int SizeofStructMemberExpression(struct s p) {
+ return sizeof(p.xx * p.yy); // expected-warning{{Using sizeof() on an expression with an operator may yield unexpected results}}
+}
+
+int SizeofLiteralExpression() {
+ return sizeof(2 + 1); // expected-warning{{Using sizeof() on an expression with an operator may yield unexpected results}}
+}
+
+int SizeofRefExpression() {
+ int x = 1, y = 2;
+ return sizeof(x / y); // expected-warning{{Using sizeof() on an expression with an operator may yield unexpected results}}
+}
+
+int SizeofDefine() {
+ return sizeof(DEF);
+} // no-warning
+
+int SizeofDefineExpression() {
+ return sizeof(DEF + 2);
+} // no-warning
+
+int SizeofDefineExpression2() {
+ return sizeof(2 - DEF);
+} // no-warning
+
+int SizeofFunctionCallExpression() {
+ return sizeof(SizeofDefine() - 1); // expected-warning{{Using sizeof() on an expression with an operator may yield unexpected results}}
+}
+
+int SizeofSizeof() {
+ return sizeof(sizeof(1)); // expected-warning{{Using sizeof() on sizeof() may yield unexpected results}}
+}
+
+int SizeofComparison() {
+ return sizeof(1+1) == sizeof(1+1);
+}// no-warning
+
+int SizeofComparison2() {
+ return sizeof(1+1) == 1; // expected-warning{{Using sizeof() on an expression with an operator may yield unexpected results}}
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
