[Sema] Diagnose tautological bounds checks (#120222)

This diagnoses comparisons like `ptr + unsigned_index < ptr` and `ptr +
unsigned_index >= ptr`, which are always false/true because addition of
a pointer and an unsigned index cannot wrap (or the behavior is
undefined).

This warning is intended to help find broken bounds checks (which must
be implemented in terms of uintptr_t instead).

Fixes #120214.

Author: nikic

clang/docs/ReleaseNotes.rst

@@ -683,6 +683,16 @@ Improvements to Clang's diagnostics
       views.push_back(std::string("123")); // warning
     }
 
+- Clang now emits a ``-Wtautological-compare`` diagnostic when a check for
+  pointer addition overflow is always true or false, because overflow would
+  be undefined behavior.
+
+  .. code-block:: c++
+
+    bool incorrect_overflow_check(const char *ptr, size_t index) {
+      return ptr + index < ptr; // warning
+    }
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -10246,7 +10246,7 @@ def warn_dangling_reference_captured_by_unknown : Warning<
 // should result in a warning, since these always evaluate to a constant.
 // Array comparisons have similar warnings
 def warn_comparison_always : Warning<
-  "%select{self-|array }0comparison always evaluates to "
+  "%select{self-|array |pointer }0comparison always evaluates to "
   "%select{a constant|true|false|'std::strong_ordering::equal'}1">,
   InGroup<TautologicalCompare>;
 def warn_comparison_bitwise_always : Warning<

clang/lib/Sema/SemaExpr.cpp

@@ -11786,6 +11786,50 @@ static bool checkForArray(const Expr *E) {
   return D->getType()->isArrayType() && !D->isWeak();
 }
 
+/// Detect patterns ptr + size >= ptr and ptr + size < ptr, where ptr is a
+/// pointer and size is an unsigned integer. Return whether the result is
+/// always true/false.
+static std::optional<bool> isTautologicalBoundsCheck(const Expr *LHS,
+                                                     const Expr *RHS,
+                                                     BinaryOperatorKind Opc) {
+  if (!LHS->getType()->isPointerType())
+    return std::nullopt;
+
+  // Canonicalize to >= or < predicate.
+  switch (Opc) {
+  case BO_GE:
+  case BO_LT:
+    break;
+  case BO_GT:
+    std::swap(LHS, RHS);
+    Opc = BO_LT;
+    break;
+  case BO_LE:
+    std::swap(LHS, RHS);
+    Opc = BO_GE;
+    break;
+  default:
+    return std::nullopt;
+  }
+
+  auto *BO = dyn_cast<BinaryOperator>(LHS);
+  if (!BO || BO->getOpcode() != BO_Add)
+    return std::nullopt;
+
+  Expr *Other;
+  if (Expr::isSameComparisonOperand(BO->getLHS(), RHS))
+    Other = BO->getRHS();
+  else if (Expr::isSameComparisonOperand(BO->getRHS(), RHS))
+    Other = BO->getLHS();
+  else
+    return std::nullopt;
+
+  if (!Other->getType()->isUnsignedIntegerType())
+    return std::nullopt;
+
+  return Opc == BO_GE;
+}
+
 /// Diagnose some forms of syntactically-obvious tautological comparison.
 static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
                                            Expr *LHS, Expr *RHS,
@@ -11895,6 +11939,12 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
                             S.PDiag(diag::warn_comparison_always)
                                 << 1 /*array comparison*/
                                 << Result);
+    } else if (std::optional<bool> Res =
+                   isTautologicalBoundsCheck(LHS, RHS, Opc)) {
+      S.DiagRuntimeBehavior(Loc, nullptr,
+                            S.PDiag(diag::warn_comparison_always)
+                                << 2 /*pointer comparison*/
+                                << (*Res ? AlwaysTrue : AlwaysFalse));
     }
   }
 

clang/test/Sema/tautological-pointer-comparison.c

@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int add_ptr_idx_ult_ptr(const char *ptr, unsigned index) {
+  return ptr + index < ptr; // expected-warning {{pointer comparison always evaluates to false}}
+}
+
+int add_idx_ptr_ult_ptr(const char *ptr, unsigned index) {
+  return index + ptr < ptr; // expected-warning {{pointer comparison always evaluates to false}}
+}
+
+int ptr_ugt_add_ptr_idx(const char *ptr, unsigned index) {
+  return ptr > ptr + index; // expected-warning {{pointer comparison always evaluates to false}}
+}
+
+int ptr_ugt_add_idx_ptr(const char *ptr, unsigned index) {
+  return ptr > index + ptr; // expected-warning {{pointer comparison always evaluates to false}}
+}
+
+int add_ptr_idx_uge_ptr(const char *ptr, unsigned index) {
+  return ptr + index >= ptr; // expected-warning {{pointer comparison always evaluates to true}}
+}
+
+int add_idx_ptr_uge_ptr(const char *ptr, unsigned index) {
+  return index + ptr >= ptr; // expected-warning {{pointer comparison always evaluates to true}}
+}
+
+int ptr_ule_add_ptr_idx(const char *ptr, unsigned index) {
+  return ptr <= ptr + index; // expected-warning {{pointer comparison always evaluates to true}}
+}
+
+int ptr_ule_add_idx_ptr(const char *ptr, unsigned index) {
+  return ptr <= index + ptr; // expected-warning {{pointer comparison always evaluates to true}}
+}
+
+int add_ptr_idx_ult_ptr_array(unsigned index) {
+  char ptr[10];
+  return ptr + index < ptr; // expected-warning {{pointer comparison always evaluates to false}}
+}
+
+// Negative tests with wrong predicate.
+
+int add_ptr_idx_ule_ptr(const char *ptr, unsigned index) {
+  return ptr + index <= ptr;
+}
+
+int add_ptr_idx_ugt_ptr(const char *ptr, unsigned index) {
+  return ptr + index > ptr;
+}
+
+int ptr_uge_add_idx_ptr(const char *ptr, unsigned index) {
+  return ptr >= index + ptr;
+}
+
+int ptr_ult_add_idx_ptr(const char *ptr, unsigned index) {
+  return ptr < index + ptr;
+}
+
+// Negative test with signed index.
+
+int add_ptr_idx_ult_ptr_signed(const char *ptr, int index) {
+  return ptr + index < ptr;
+}
+
+// Negative test with unrelated pointers. 
+
+int add_ptr_idx_ult_ptr2(const char *ptr, const char *ptr2, unsigned index) {
+  return ptr + index < ptr2;
+}
+
+// Negative test with non-pointer operands.
+
+int add_ptr_idx_ult_ptr_not_pointer(unsigned ptr, unsigned index) {
+  return ptr + index < ptr;
+}