rtrieu updated this revision to Diff 214752.
rtrieu added a comment.
Check array accesses.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66045/new/
https://reviews.llvm.org/D66045
Files:
include/clang/AST/Expr.h
lib/AST/Expr.cpp
lib/Analysis/CFG.cpp
lib/Sema/SemaExpr.cpp
test/Analysis/array-struct-region.cpp
test/Sema/warn-overlap.c
test/SemaCXX/self-comparison.cpp
Index: test/SemaCXX/self-comparison.cpp
===================================================================
--- test/SemaCXX/self-comparison.cpp
+++ test/SemaCXX/self-comparison.cpp
@@ -40,3 +40,71 @@
Y<T>::n == Y<U>::n;
}
template bool g<int, int>(); // should not produce any warnings
+
+namespace member_tests {
+struct B {
+ int field;
+ static int static_field;
+ int test(B b) {
+ return field == field; // expected-warning {{self-comparison always evaluates to true}}
+ return static_field == static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return static_field == b.static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return B::static_field == this->static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return this == this; // expected-warning {{self-comparison always evaluates to true}}
+
+ return field == b.field;
+ return this->field == b.field;
+ }
+};
+
+enum {
+ I0,
+ I1,
+ I2,
+};
+
+struct S {
+ int field;
+ static int static_field;
+ int array[4];
+};
+
+struct T {
+ int field;
+ static int static_field;
+ int array[4];
+ S s;
+};
+
+int struct_test(S s1, S s2, S *s3, T t) {
+ return s1.field == s1.field; // expected-warning {{self-comparison always evaluates to true}}
+ return s2.field == s2.field; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.static_field == s2.static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return S::static_field == s1.static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.array == s1.array; // expected-warning {{self-comparison always evaluates to true}}
+ return t.s.static_field == S::static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return s3->field == s3->field; // expected-warning {{self-comparison always evaluates to true}}
+ return s3->static_field == S::static_field; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.array[0] == s1.array[0]; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.array[I1] == s1.array[I1]; // expected-warning {{self-comparison always evaluates to true}}
+ return s1.array[s2.array[0]] == s1.array[s2.array[0]]; // expected-warning {{self-comparison always evaluates to true}}
+ return s3->array[t.field] == s3->array[t.field]; // expected-warning {{self-comparison always evaluates to true}}
+
+ // Try all operators
+ return t.field == t.field; // expected-warning {{self-comparison always evaluates to true}}
+ return t.field <= t.field; // expected-warning {{self-comparison always evaluates to true}}
+ return t.field >= t.field; // expected-warning {{self-comparison always evaluates to true}}
+
+ return t.field != t.field; // expected-warning {{self-comparison always evaluates to false}}
+ return t.field < t.field; // expected-warning {{self-comparison always evaluates to false}}
+ return t.field > t.field; // expected-warning {{self-comparison always evaluates to false}}
+
+ // no warning
+ return s1.field == s2.field;
+ return s2.array == s1.array;
+ return s2.array[0] == s1.array[0];
+ return s1.array[I1] == s1.array[I2];
+
+ return s1.static_field == t.static_field;
+};
+} // namespace member_tests
Index: test/Sema/warn-overlap.c
===================================================================
--- test/Sema/warn-overlap.c
+++ test/Sema/warn-overlap.c
@@ -141,3 +141,17 @@
return x < 1 || x != 0;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
}
+
+struct A {
+ int x;
+ int y;
+};
+
+int struct_test(struct A a) {
+ return a.x > 5 && a.y < 1; // no warning, different variables
+
+ return a.x > 5 && a.x < 1;
+ // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+ return a.y == 1 || a.y != 1;
+ // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
Index: test/Analysis/array-struct-region.cpp
===================================================================
--- test/Analysis/array-struct-region.cpp
+++ test/Analysis/array-struct-region.cpp
@@ -1,20 +1,26 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -x c %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -x c++ -std=c++14 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -x c++ -std=c++17 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c++ -std=c++14 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
+// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c++ -std=c++17 %s
void clang_analyzer_eval(int);
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10126,20 +10126,18 @@
<< FixItHint::CreateInsertion(SecondClose, ")");
}
-// Get the decl for a simple expression: a reference to a variable,
-// an implicit C++ field reference, or an implicit ObjC ivar reference.
-static ValueDecl *getCompareDecl(Expr *E) {
- if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
- return DR->getDecl();
- if (ObjCIvarRefExpr *Ivar = dyn_cast<ObjCIvarRefExpr>(E)) {
- if (Ivar->isFreeIvar())
- return Ivar->getDecl();
- }
- if (MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
+// Returns true if E refers to a non-weak array.
+static bool checkForArray(const Expr *E) {
+ const ValueDecl *D = nullptr;
+ if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E)) {
+ D = DR->getDecl();
+ } else if (const MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
if (Mem->isImplicitAccess())
- return Mem->getMemberDecl();
+ D = Mem->getMemberDecl();
}
- return nullptr;
+ if (!D)
+ return false;
+ return D->getType()->isArrayType() && !D->isWeak();
}
/// Diagnose some forms of syntactically-obvious tautological comparison.
@@ -10172,8 +10170,6 @@
// obvious cases in the definition of the template anyways. The idea is to
// warn when the typed comparison operator will always evaluate to the same
// result.
- ValueDecl *DL = getCompareDecl(LHSStripped);
- ValueDecl *DR = getCompareDecl(RHSStripped);
// Used for indexing into %select in warn_comparison_always
enum {
@@ -10182,7 +10178,8 @@
AlwaysFalse,
AlwaysEqual, // std::strong_ordering::equal from operator<=>
};
- if (DL && DR && declaresSameEntity(DL, DR)) {
+
+ if (Expr::isSameComparisonOperand(LHS, RHS)) {
unsigned Result;
switch (Opc) {
case BO_EQ: case BO_LE: case BO_GE:
@@ -10202,9 +10199,7 @@
S.PDiag(diag::warn_comparison_always)
<< 0 /*self-comparison*/
<< Result);
- } else if (DL && DR &&
- DL->getType()->isArrayType() && DR->getType()->isArrayType() &&
- !DL->isWeak() && !DR->isWeak()) {
+ } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
// What is it always going to evaluate to?
unsigned Result;
switch(Opc) {
Index: lib/Analysis/CFG.cpp
===================================================================
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -81,12 +81,12 @@
return nullptr;
}
-/// Tries to interpret a binary operator into `Decl Op Expr` form, if Expr is
-/// an integer literal or an enum constant.
+/// Tries to interpret a binary operator into `Expr Op NumExpr` form, if
+/// NumExpr is an integer literal or an enum constant.
///
/// If this fails, at least one of the returned DeclRefExpr or Expr will be
/// null.
-static std::tuple<const DeclRefExpr *, BinaryOperatorKind, const Expr *>
+static std::tuple<const Expr *, BinaryOperatorKind, const Expr *>
tryNormalizeBinaryOperator(const BinaryOperator *B) {
BinaryOperatorKind Op = B->getOpcode();
@@ -108,8 +108,7 @@
Constant = tryTransformToIntOrEnumConstant(B->getLHS());
}
- auto *D = dyn_cast<DeclRefExpr>(MaybeDecl->IgnoreParenImpCasts());
- return std::make_tuple(D, Op, Constant);
+ return std::make_tuple(MaybeDecl, Op, Constant);
}
/// For an expression `x == Foo && x == Bar`, this determines whether the
@@ -1017,34 +1016,34 @@
if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
return {};
- const DeclRefExpr *Decl1;
- const Expr *Expr1;
+ const Expr *DeclExpr1;
+ const Expr *NumExpr1;
BinaryOperatorKind BO1;
- std::tie(Decl1, BO1, Expr1) = tryNormalizeBinaryOperator(LHS);
+ std::tie(DeclExpr1, BO1, NumExpr1) = tryNormalizeBinaryOperator(LHS);
- if (!Decl1 || !Expr1)
+ if (!DeclExpr1 || !NumExpr1)
return {};
- const DeclRefExpr *Decl2;
- const Expr *Expr2;
+ const Expr *DeclExpr2;
+ const Expr *NumExpr2;
BinaryOperatorKind BO2;
- std::tie(Decl2, BO2, Expr2) = tryNormalizeBinaryOperator(RHS);
+ std::tie(DeclExpr2, BO2, NumExpr2) = tryNormalizeBinaryOperator(RHS);
- if (!Decl2 || !Expr2)
+ if (!DeclExpr2 || !NumExpr2)
return {};
// Check that it is the same variable on both sides.
- if (Decl1->getDecl() != Decl2->getDecl())
+ if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2))
return {};
// Make sure the user's intent is clear (e.g. they're comparing against two
// int literals, or two things from the same enum)
- if (!areExprTypesCompatible(Expr1, Expr2))
+ if (!areExprTypesCompatible(NumExpr1, NumExpr2))
return {};
Expr::EvalResult L1Result, L2Result;
- if (!Expr1->EvaluateAsInt(L1Result, *Context) ||
- !Expr2->EvaluateAsInt(L2Result, *Context))
+ if (!NumExpr1->EvaluateAsInt(L1Result, *Context) ||
+ !NumExpr2->EvaluateAsInt(L2Result, *Context))
return {};
llvm::APSInt L1 = L1Result.Val.getInt();
Index: lib/AST/Expr.cpp
===================================================================
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -3915,6 +3915,111 @@
return false;
}
+bool Expr::isSameComparisonOperand(const Expr* E1, const Expr* E2) {
+ E1 = E1->IgnoreParens();
+ E2 = E2->IgnoreParens();
+
+ if (E1->getStmtClass() != E2->getStmtClass())
+ return false;
+
+ switch (E1->getStmtClass()) {
+ default:
+ return false;
+ case CXXThisExprClass:
+ return true;
+ case DeclRefExprClass: {
+ // DeclRefExpr without an ImplicitCastExpr can happen for integral
+ // template parameters.
+ const auto *DRE1 = cast<DeclRefExpr>(E1);
+ const auto *DRE2 = cast<DeclRefExpr>(E2);
+ return DRE1->isRValue() && DRE2->isRValue() &&
+ DRE1->getDecl() == DRE2->getDecl();
+ }
+ case ImplicitCastExprClass: {
+ // Peel off implicit casts.
+ while (true) {
+ const auto *ICE1 = dyn_cast<ImplicitCastExpr>(E1);
+ const auto *ICE2 = dyn_cast<ImplicitCastExpr>(E2);
+ if (!ICE1 || !ICE2)
+ return false;
+ if (ICE1->getCastKind() != ICE2->getCastKind())
+ return false;
+ E1 = ICE1->getSubExpr()->IgnoreParens();
+ E2 = ICE2->getSubExpr()->IgnoreParens();
+ // The final cast must be one of these types.
+ if (ICE1->getCastKind() == CK_LValueToRValue ||
+ ICE1->getCastKind() == CK_ArrayToPointerDecay ||
+ ICE1->getCastKind() == CK_FunctionToPointerDecay) {
+ break;
+ }
+ }
+
+ const auto *DRE1 = dyn_cast<DeclRefExpr>(E1);
+ const auto *DRE2 = dyn_cast<DeclRefExpr>(E2);
+ if (DRE1 && DRE2)
+ return declaresSameEntity(DRE1->getDecl(), DRE2->getDecl());
+
+ const auto *Ivar1 = dyn_cast<ObjCIvarRefExpr>(E1);
+ const auto *Ivar2 = dyn_cast<ObjCIvarRefExpr>(E2);
+ if (Ivar1 && Ivar2) {
+ return Ivar1->isFreeIvar() && Ivar2->isFreeIvar() &&
+ declaresSameEntity(Ivar1->getDecl(), Ivar2->getDecl());
+ }
+
+ const auto *Array1 = dyn_cast<ArraySubscriptExpr>(E1);
+ const auto *Array2 = dyn_cast<ArraySubscriptExpr>(E2);
+ if (Array1 && Array2) {
+ if (!isSameComparisonOperand(Array1->getBase(), Array2->getBase()))
+ return false;
+
+ auto Idx1 = Array1->getIdx();
+ auto Idx2 = Array2->getIdx();
+ const auto Integer1 = dyn_cast<IntegerLiteral>(Idx1);
+ const auto Integer2 = dyn_cast<IntegerLiteral>(Idx2);
+ if (Integer1 && Integer2) {
+ if (Integer1->getValue() != Integer2->getValue())
+ return false;
+ } else {
+ if (!isSameComparisonOperand(Idx1, Idx2))
+ return false;
+ }
+
+ return true;
+ }
+
+ // Walk the MemberExpr chain.
+ while (isa<MemberExpr>(E1) && isa<MemberExpr>(E2)) {
+ const auto *ME1 = cast<MemberExpr>(E1);
+ const auto *ME2 = cast<MemberExpr>(E2);
+ if (!declaresSameEntity(ME1->getMemberDecl(), ME2->getMemberDecl()))
+ return false;
+ if (const auto *D = dyn_cast<VarDecl>(ME1->getMemberDecl()))
+ if (D->isStaticDataMember())
+ return true;
+ E1 = ME1->getBase()->IgnoreParenImpCasts();
+ E2 = ME2->getBase()->IgnoreParenImpCasts();
+ }
+
+ if (isa<CXXThisExpr>(E1) && isa<CXXThisExpr>(E2))
+ return true;
+
+ // A static member variable can end the MemberExpr chain with either
+ // a MemberExpr or a DeclRefExpr.
+ auto getAnyDecl = [](const Expr *E) -> const ValueDecl * {
+ if (auto *DRE = dyn_cast<DeclRefExpr>(E))
+ return DRE->getDecl();
+ if (auto *ME = dyn_cast<MemberExpr>(E))
+ return ME->getMemberDecl();
+ return nullptr;
+ };
+
+ const ValueDecl *VD1 = getAnyDecl(E1);
+ const ValueDecl *VD2 = getAnyDecl(E2);
+ return declaresSameEntity(VD1, VD2);
+ }
+ }
+}
+
/// isArrow - Return true if the base expression is a pointer to vector,
/// return false if the base expression is a vector.
bool ExtVectorElementExpr::isArrow() const {
Index: include/clang/AST/Expr.h
===================================================================
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -906,6 +906,11 @@
return skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
}
+ /// Checks that the two Expr's will refer to the same value as a comparison
+ /// operand. The caller must ensure that the values referenced by the Expr's
+ /// are not modified between E1 and E2 or the result my be invalid.
+ static bool isSameComparisonOperand(const Expr* E1, const Expr* E2);
+
static bool classof(const Stmt *T) {
return T->getStmtClass() >= firstExprConstant &&
T->getStmtClass() <= lastExprConstant;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
