[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

1763–1764

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

1764

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

1766

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

1763–1764

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

1764

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.

1766

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

1763–1764

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

1763–1764

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

1763–1764

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.

1763–1764

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

1763–1764

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

1763–1764

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

1763–1764

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

1649–1653

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

1649–1653

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

1649–1653

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

1654

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

[ASDenysPetrov]

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

1654

I'll add the comment before the load.

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Optional<SVal> RegionStoreManager::getConstantValFromConstArrayInitializer(
   // 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();
   if (!VD->getType().isConstQualified() &&
       !R->getElementType().isConstQualified() &&
       (!B.isMainAnalysis() || !VD->hasGlobalStorage()))
     return None;
 
-  // Array's declaration should have an initializer.
-  const Expr *Init = VD->getAnyInitializer();
+  // Array's declaration should have `ConstantArrayType` type, because only this
+  // type contains an array extent. It may happen that array type can be of
+  // `IncompleteArrayType` type. To get the declaration of `ConstantArrayType`
+  // type, we should find the declaration in the redeclarations chain that has
+  // the initialization expression.
+  // NOTE: `getAnyInitializer` has an out-parameter, which returns a new `VD`
+  // from which an initializer is obtained. We replace current `VD` with the new
+  // `VD`. If the return value of the function is null than `VD` won't be
+  // replaced.
+  const Expr *Init = VD->getAnyInitializer(VD);
+  // NOTE: If `Init` is non-null, then a new `VD` is non-null for sure. So check
+  // `Init` for null only and don't worry about the replaced `VD`.
   if (!Init)
     return None;
 
   // Array's declaration should have ConstantArrayType type, because only this
   // type contains an array extent.
   const ConstantArrayType *CAT = Ctx.getAsConstantArrayType(VD->getType());
   if (!CAT)
     return None;
if (Optional<nonloc::ConcreteInt> CI = Idx.getAs<nonloc::ConcreteInt>()) {
       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)) {
     if (Optional<SVal> V = getConstantValFromConstArrayInitializer(B, VR, R))
       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_arr_no_init[10];
 void glob_arr_index4() {
   // FIXME: Should warn {{FALSE}}, since the array has a static storage.
   clang_analyzer_eval(glob_arr_no_init[2]); // expected-warning{{UNKNOWN}}
 }
+
+const int glob_arr3[];              // IncompleteArrayType
+const int glob_arr3[4] = {1, 2, 3}; // ConstantArrayType
+void glob_arr_index5() {
+  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}}
+}
+
+const int glob_arr4[];              // IncompleteArrayType
+const int glob_arr4[4] = {1, 2, 3}; // ConstantArrayType
+const int glob_arr4[];              // ConstantArrayType (according to AST)
+void glob_arr_index6() {
+  clang_analyzer_eval(glob_arr4[0] == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr4[1] == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr4[2] == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(glob_arr4[3] == 0); // expected-warning{{TRUE}}
+}
+
+void glob_invalid_index7() {
+  int x = 42;
+  int res = glob_arr4[x]; // expected-warning{{garbage or undefined}}
+}
+
+void glob_invalid_index8() {
+  int x = -42;
+  int res = glob_arr4[x]; // expected-warning{{garbage or undefined}}
+}