[PATCH] D154827: [analyzer] NonParamVarRegion should prefer definition over canonical decl

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: xazax.hun, donat.nagy, NoQ.
Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware.
Herald added a reviewer: Szelethus.
Herald added a project: All.
steakhal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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 <https://godbolt.org/z/Kdvr11EqY>:

  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 `NonParamVarRegion`s.

FYI The canonicalization of the decl was introduced by D57619 <https://reviews.llvm.org/D57619> in 2019.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154827

Files:
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/test/Analysis/globals.c


Index: clang/test/Analysis/globals.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/globals.c
@@ -0,0 +1,18 @@
+// 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}}
+}
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1077,13 +1077,16 @@
     }
   }
 
-  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);
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D154827.538567.patch
Type: text/x-patch
Size: 1696 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230710/52f53e50/attachment.bin>