Revert "[analyzer] Remove the "postponed" hack, deal with derived symbols..."

This reverts commit r341722.

The "postponed" mechanism turns out to be necessary in order to handle
situations when a symbolic region is only kept alive by implicit bindings
in the Store. Otherwise the region is never scanned by the Store's worklist
and the binding gets dropped despite being live, as demonstrated
by the newly added tests.

Differential Revision: https://reviews.llvm.org/D57554

llvm-svn: 353350

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

@@ -773,9 +773,6 @@ class SymbolicRegion : public SubRegion {
     assert(s->getType()->isAnyPointerType() ||
            s->getType()->isReferenceType() ||
            s->getType()->isBlockPointerType());
-
-    // populateWorklistFromSymbol() relies on this assertion, and needs to be
-    // updated if more cases are introduced.
     assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg));
   }
 

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

@@ -2386,10 +2386,7 @@ RegionStoreManager::bindAggregate(RegionBindingsConstRef B,
 namespace {
 class RemoveDeadBindingsWorker
     : public ClusterAnalysis<RemoveDeadBindingsWorker> {
-  using ChildrenListTy = SmallVector<const SymbolDerived *, 4>;
-  using MapParentsToDerivedTy = llvm::DenseMap<SymbolRef, ChildrenListTy>;
-
-  MapParentsToDerivedTy ParentsToDerived;
+  SmallVector<const SymbolicRegion *, 12> Postponed;
   SymbolReaper &SymReaper;
   const StackFrameContext *CurrentLCtx;
 
@@ -2410,10 +2407,8 @@ public:
 
   bool AddToWorkList(const MemRegion *R);
 
+  bool UpdatePostponed();
   void VisitBinding(SVal V);
-
-private:
-  void populateWorklistFromSymbol(SymbolRef s);
 };
 }
 
@@ -2433,11 +2428,10 @@ void RemoveDeadBindingsWorker::VisitAddedToCluster(const MemRegion *baseR,
   }
 
   if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(baseR)) {
-    if (SymReaper.isLive(SR->getSymbol())) {
+    if (SymReaper.isLive(SR->getSymbol()))
       AddToWorkList(SR, &C);
-    } else if (const auto *SD = dyn_cast<SymbolDerived>(SR->getSymbol())) {
-      ParentsToDerived[SD->getParentSymbol()].push_back(SD);
-    }
+    else
+      Postponed.push_back(SR);
 
     return;
   }
