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?
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]> 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: [email protected]
>
> www.evidente.se
>
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 20 september 2013 11:19
> Till: [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
> E-mail: [email protected]
>
> www.evidente.se
>
> ________________________________________
> Från: Anders Rönnholm
> Skickat: den 20 september 2013 10:23
> Till: [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
> E-mail: [email protected]
>
> www.evidente.se
Index: test/Analysis/sizeofonexpression.c
===================================================================
--- test/Analysis/sizeofonexpression.c (revision 0)
+++ test/Analysis/sizeofonexpression.c (working copy)
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=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{{Sizeof() on an expression with an operator may yield unexpected results}}
+}
+
+int SizeofStructMemberExpression(struct s p) {
+ return sizeof(p.xx * p.yy); // expected-warning{{Sizeof() on an expression with an operator may yield unexpected results}}
+}
+
+int SizeofLiteralExpression() {
+ return sizeof(2 + 1); // expected-warning{{Sizeof() on an expression with an operator may yield unexpected results}}
+}
+
+int SizeofRefExpression() {
+ int x = 1, y = 2;
+ return sizeof(x / y); // expected-warning{{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{{Sizeof() on an expression with an operator may yield unexpected results}}
+}
Index: lib/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- lib/StaticAnalyzer/Checkers/Checkers.td (revision 191075)
+++ 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,93 @@
+//==- CheckSizeofOnExpression.cpp - Check expressions in sizeof() *- C++ -*-==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines a check for expressions made in sizeof()
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class WalkAST : public StmtVisitor<WalkAST> {
+ BugReporter &BR;
+ AnalysisDeclContext* AC;
+
+public:
+ WalkAST(BugReporter &br, AnalysisDeclContext* ac) : BR(br), AC(ac) {}
+ void VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E);
+ void VisitStmt(Stmt *S) { VisitChildren(S); }
+ void VisitChildren(Stmt *S);
+};
+}
+
+void WalkAST::VisitChildren(Stmt *S) {
+ for (Stmt::child_iterator I = S->child_begin(), E = S->child_end(); I!=E; ++I)
+ if (Stmt *child = *I)
+ Visit(child);
+}
+
+
+void WalkAST::VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *E) {
+ if (E->getKind() != UETT_SizeOf)
+ return;
+
+ if (E->isArgumentType())
+ return;
+
+ Expr *Expr = E->getArgumentExpr();
+ if (!Expr)
+ return;
+
+ BinaryOperator *Binop = dyn_cast<BinaryOperator>(Expr->IgnoreParens());
+
+ //Only check binary operators
+ if (!Binop)
+ return;
+
+ // is expression based on macro => don't warn
+ if ((Binop->getLHS()->getExprLoc().isMacroID()) ||
+ (Binop->getRHS()->getExprLoc().isMacroID()))
+ return;
+
+ SourceRange R = Expr->getSourceRange();
+ PathDiagnosticLocation ELoc =
+ PathDiagnosticLocation::createBegin(E, BR.getSourceManager(), AC);
+ 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);
+}
+
+//===----------------------------------------------------------------------===//
+// SizeofOnExpressionChecker
+//===----------------------------------------------------------------------===//
+
+namespace {
+class SizeofOnExpressionChecker : public Checker<check::ASTCodeBody> {
+public:
+ void checkASTCodeBody(const Decl *D, AnalysisManager& mgr,
+ BugReporter &BR) const {
+ WalkAST walker(BR, mgr.getAnalysisDeclContext(D));
+ walker.Visit(D->getBody());
+ }
+};
+}
+
+void ento::registerSizeofOnExpressionChecker(CheckerManager &mgr) {
+ mgr.registerChecker<SizeofOnExpressionChecker>();
+}
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt (revision 191075)
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt (working copy)
@@ -19,6 +19,7 @@
CheckObjCDealloc.cpp
CheckObjCInstMethSignature.cpp
CheckSecuritySyntaxOnly.cpp
+ CheckSizeofOnExpression.cpp
CheckSizeofPointer.cpp
CheckerDocumentation.cpp
ChrootChecker.cpp
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
