[analyzer] NonParamVarRegion should prefer definition over canonical decl

[steakhal]

Summary

When we construct a NonParamVarRegion, we canonicalize the decl to
always use the same entity for consistency.
At the moment that is the canonical decl - which is the first decl in
the redecl chain.

However, this can cause problems with tentative declarations and extern
declarations if we declare an array with unknown bounds.

Consider this C example:

typedef typeof(sizeof(int)) size_t;
size_t clang_analyzer_getExtent(const void *p);
void clang_analyzer_dump(size_t n);

extern const unsigned char extern_redecl[];
const unsigned char extern_redecl[] = { 1,2,3,4 };
const unsigned char tentative_redecl[];
const unsigned char tentative_redecl[] = { 1,2,3,4 };

const unsigned char direct_decl[] = { 1,2,3,4 };

void test_redeclaration_extent(void) {
  clang_analyzer_dump(clang_analyzer_getExtent(direct_decl));      // 4
  clang_analyzer_dump(clang_analyzer_getExtent(extern_redecl));    // should be 4 instead of Unknown
  clang_analyzer_dump(clang_analyzer_getExtent(tentative_redecl)); // should be 4 instead of Unknown
}

The getType() of the canonical decls for the forward declared globals,
will return IncompleteArrayType, unlike the
getDefinition()->getType(), which would have returned
ConstantArrayType of 4 elements.

This makes the MemRegionManager::getStaticSize() return Unknown as
the extent for the array variables, leading to FNs.

To resolve this, I think we should prefer the definition decl (if
present) over the canonical decl when constructing NonParamVarRegions.

FYI The canonicalization of the decl was introduced by D57619 in 2019.

[tomasz-kaminski-sonarsource]

Looks good. Thanks!

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

if (!STC) {
         }
         else {
           sReg = getGlobalsRegion();
         }
       }
     }
   }
 
-  return getSubRegion<NonParamVarRegion>(D, sReg);
+  return getNonParamVarRegion(D, sReg);
 }
 
 const NonParamVarRegion *
 MemRegionManager::getNonParamVarRegion(const VarDecl *D,
                                        const MemRegion *superR) {
+  // Prefer the definition over the canonical decl as the canonical form.
   D = D->getCanonicalDecl();
+  if (const VarDecl *Def = D->getDefinition())
+    D = Def;
   return getSubRegion<NonParamVarRegion>(D, superR);
 }
 
 const ParamVarRegion *
 MemRegionManager::getParamVarRegion(const Expr *OriginExpr, unsigned Index,
                                     const LocationContext *LC) {
   const StackFrameContext *SFC = LC->getStackFrame();
   assert(SFC);

clang/test/Analysis/globals.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+typedef typeof(sizeof(int)) size_t;
+size_t clang_analyzer_getExtent(const void *p);
+void clang_analyzer_dump(size_t n);
+
+extern const unsigned char extern_redecl[];
+const unsigned char extern_redecl[] = { 1,2,3,4 };
+const unsigned char tentative_redecl[];
+const unsigned char tentative_redecl[] = { 1,2,3,4 };
+
+const unsigned char direct_decl[] = { 1,2,3,4 };
+
+void test_redeclaration_extent(void) {
+  clang_analyzer_dump(clang_analyzer_getExtent(direct_decl));      // expected-warning {{4 S64b}}
+  clang_analyzer_dump(clang_analyzer_getExtent(extern_redecl));    // expected-warning {{4 S64b}}
+  clang_analyzer_dump(clang_analyzer_getExtent(tentative_redecl)); // expected-warning {{4 S64b}}
+}