[analyzer] Canonicalize variable declarations in VarRegion objects.

[NoQ]

Summary

A follow-up from D46421#1375147 - huh, yes, indeed, this is broken:

extern int x;

void foo() { x = 2; }

extern int x;

int main() {
  x = 1;
  foo();

  // TRUE but should be FALSE
  clang_analyzer_eval(x == 1);
  // FALSE but should be TRUE
  clang_analyzer_eval(x == 2); 
}

This does actually create a Store that looks like this:

(x,0,direct) : 1 S32b

(x,0,direct) : 2 S32b

Fxd. Note that we cannot canonicalize decls in constructors because static VarRegion::Profile() gets called in MemRegionManager::getSubRegion() before calling the actual constructor in order to find the location of the variable, and this should correspond exactly to the non-static VarRegion::Profile() at later points in the program, otherwise even in this example we get much more than 2 duplicate regions (MemRegionManager would repeatedly construct the region and then fail to look it up next time it is requested).

[xazax.hun]

Looks good, nice catch. :)

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

 
 class DeclRegion : public TypedValueRegion {
 protected:
   const ValueDecl *D;
 
   DeclRegion(const ValueDecl *d, const MemRegion *sReg, Kind k)
       : TypedValueRegion(sReg, k), D(d) {
     assert(classof(this));
-    assert(d);
+    assert(d && d->isCanonicalDecl());
   }
 
   static void ProfileRegion(llvm::FoldingSetNodeID& ID, const Decl *D,
                       const MemRegion* superRegion, Kind k);
 
 public:
   const ValueDecl *getDecl() const { return D; }
   void Profile(llvm::FoldingSetNodeID& ID) const override;

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

while (LC) {
 
     LC = LC->getParent();
   }
   return (const StackFrameContext *)nullptr;
 }
 
 const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
                                                 const LocationContext *LC) {
+  D = D->getCanonicalDecl();
   const MemRegion *sReg = nullptr;
 
   if (D->hasGlobalStorage() && !D->isStaticLocal()) {
 
     // First handle the globals defined in system headers.
     if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
       // Whitelist the system globals which often DO GET modified, assume the
       // rest are immutable.
// Finally handle static locals.
     }
   }
 
   return getSubRegion<VarRegion>(D, sReg);
 }
 
 const VarRegion *MemRegionManager::getVarRegion(const VarDecl *D,
                                                 const MemRegion *superR) {
+  D = D->getCanonicalDecl();
   return getSubRegion<VarRegion>(D, superR);
 }
 
 const BlockDataRegion *
 MemRegionManager::getBlockDataRegion(const BlockCodeRegion *BC,
                                      const LocationContext *LC,
                                      unsigned blockCount) {
   const MemSpaceRegion *sReg = nullptr;

clang/test/Analysis/globals.cpp

 {
     int * p = (int*)sizeof(int);
 };
 void recordinit()
 {
     S3 s3;
     *(s3.p - 1) = 0; // expected-warning{{Dereference of null pointer}}
 }
+
+extern int ext_int;
+
+void update_original_declaration() {
+  ext_int = 2;
+}
+
+extern int ext_int;
+
+int test_redeclaration() {
+  ext_int = 1;
+  update_original_declaration();
+  int int_int = 3 / (ext_int - 1); // no-warning
+  return int_int / (ext_int - 2); // expected-warning{{Division by zero}}
+}