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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@353350 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h b/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
index 2121b5bc04ff1f44f7befd646c0d75abd0fb1e7e..85eb4b865abc9e289937232798a84344a25f57fe 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ b/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));
   }
 

diff --git a/lib/StaticAnalyzer/Core/RegionStore.cpp b/lib/StaticAnalyzer/Core/RegionStore.cpp
index 679d95b7c589ac724d58d98e86b720a7afa6d3b8..3323bb97a7124bee841ed87e35a454e18edc629b 100644 (file)
--- a/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/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

diff --git a/test/Analysis/malloc.c b/test/Analysis/malloc.c
index d4a44c576241be42566a11dce797cf5f98c6f323..cbc21b492e5902774539a2e61f044d4a321e8052 100644 (file)
--- a/test/Analysis/malloc.c
+++ b/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.
 

diff --git a/test/Analysis/symbol-reaper.c b/test/Analysis/symbol-reaper.c
index ef8ff18a2d831fcfacc67f26b3900af10af96994..62bb5d6c6b3433bb4b0a4890a1fa295c1536668c 100644 (file)
--- a/test/Analysis/symbol-reaper.c
+++ b/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}}
+}