I'm fine with this staying in the analyzer for now, unless David, Richard, or Eli feel it should be a warning right away. + 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). 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 <[email protected]> wrote: Hi, |
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
