[clang][analyzer] Remove array bounds check from PointerSubChecker (#102580)

At pointer subtraction only pointers are allowed that point into an
array (or one after the end), this fact was checker by the checker. This
check is now removed because it is a special case of array indexing
error that is handled by different checkers (like ArrayBoundsV2).

Author: balazske

clang/docs/analyzer/checkers.rst

@@ -2501,7 +2501,11 @@ alpha.core.PointerSub (C)
 Check for pointer subtractions on two pointers pointing to different memory
 chunks. According to the C standard §6.5.6 only subtraction of pointers that
 point into (or one past the end) the same array object is valid (for this
-purpose non-array variables are like arrays of size 1).
+purpose non-array variables are like arrays of size 1). This checker only
+searches for different memory objects at subtraction, but does not check if the
+array index is correct. Furthermore, only cases are reported where
+stack-allocated objects are involved (no warnings on pointers to memory
+allocated by `malloc`).
 
 .. code-block:: c
 
@@ -2511,11 +2515,6 @@ purpose non-array variables are like arrays of size 1).
    x = &d[4] - &c[1]; // warn: 'c' and 'd' are different arrays
    x = (&a + 1) - &a;
    x = &b - &a; // warn: 'a' and 'b' are different variables
-   x = (&a + 2) - &a; // warn: for a variable it is only valid to have a pointer
-                      // to one past the address of it
-   x = &c[10] - &c[0];
-   x = &c[11] - &c[0]; // warn: index larger than one past the end
-   x = &c[-1] - &c[0]; // warn: negative index
  }
 
  struct S {
@@ -2538,9 +2537,6 @@ offsets of members in a structure, using pointer subtractions. This is still
 undefined behavior according to the standard and code like this can be replaced
 with the `offsetof` macro.
 
-The checker only reports cases where stack-allocated objects are involved (no
-warnings on pointers to memory allocated by `malloc`).
-
 .. _alpha-core-StackAddressAsyncEscape:
 
 alpha.core.StackAddressAsyncEscape (C)

clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp

@@ -31,99 +31,12 @@ class PointerSubChecker
   const llvm::StringLiteral Msg_MemRegionDifferent =
       "Subtraction of two pointers that do not point into the same array "
       "is undefined behavior.";
-  const llvm::StringLiteral Msg_LargeArrayIndex =
-      "Using an array index greater than the array size at pointer subtraction "
-      "is undefined behavior.";
-  const llvm::StringLiteral Msg_NegativeArrayIndex =
-      "Using a negative array index at pointer subtraction "
-      "is undefined behavior.";
-  const llvm::StringLiteral Msg_BadVarIndex =
-      "Indexing the address of a variable with other than 1 at this place "
-      "is undefined behavior.";
-
-  /// Check that an array is indexed in the allowed range that is 0 to "one
-  /// after the end". The "array" can be address of a non-array variable.
-  /// @param E Expression of the pointer subtraction.
-  /// @param ElemReg An indexed region in the subtraction expression.
-  /// @param Reg Region of the other side of the expression.
-  bool checkArrayBounds(CheckerContext &C, const Expr *E,
-                        const ElementRegion *ElemReg,
-                        const MemRegion *Reg) const;
 
 public:
   void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
 };
 }
 
