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; 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(E1); + const auto *DRE2 = cast(E2); + return DRE1->isRValue() && DRE2->isRValue() && + DRE1->getDecl() == DRE2->getDecl(); + } + case ImplicitCastExprClass: { + // Peel off implicit casts. + while (true) { + const auto *ICE1 = dyn_cast(E1); + const auto *ICE2 = dyn_cast(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(E1); + const auto *DRE2 = dyn_cast(E2); + if (DRE1 && DRE2) + return declaresSameEntity(DRE1->getDecl(), DRE2->getDecl()); + + const auto *Ivar1 = dyn_cast(E1); + const auto *Ivar2 = dyn_cast(E2); + if (Ivar1 && Ivar2) { + return Ivar1->isFreeIvar() && Ivar2->isFreeIvar() && + declaresSameEntity(Ivar1->getDecl(), Ivar2->getDecl()); + } + + const auto *Array1 = dyn_cast(E1); + const auto *Array2 = dyn_cast(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(Idx1); + const auto Integer2 = dyn_cast(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(E1) && isa(E2)) { + const auto *ME1 = cast(E1); + const auto *ME2 = cast(E2); + if (!declaresSameEntity(ME1->getMemberDecl(), ME2->getMemberDecl())) + return false; + if (const auto *D = dyn_cast(ME1->getMemberDecl())) + if (D->isStaticDataMember()) + return true; + E1 = ME1->getBase()->IgnoreParenImpCasts(); + E2 = ME2->getBase()->IgnoreParenImpCasts(); + } + + if (isa(E1) && isa(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(E)) + return DRE->getDecl(); + if (auto *ME = dyn_cast(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: 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 +static std::tuple tryNormalizeBinaryOperator(const BinaryOperator *B) { BinaryOperatorKind Op = B->getOpcode(); @@ -108,8 +108,7 @@ Constant = tryTransformToIntOrEnumConstant(B->getLHS()); } - auto *D = dyn_cast(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/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(E)) - return DR->getDecl(); - if (ObjCIvarRefExpr *Ivar = dyn_cast(E)) { - if (Ivar->isFreeIvar()) - return Ivar->getDecl(); - } - if (MemberExpr *Mem = dyn_cast(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(E)) { + D = DR->getDecl(); + } else if (const MemberExpr *Mem = dyn_cast(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: 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: 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/SemaCXX/compare-cxx2a.cpp =================================================================== --- test/SemaCXX/compare-cxx2a.cpp +++ test/SemaCXX/compare-cxx2a.cpp @@ -8,12 +8,18 @@ #define ASSERT_TYPE(...) static_assert(__is_same(__VA_ARGS__)) #define ASSERT_EXPR_TYPE(Expr, Expect) static_assert(__is_same(decltype(Expr), Expect)); +struct S { + static int x[5]; +}; + void self_compare() { int a; int *b = nullptr; + S s; (void)(a <=> a); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}} (void)(b <=> b); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}} + (void)(s.x[a] <=> S::x[a]); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}} } void test0(long a, unsigned long b) { Index: test/SemaCXX/self-comparison.cpp =================================================================== --- test/SemaCXX/self-comparison.cpp +++ test/SemaCXX/self-comparison.cpp @@ -40,3 +40,84 @@ Y::n == Y::n; } template bool g(); // 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; +}; + +struct U { + bool operator!=(const U&); +}; + +bool operator==(const U&, const U&); + +// May want to warn on this in the future. +int user_defined(U u) { + return u == u; + return u != u; +} + +} // namespace member_tests