[clang] [Sema] Diagnose tautological bounds checks (PR #120222)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Nikita Popov (nikic)

<details>
<summary>Changes</summary>

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 https://github.com/llvm/llvm-project/issues/120214.

---
Full diff: https://github.com/llvm/llvm-project/pull/120222.diff


3 Files Affected:

- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1-1) 
- (modified) clang/lib/Sema/SemaExpr.cpp (+49) 
- (added) clang/test/Sema/tautological-pointer-comparison.c (+69) 


``````````diff
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 9344b620779b84..d67a81f8564a8e 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/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<
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 20bf6f7f6f28ff..4e814972d5c978 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -11786,6 +11786,49 @@ 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(Expr *LHS, 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 +11938,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));
     }
   }
 
diff --git a/clang/test/Sema/tautological-pointer-comparison.c b/clang/test/Sema/tautological-pointer-comparison.c
new file mode 100644
index 00000000000000..f3f7c7da8d6907
--- /dev/null
+++ b/clang/test/Sema/tautological-pointer-comparison.c
@@ -0,0 +1,69 @@
+// 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}}
+}
+
+// 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;
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/120222