[analyzer] Retrieve incomplete array extent from its redeclaration.

[ASDenysPetrov]

Summary

Fix a case when the extent can not be retrieved correctly from incomplete array declaration. Use redeclaration to get the array extent.

[ASDenysPetrov]

@NoQ @martong
Guys, I've patched the bug (mentioned in D104285). Could you check it? It works, but I have doubts using VR->getDecl()->getMostRecentDecl(); as a correct way to get a right declaration.

[steakhal]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1663

I think you supposed to use the getCanonicalDecl() instead.

1673

What does the function return if this branch is not taken?
Shouldn't you handle IncompleteArrayType as well?

1718

I would favor not adding another level of indentation xD

clang/test/Analysis/initialization.c

101

Why is this a VLA? I thought it's just an IncompleteArrayType.
Could you please make sure that this is actually a VLA?

[ASDenysPetrov]

@steakhal
Thank you for your comments.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1663

Using getCanonicalDecl does not fix the issue. I've checked.

1673

What does the function return if this branch is not taken?

In case of a false branch it goes down the code to getBindingForFieldOrElementCommon.

Shouldn't you handle IncompleteArrayType as well?

Only ConstantArrayType has getSize() method. And getMostRecentDecl give us an ConstantArrayType even if it is declared as IncompleteArrayType, but getCanonicalDecl does not.

1718

It annoys me as well. I could use goto but I suppose you won't like it even more. :-)

clang/test/Analysis/initialization.c

101

I mixed up in terminilogy. I'll fix it.

[balazske]

clang/test/Analysis/initialization.c

102

It is possible to add a line const int glob_arr3[]; again and then it may not work?

[NoQ]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1663

Aha ok, can you try iterating over redecls()?

Separately, I suspect that this should be performed before the VarRegion is constructed in the first place. Maybe in its constructor we should assert(VD->isThisDeclarationADefinition()) or something like that.

[ASDenysPetrov]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1663

Aha ok, can you try iterating over redecls()?

Do you assume that in a list {redecl1, redecl2, redecl3} redecl2 may be our guy but 1 and 3 may not?

Separately, I suspect that this should be performed before the VarRegion is constructed in the first place.

I'm not sure I got what you mean, but as I undestood that it is not be a part of this fix, right?

clang/test/Analysis/initialization.c

102

Yes. I tryed. It works. Thanks! But I'll add a test case.

[martong]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1661–1663

1663

I think this should be part of this fix, but you don't have to do that iteration, there is a better thing for the job: getAnyInitializer.

What we need is to find that Decl that has the InitExpr attached. This may not be the canonical nor the most recent redecl.
Please see my code change suggestion below.

clang/test/Analysis/initialization.c

101–102

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.

[ASDenysPetrov]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1661–1663

Thnx, I'll try this approach.

clang/test/Analysis/initialization.c

101–102

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]'

[steakhal]

clang/test/Analysis/initialization.c

101–102

So, the redeclaration actually changed the type to ConstantArrayType. If so, you should probably mention this in the comment.

[martong]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1669–1713

Denys, I suppose we could replace the whole inner block with a function? Similarly to getSValFromStringLiteralByIndex from D107339 . That could greatly increase readability.

[ASDenysPetrov]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1669–1713

Make sense. I'll carry this block out.

clang/test/Analysis/initialization.c

101–102

OK, I'll mention.

[ASDenysPetrov]

Rebased on top of D106681.

[NoQ]

Amazing, thanks a lot!

[ASDenysPetrov]

Updated according to the latest suggestions.
@martong I think now this patch is way more simple and clear than before.

[ASDenysPetrov]

In D111542#3073709, @NoQ wrote:

Amazing, thanks a lot!

Thank you, @NoQ, for the acceptance. I got some fresher update. And, of course in order to load it, we should accept a parent revision D106681 as well.

[ASDenysPetrov]

Fixed redefinition in test file initialization.c.

[steakhal]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1632–1636

Wait. But if VD is null, you get a null-dereference.
But you already dereferenced VD multiple times, so it cannot be null.

Oh, but the getAnyInitializer() will overwrite it! That's a surprise.
TBH I would rather pass a fresh uninitialized pointer if you really need the exact decl which actually provided the initialized expression to make this behavior explicit.

That way, with a properly chosen name you could spare the NOTE comment as well.

[martong]

In D111542#3074926, @ASDenysPetrov wrote:

Updated according to the latest suggestions.
@martong I think now this patch is way more simple and clear than before.

Thanks, indeed! LGTM (with nits about VD)!

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1632–1636

Yes I agree, probably it is easier to follow the code if you make it explicit that VD is overwritten here.

[ASDenysPetrov]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1632–1636

Wait. But if VD is null, you get a null-dereference.

We have an assertion above.

Oh, but the getAnyInitializer() will overwrite it! That's a surprise.

So was for me :) I'll add a NOTE in the comment.

[ASDenysPetrov]

Rebased. Updated comment for obscure behaviour of getAnyInitializer function.

[steakhal]

Minor nits. Aside from that just land it.
Thanks for the fix.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1637

If the return value of getAnyInitializer() is null, then the value of VD will be actually preserved.

[ASDenysPetrov]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1637

I'll add the comment before the load.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

RegionStoreManager::findLazyBinding(RegionBindingsConstRef B,
 
   return Result;
 }
 
 SVal RegionStoreManager::getBindingForElement(RegionBindingsConstRef B,
                                               const ElementRegion* R) {
   // Check if the region has a binding.
   if (const Optional<SVal> &V = B.getDirectBinding(R))
     return *V;
 
   const MemRegion* superR = R->getSuperRegion();
 
   // Check if the region is an element region of a string literal.
   if (const StringRegion *StrR = dyn_cast<StringRegion>(superR)) {
     // FIXME: Handle loads from strings where the literal is treated as
     // an integer, e.g., *((unsigned int*)"hello")
     QualType T = Ctx.getAsArrayType(StrR->getValueType())->getElementType();
     if (!Ctx.hasSameUnqualifiedType(T, R->getElementType()))
       return UnknownVal();
 
     const StringLiteral *Str = StrR->getStringLiteral();
     SVal Idx = R->getIndex();
     if (Optional<nonloc::ConcreteInt> CI = Idx.getAs<nonloc::ConcreteInt>()) {
       int64_t i = CI->getValue().getSExtValue();
       // Abort on string underrun.  This can be possible by arbitrary
       // clients of getBindingForElement().
       if (i < 0)
         return UndefinedVal();
       int64_t length = Str->getLength();
       // Technically, only i == length is guaranteed to be null.
       // However, such overflows should be caught before reaching this point;
       // the only time such an access would be made is if a string literal was
       // used to initialize a larger array.
       char c = (i >= length) ? '\0' : Str->getCodeUnit(i);
       return svalBuilder.makeIntVal(c, T);
     }
   } else if (const VarRegion *VR = dyn_cast<VarRegion>(superR)) {
     // Check if the containing array has an initialized value that we can trust.
     // We can trust a const value or a value of a global initializer in main().
-    const VarDecl *VD = VR->getDecl();
+    const VarDecl *VD = VR->getDecl()->getMostRecentDecl();
     if (VD->getType().isConstQualified() ||
         R->getElementType().isConstQualified() ||
         (B.isMainAnalysis() && VD->hasGlobalStorage())) {
       if (const Expr *Init = VD->getAnyInitializer()) {
         if (const auto *InitList = dyn_cast<InitListExpr>(Init)) {
           // 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();
-
+            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):
               //   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.
+                //   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;
               }
             }
           }
         }
       }
     }
+  }
 
   // Check for loads from a code text region.  For such loads, just give up.
   if (isa<CodeTextRegion>(superR))
     return UnknownVal();
 
   // Handle the case where we are indexing into a larger scalar object.
   // For example, this handles:
   //   int x = ...

clang/test/Analysis/initialization.c

 }
 
 // TODO: Support multidimensional array.
 void glob_invalid_index4() {
   int x = 3, y = 2;
   // FIXME: Should warn {{garbage or undefined}}.
   int res = glob_arr2[x][y]; // no-warning
 }
+
+const int glob_arr3[]; // VLA
+const int glob_arr3[4] = {1, 2, 3};
+void foo() {
+  clang_analyzer_eval(glob_arr3[0] == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr3[1] == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr3[2] == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr3[3] == 0); // expected-warning{{TRUE}}
+}
+
+void glob_invalid_index5() {
+  int x = 42;
+  int res = glob_arr3[x]; // expected-warning{{garbage or undefined}}
+}
+
+void glob_invalid_index6() {
+  int x = -42;
+  int res = glob_arr3[x]; // expected-warning{{garbage or undefined}}
+}