@@ -2450,7 +2444,7 @@ void RemoveDeadBindingsWorker::VisitAddedToCluster(const MemRegion *baseR,
   // CXXThisRegion in the current or parent location context is live.
   if (const CXXThisRegion *TR = dyn_cast<CXXThisRegion>(baseR)) {
     const auto *StackReg =
-      cast<StackArgumentsSpaceRegion>(TR->getSuperRegion());
+        cast<StackArgumentsSpaceRegion>(TR->getSuperRegion());
     const StackFrameContext *RegCtx = StackReg->getStackFrame();
     if (CurrentLCtx &&
         (RegCtx == CurrentLCtx || RegCtx->isParentOf(CurrentLCtx)))
@@ -2494,15 +2488,6 @@ void RemoveDeadBindingsWorker::VisitBinding(SVal V) {
   // If V is a region, then add it to the worklist.
   if (const MemRegion *R = V.getAsRegion()) {
     AddToWorkList(R);
-
-    if (const auto *TVR = dyn_cast<TypedValueRegion>(R)) {
-      DefinedOrUnknownSVal RVS =
-          RM.getSValBuilder().getRegionValueSymbolVal(TVR);
-      if (const MemRegion *SR = RVS.getAsRegion()) {
-        AddToWorkList(SR);
-      }
-    }
-
     SymReaper.markLive(R);
 
     // All regions captured by a block are also live.
@@ -2516,30 +2501,25 @@ void RemoveDeadBindingsWorker::VisitBinding(SVal V) {
 
 
   // Update the set of live symbols.
-  for (auto SI = V.symbol_begin(), SE = V.symbol_end(); SI != SE; ++SI) {
-    populateWorklistFromSymbol(*SI);
-
-    for (const auto *SD : ParentsToDerived[*SI])
-      populateWorklistFromSymbol(SD);
-
+  for (auto SI = V.symbol_begin(), SE = V.symbol_end(); SI!=SE; ++SI)
     SymReaper.markLive(*SI);
-  }
 }
 
-void RemoveDeadBindingsWorker::populateWorklistFromSymbol(SymbolRef S) {
-  if (const auto *SD = dyn_cast<SymbolData>(S)) {
-    if (Loc::isLocType(SD->getType()) && !SymReaper.isLive(SD)) {
-      const SymbolicRegion *SR = RM.getRegionManager().getSymbolicRegion(SD);
+bool RemoveDeadBindingsWorker::UpdatePostponed() {
+  // See if any postponed SymbolicRegions are actually live now, after
+  // having done a scan.
+  bool Changed = false;
 
-      if (B.contains(SR))
-        AddToWorkList(SR);
-
-      const SymbolicRegion *SHR =
-          RM.getRegionManager().getSymbolicHeapRegion(SD);
-      if (B.contains(SHR))
-        AddToWorkList(SHR);
+  for (auto I = Postponed.begin(), E = Postponed.end(); I != E; ++I) {
+    if (const SymbolicRegion *SR = *I) {
+      if (SymReaper.isLive(SR->getSymbol())) {
+        Changed |= AddToWorkList(SR);
+        *I = nullptr;
+      }
     }
   }
+
+  return Changed;
 }
 
 StoreRef RegionStoreManager::removeDeadBindings(Store store,
@@ -2555,7 +2535,7 @@ StoreRef RegionStoreManager::removeDeadBindings(Store store,
     W.AddToWorkList(*I);
   }
 
-  W.RunWorkList();
+  do W.RunWorkList(); while (W.UpdatePostponed());
 
   // We have now scanned the store, marking reachable regions and symbols
   // as live.  We now remove all the regions that are dead from the store

clang/test/Analysis/malloc.c

@@ -1794,6 +1794,24 @@ void testNoCrashOnOffendingParameter() {
   allocateSomeMemory(offendingParameter, &ptr);
 } // expected-warning {{Potential leak of memory pointed to by 'ptr'}}
 
+
+// Test a false positive caused by a bug in liveness analysis.
+struct A {
+  int *buf;
+};
+struct B {
+  struct A *a;
+};
+void livenessBugRealloc(struct A *a) {
+  a->buf = realloc(a->buf, sizeof(int)); // no-warning
+}
+void testLivenessBug(struct B *in_b) {
+  struct B *b = in_b;
+  livenessBugRealloc(b->a);
+ ((void) 0); // An attempt to trick liveness analysis.
+  livenessBugRealloc(b->a);
+}
+
 // ----------------------------------------------------------------------------
 // False negatives.
 

clang/test/Analysis/symbol-reaper.c

@@ -133,3 +133,28 @@ void test_zombie_referenced_only_through_field_in_store_value() {
   clang_analyzer_warnOnDeadSymbol((int) s);
   int *x = &s->field;
 } // expected-warning{{SYMBOL DEAD}}
+
+void double_dereference_of_implicit_value_aux1(int *p) {
+  *p = 0;
+}
+
+void double_dereference_of_implicit_value_aux2(int *p) {
+  if (*p != 0)
+    clang_analyzer_warnIfReached(); // no-warning
+}
+
+void test_double_dereference_of_implicit_value(int **x) {
+  clang_analyzer_warnOnDeadSymbol(**x);
+  int **y = x;
+  {
+    double_dereference_of_implicit_value_aux1(*y);
+    // Give time for symbol reaping to happen.
+    ((void)0);
+    // The symbol for **y was cleaned up from the Store at this point,
+    // even though it was not perceived as dead when asked explicitly.
+    // For that reason the SYMBOL DEAD warning never appeared at this point.
+    double_dereference_of_implicit_value_aux2(*y);
+  }
+  // The symbol is generally reaped here regardless.
+  ((void)0); // expected-warning{{SYMBOL DEAD}}
+}