-static bool isArrayVar(const MemRegion *R) {
-  while (R) {
-    if (isa<VarRegion>(R))
-      return true;
-    if (const auto *ER = dyn_cast<ElementRegion>(R))
-      R = ER->getSuperRegion();
-    else
-      return false;
-  }
-  return false;
-}
-
-bool PointerSubChecker::checkArrayBounds(CheckerContext &C, const Expr *E,
-                                         const ElementRegion *ElemReg,
-                                         const MemRegion *Reg) const {
-  if (!ElemReg)
-    return true;
-
-  const MemRegion *SuperReg = ElemReg->getSuperRegion();
-  if (!isArrayVar(SuperReg))
-    return true;
-
-  auto ReportBug = [&](const llvm::StringLiteral &Msg) {
-    if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
-      auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
-      R->addRange(E->getSourceRange());
-      C.emitReport(std::move(R));
-    }
-  };
-
-  ProgramStateRef State = C.getState();
-  SValBuilder &SVB = C.getSValBuilder();
-
-  if (SuperReg == Reg) {
-    // Case like `(&x + 1) - &x`. Only 1 or 0 is allowed as index.
-    if (const llvm::APSInt *I = SVB.getKnownValue(State, ElemReg->getIndex());
-        I && (!I->isOne() && !I->isZero()))
-      ReportBug(Msg_BadVarIndex);
-    return false;
-  }
-
-  DefinedOrUnknownSVal ElemCount =
-      getDynamicElementCount(State, SuperReg, SVB, ElemReg->getElementType());
-  auto IndexTooLarge = SVB.evalBinOp(C.getState(), BO_GT, ElemReg->getIndex(),
-                                     ElemCount, SVB.getConditionType())
-                           .getAs<DefinedOrUnknownSVal>();
-  if (IndexTooLarge) {
-    ProgramStateRef S1, S2;
-    std::tie(S1, S2) = C.getState()->assume(*IndexTooLarge);
-    if (S1 && !S2) {
-      ReportBug(Msg_LargeArrayIndex);
-      return false;
-    }
-  }
-  auto IndexTooSmall = SVB.evalBinOp(State, BO_LT, ElemReg->getIndex(),
-                                     SVB.makeZeroVal(SVB.getArrayIndexType()),
-                                     SVB.getConditionType())
-                           .getAs<DefinedOrUnknownSVal>();
-  if (IndexTooSmall) {
-    ProgramStateRef S1, S2;
-    std::tie(S1, S2) = State->assume(*IndexTooSmall);
-    if (S1 && !S2) {
-      ReportBug(Msg_NegativeArrayIndex);
-      return false;
-    }
-  }
-  return true;
-}
-
 void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
                                      CheckerContext &C) const {
   // When doing pointer subtraction, if the two pointers do not point to the
@@ -151,9 +64,11 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
   const auto *ElemLR = dyn_cast<ElementRegion>(LR);
   const auto *ElemRR = dyn_cast<ElementRegion>(RR);
 
-  if (!checkArrayBounds(C, B->getLHS(), ElemLR, RR))
+  // Allow cases like "(&x + 1) - &x".
+  if (ElemLR && ElemLR->getSuperRegion() == RR)
     return;
-  if (!checkArrayBounds(C, B->getRHS(), ElemRR, LR))
+  // Allow cases like "&x - (&x + 1)".
+  if (ElemRR && ElemRR->getSuperRegion() == LR)
     return;
 
   const ValueDecl *DiffDeclL = nullptr;

clang/test/Analysis/pointer-sub-notes.c

@@ -1,22 +1,5 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.PointerSub -analyzer-output=text -verify %s
 
-void negative_1() {
-  int a[3];
-  int x = -1;
-  // FIXME: should indicate that 'x' is -1
-  int d = &a[x] - &a[0]; // expected-warning{{Using a negative array index at pointer subtraction is undefined behavior}} \
-                         // expected-note{{Using a negative array index at pointer subtraction is undefined behavior}}
-}
-
-void negative_2() {
-  int a[3];
-  int *p1 = a, *p2 = a;
-  --p2;
-  // FIXME: should indicate that 'p2' is negative
-  int d = p1 - p2; // expected-warning{{Using a negative array index at pointer subtraction is undefined behavior}} \
-                   // expected-note{{Using a negative array index at pointer subtraction is undefined behavior}}
-}
-
 void different_1() {
   int a[3]; // expected-note{{Array at the left-hand side of subtraction}}
   int b[3]; // expected-note{{Array at the right-hand side of subtraction}}

clang/test/Analysis/pointer-sub.c

@@ -9,13 +9,7 @@ void f1(void) {
   d = (&x + 1) - &x; // no-warning ('&x' is like a single-element array)
   d = &x - (&x + 1); // no-warning
   d = (&x + 0) - &x; // no-warning
-  d = (&x - 1) - &x; // expected-warning{{Indexing the address of a variable with other than 1 at this place is undefined behavior}}
-  d = (&x + 2) - &x; // expected-warning{{Indexing the address of a variable with other than 1 at this place is undefined behavior}}
-
-  d = (z + 9) - z; // no-warning (pointers to same array)
-  d = (z + 10) - z; // no-warning (pointer to "one after the end")
-  d = (z + 11) - z; // expected-warning{{Using an array index greater than the array size at pointer subtraction is undefined behavior}}
-  d = (z - 1) - z; // expected-warning{{Using a negative array index at pointer subtraction is undefined behavior}}
+  d = (z + 10) - z; // no-warning
 }
 
 void f2(void) {
@@ -28,11 +22,6 @@ void f2(void) {
   q = &b[3];
   d = q - p; // expected-warning{{Subtraction of two pointers that}}
 
-  q = a + 10;
-  d = q - p; // no warning (use of pointer to one after the end is allowed)
-  q = a + 11;
-  d = q - a; // expected-warning{{Using an array index greater than the array size at pointer subtraction is undefined behavior}}
-
   d = &a[4] - a; // no-warning
   d = &a[2] - p; // no-warning
   d = &c - p; // expected-warning{{Subtraction of two pointers that}}