danielmarjamaki updated this revision to Diff 41858.
danielmarjamaki added a comment.
Another diff.
This time I have stripped the checker to make it simpler to review, triage, etc.
It will now only warn about loss of precision in assignments when RHS is a
variable.
For me, triaging these results are time consuming. It's not obvious at first
glance why there is warning. I have tried to strip down the source files to
smaller testcases to see where it "goes wrong" and then very often saw that the
warning was a TP.
When scanning 692 projects with this checker I got 56 warnings. I've triaged 21
random warnings of these so far and saw 20 TP and 1 FP.
When I have triaged the results there is one kind of "possible FP" that I see,
like this:
unsigned long x = largevalue;
bytes[0] = x; // Possible FP here. Technically it's a TP but it's an loss of
precision by design.
bytes[1] = x >> 8;
....
I am currently considering to allow this FP. Any opinions about allowing it?
http://reviews.llvm.org/D13126
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
test/Analysis/conversion.c
Index: test/Analysis/conversion.c
===================================================================
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.Conversion -verify %s
+
+unsigned char U8;
+signed char S8;
+
+void assign1() {
+ unsigned X = 1000;
+ U8 = X; // expected-warning {{Loss of precision}}
+}
+
+// don't warn for macros
+#define DOSTUFF ({ unsigned X = 1000; U8 = X; })
+void dontwarn1() {
+ DOSTUFF;
+}
+
+// don't warn for calculations
+// seen some fp. For instance: c2 = (c2 >= 'A' && c2 <= 'Z') ? c2 - 'A' + 'a' : c2;
+// there is a todo in the checker to handle calculations
+void dontwarn2() {
+ signed S = -32;
+ U8 = S+10;
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -0,0 +1,165 @@
+//=== ConversionChecker.cpp -------------------------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines ConversionChecker that warns about dangerous conversions where
+// there is possible loss of precision.
+//
+//===----------------------------------------------------------------------===//
+#include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class ConversionChecker : public Checker<check::PreStmt<ImplicitCastExpr>> {
+ mutable std::unique_ptr<BuiltinBug> BT;
+
+public:
+ void checkPreStmt(const ImplicitCastExpr *Cast, CheckerContext &C) const {
+ // TODO: For now we only warn about DeclRefExpr, to avoid noise. Warn for
+ // calculations also.
+ if (!isa<DeclRefExpr>(Cast->IgnoreParenImpCasts()))
+ return;
+
+ // Don't warn for loss of precision in macros
+ if (Cast->getExprLoc().isMacroID())
+ return;
+
+ const ParentMap &PM = C.getLocationContext()->getParentMap();
+ const Stmt *Parent = PM.getParent(Cast);
+ if (!Parent)
+ return;
+
+ const BinaryOperator *B = dyn_cast<BinaryOperator>(Parent);
+ if (!B)
+ return;
+
+ BinaryOperator::Opcode Opc = B->getOpcode();
+ if (Opc == BO_Assign || Opc == BO_MulAssign)
+ diagnoseLossOfPrecision(Cast, C);
+ }
+
+private:
+ void diagnoseLossOfPrecision(const ImplicitCastExpr *Cast,
+ CheckerContext &C) const;
+
+ void reportBug(CheckerContext &C, const char Msg[]) const {
+ // Generate an error node.
+ ExplodedNode *N = C.generateErrorNode(C.getState());
+ if (!N)
+ return;
+
+ if (!BT)
+ BT.reset(new BuiltinBug(this, "Conversion", "Loss of sign/precision."));
+
+ // Generate a report for this bug.
+ auto R = llvm::make_unique<BugReport>(*BT, Msg, N);
+ C.emitReport(std::move(R));
+ }
+};
+}
+
+static bool isSigned(const Expr *E) {
+ const Type *T = E ? E->getType().getTypePtr() : nullptr;
+ return T && T->isSignedIntegerType();
+}
+
+// Can E value be greater or equal than Val?
+static bool canBeGreaterEqual(CheckerContext &C, const Expr *E,
+ unsigned long long Val) {
+ ProgramStateRef State = C.getState();
+ SVal EVal = State->getSVal(E, C.getLocationContext());
+ if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>())
+ return false;
+ DefinedSVal DefinedEVal = EVal.castAs<DefinedSVal>();
+
+ SValBuilder &Bldr = C.getSValBuilder();
+ DefinedSVal V = Bldr.makeIntVal(Val, C.getASTContext().LongLongTy);
+
+ // Is DefinedEVal less than V?
+ SVal GE =
+ Bldr.evalBinOp(State, BO_GE, DefinedEVal, V, Bldr.getConditionType());
+ if (GE.isUnknownOrUndef())
+ return false;
+ ProgramStateRef StGE = State->assume(GE.castAs<DefinedSVal>(), true);
+ ProgramStateRef StLT = State->assume(GE.castAs<DefinedSVal>(), false);
+ return (StGE && !StLT);
+}
+
+// Can E have negative value?
+static bool canBeNegative(CheckerContext &C, const Expr *E) {
+ ProgramStateRef State = C.getState();
+ SVal EVal = State->getSVal(E, C.getLocationContext());
+ if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>())
+ return false;
+ DefinedSVal DefinedEVal = EVal.castAs<DefinedSVal>();
+
+ SValBuilder &Bldr = C.getSValBuilder();
+ DefinedSVal V = Bldr.makeIntVal(0, false);
+
+ SVal LT =
+ Bldr.evalBinOp(State, BO_LT, DefinedEVal, V, Bldr.getConditionType());
+
+ // Is E value greater than MaxVal?
+ ProgramStateRef StNegative = State->assume(LT.castAs<DefinedSVal>(), true);
+ ProgramStateRef StPositive = State->assume(LT.castAs<DefinedSVal>(), false);
+
+ return (StNegative && !StPositive);
+}
+
+static bool isConstant(const Expr *E, const ASTContext &Ctx) {
+ E = E->IgnoreParenCasts();
+ if (const auto *B = dyn_cast<BinaryOperator>(E))
+ return isConstant(B->getLHS(), Ctx) && isConstant(B->getRHS(), Ctx);
+ if (const auto *C = dyn_cast<ConditionalOperator>(E))
+ return isConstant(C->getTrueExpr(), Ctx) &&
+ isConstant(C->getFalseExpr(), Ctx);
+ if (const auto *U = dyn_cast<UnaryOperator>(E))
+ return isConstant(U->getSubExpr(), Ctx);
+ return E->isEvaluatable(Ctx);
+}
+
+void ConversionChecker::diagnoseLossOfPrecision(const ImplicitCastExpr *Cast,
+ CheckerContext &C) const {
+ // Don't warn about explicit conversions in assignments.
+ if (isConstant(Cast, C.getASTContext()))
+ return;
+
+ QualType ResultType = Cast->getType();
+ QualType SubType = Cast->IgnoreParenCasts()->getType();
+
+ if (!SubType.getTypePtr()->isIntegerType() ||
+ !ResultType.getTypePtr()->isIntegerType())
+ return;
+
+ if (C.getASTContext().getIntWidth(ResultType) >=
+ C.getASTContext().getIntWidth(SubType))
+ return;
+
+ unsigned W = C.getASTContext().getIntWidth(ResultType);
+ if (W == 1 || W >= 64U)
+ return;
+
+ if (isSigned(Cast->IgnoreParenImpCasts()) &&
+ canBeNegative(C, Cast->getSubExpr()))
+ return;
+
+ unsigned long long MaxVal = 1ULL << W;
+ if (canBeGreaterEqual(C, Cast->getSubExpr(), MaxVal))
+ reportBug(C, "Loss of precision, value too high");
+}
+
+void ento::registerConversionChecker(CheckerManager &mgr) {
+ mgr.registerChecker<ConversionChecker>();
+}
Index: lib/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- lib/StaticAnalyzer/Checkers/Checkers.td
+++ lib/StaticAnalyzer/Checkers/Checkers.td
@@ -127,6 +127,10 @@
HelpText<"Check for cast from non-struct pointer to struct pointer">,
DescFile<"CastToStructChecker.cpp">;
+def ConversionChecker : Checker<"Conversion">,
+ HelpText<"Loss of sign/precision in conversions">,
+ DescFile<"ConversionChecker.cpp">;
+
def IdenticalExprChecker : Checker<"IdenticalExpr">,
HelpText<"Warn about unintended use of identical expressions in operators">,
DescFile<"IdenticalExprChecker.cpp">;
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -27,6 +27,7 @@
CheckerDocumentation.cpp
ChrootChecker.cpp
ClangCheckers.cpp
+ ConversionChecker.cpp
DeadStoresChecker.cpp
DebugCheckers.cpp
DereferenceChecker.cpp
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
