[PATCH] D111542: [analyzer] Retrieve incomplete array extent from its redeclaration.

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1669-1711
           // The array index has to be known.
           if (auto CI = R->getIndex().getAs<nonloc::ConcreteInt>()) {
-            // If it is not an array, return Undef.
-            QualType T = VD->getType();
-            const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(T);
-            if (!CAT)
-              return UndefinedVal();
-
-            // Support one-dimensional array.
-            // C++20 [expr.add] 7.6.6.4 (excerpt):
-            //   If P points to an array element i of an array object x with n
-            //   elements, where i < 0 or i > n, the behavior is undefined.
-            //   Dereferencing is not allowed on the "one past the last
-            //   element", when i == n.
-            // Example:
-            //   const int arr[4] = {1, 2};
-            //   const int *ptr = arr;
-            //   int x0 = ptr[0]; // 1
-            //   int x1 = ptr[1]; // 2
-            //   int x2 = ptr[2]; // 0
-            //   int x3 = ptr[3]; // 0
-            //   int x4 = ptr[4]; // UB
-            // TODO: Support multidimensional array.
-            if (!isa<ConstantArrayType>(CAT->getElementType())) {
-              // One-dimensional array.
-              const llvm::APSInt &Idx = CI->getValue();
-              const auto I = static_cast<uint64_t>(Idx.getExtValue());
-              // Use `getZExtValue` because array extent can not be negative.
-              const uint64_t Extent = CAT->getSize().getZExtValue();
-              // Check for `Idx < 0`, NOT for `I < 0`, because `Idx` CAN be
-              // negative, but `I` can NOT.
-              if (Idx < 0 || I >= Extent)
-                return UndefinedVal();
-
-              // C++20 [expr.add] 9.4.17.5 (excerpt):
-              //   i-th array element is value-initialized for each k < i ≤ n,
-              //   where k is an expression-list size and n is an array extent.
-              if (I >= InitList->getNumInits())
-                return svalBuilder.makeZeroVal(R->getElementType());
-
-              // Return a constant value, if it is presented.
-              // FIXME: Support other SVals.
-              const Expr *E = InitList->getInit(I);
-              if (Optional<SVal> V = svalBuilder.getConstantVal(E))
-                return *V;
+            // We treat only ConstantArrayType.
+            const QualType T = VD->getType();
+            if (const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(T)) {
+              // Support one-dimensional array.
+              // C++20 [expr.add] 7.6.6.4 (excerpt):
----------------
martong wrote:
> Denys, I suppose we could replace the whole inner block with a function? Similarly to `getSValFromStringLiteralByIndex` from D107339 . That could greatly increase readability.
Make sense. I'll carry this block out.


================
Comment at: clang/test/Analysis/initialization.c:101-102
+
+const int glob_arr3[];              // Incomplete array declaration
+const int glob_arr3[4] = {1, 2, 3}; // Incomplete Array redeclaration
+void foo() {
----------------
steakhal wrote:
> ASDenysPetrov wrote:
> > martong wrote:
> > > I'd like to see some more elaborate test cases. Notably
> > > ```
> > > const int glob_arr3[];              // Incomplete array declaration
> > > const int glob_arr3[4] = {1, 2, 3}; // Incomplete Array redeclaration
> > > const int glob_arr3[];              // Incomplete array redeclaration
> > > ```
> > > here neither the canonical nor the most recent decl have the initexpr.
> > > And I think this is what @balazske tried to point out.
> > Exactly. I'll add this particular case, but I should mention that **AST** surprisingly shows the third redeclaration as `ConstantArrayType` with the extent. Thus, it works for the current fix.
> > ```
> > |-VarDecl 0xc0725b0 <line:6:1, col:21> col:11 glob_arr3 'const int []'
> > |-VarDecl 0xc072700 prev 0xc0725b0 <line:7:1, col:34> col:11 glob_arr3 'const int [4]' cinit
> > | `-InitListExpr 0xc072830 <col:26, col:34> 'const int [4]'
> > |   |-array_filler: ImplicitValueInitExpr 0xc0728a8 <<invalid sloc>> 'const int'
> > |   |-IntegerLiteral 0xc072768 <col:27> 'int' 1
> > |   |-IntegerLiteral 0xc072788 <col:30> 'int' 2
> > |   `-IntegerLiteral 0xc0727a8 <col:33> 'int' 3
> > `-VarDecl 0xc0728e0 prev 0xc072700 <line:8:1, col:21> col:11 glob_arr3 'const int [4]'
> > ```
> So, the redeclaration actually changed the type to `ConstantArrayType`. If so, you should probably mention this in the comment.
OK, I'll mention.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111542/new/

https://reviews.llvm.org/D111542