]> granicus.if.org Git - clang/commitdiff
Merge and improve code that detects same value in comparisons.
authorRichard Trieu <rtrieu@google.com>
Sat, 21 Sep 2019 03:02:26 +0000 (03:02 +0000)
committerRichard Trieu <rtrieu@google.com>
Sat, 21 Sep 2019 03:02:26 +0000 (03:02 +0000)
-Wtautological-overlap-compare and self-comparison from -Wtautological-compare
relay on detecting the same operand in different locations.  Previously, each
warning had it's own operand checker.  Now, both are merged together into
one function that each can call.  The function also now looks through member
access and array accesses.

Differential Revision: https://reviews.llvm.org/D66045

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@372453 91177308-0d34-0410-b5e6-96231b3b80d8

docs/ReleaseNotes.rst
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/compare-cxx2a.cpp
test/SemaCXX/self-comparison.cpp

index e04e568ddb0a935454bde6d13e09802643a944a1..bb93fb547770c1e56d45d4e8dc9e35557bf01367 100644 (file)
@@ -53,6 +53,9 @@ Improvements to Clang's diagnostics
 
 - -Wtautological-overlap-compare will warn on negative numbers and non-int
   types.
+- -Wtautological-compare for self comparisons and
+  -Wtautological-overlap-compare will now look through member and array
+  access to determine if two operand expressions are the same.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
index 30fa84315ff5376ff58ef03a17e3315e07ae9ec8..ffa7d4db96a4794cfc4c0de4d633c87dd369dc01 100644 (file)
@@ -906,6 +906,11 @@ public:
     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 00c6a8170406f14d2bc6538999772c0778dacda4..b5fb7fafb00a28042a9ace9e7ab9e9d59d2f441c 100644 (file)
@@ -3921,6 +3921,111 @@ bool Expr::refersToGlobalRegisterVar() const {
   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 (const auto *DRE = dyn_cast<DeclRefExpr>(E))
+          return DRE->getDecl();
+        if (const 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 86816997f0719dcdc60149bf4b86b4707fea84da..687cf5705913839657bf6edce682062661a62aab 100644 (file)
@@ -105,12 +105,12 @@ static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
   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();
 
@@ -132,8 +132,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
     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
@@ -1045,34 +1044,34 @@ private:
     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 808c0e450a558d1c7da93942530653606c39cb63..6dfbc4cda598bfaacb5e14cdafc9d32439b41f3b 100644 (file)
@@ -10245,20 +10245,18 @@ static void diagnoseLogicalNotOnLHSofCheck(Sema &S, ExprResult &LHS,
       << 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.
@@ -10291,8 +10289,6 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
   // 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 {
@@ -10301,7 +10297,8 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
     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:
@@ -10321,9 +10318,7 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
                           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 cfb57d39242db285964a88d5483f151eb60a68e9..1b9fa3e8db55c6a5c7de159736a3d3b90f32362c 100644 (file)
@@ -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 d72e60755db7610ec07e88b081fd00802528398e..066312591c1eba0c1473ab50287fd7c4f929516c 100644 (file)
@@ -158,3 +158,17 @@ int no_warning(unsigned x) {
   return x >= 0 || x == 1;
   // no warning since "x >= 0" is caught by a different tautological warning.
 }
+
+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 c68a0ae9133f3df87770c0f3e4c452467032a5ce..b6e7fc806114bdf32c8c1ad00c3f2ae7acb1dfa8 100644 (file)
@@ -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 ac129b68a67a9219514222a9d30ae64791e98606..4ea16665f1985584b2e11a6e26eebc5323c001a0 100644 (file)
@@ -40,3 +40,84 @@ bool g() {
     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;
+};
+
+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