[PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Now that we have a way to test symbol reaper, please, add more coverage to the symbol-reaper.c test, including the test that Jordan mentioned. Even if it is not fixed, it's good to include it with a FIXME note.

What is the performance impact of this change?

The changes to the RegionStore and Environment seem inconsistent and repetitive.. For example, we can remove a lot of this just by changing SymbolReaper::markLive(const MemRegion *region), see below. How is this patch different from your solution? Can it be generalized to handle more cases?

  diff --git a/lib/StaticAnalyzer/Core/RegionStore.cpp b/lib/StaticAnalyzer/Core/RegionStore.cpp
  index 49b5ac3..55db545 100644
  --- a/lib/StaticAnalyzer/Core/RegionStore.cpp
  +++ b/lib/StaticAnalyzer/Core/RegionStore.cpp
  @@ -2320,8 +2320,14 @@ void removeDeadBindingsWorker::VisitCluster(const MemRegion *baseR,
     if (const SymbolicRegion *SymR = dyn_cast<SymbolicRegion>(baseR))
       SymReaper.markLive(SymR->getSymbol());
   
  -  for (ClusterBindings::iterator I = C->begin(), E = C->end(); I != E; ++I)
  +  for (ClusterBindings::iterator I = C->begin(), E = C->end(); I != E; ++I) {
  +    // Mark the key of each binding as live.
  +    const BindingKey &K = I.getKey();
  +    if (auto subR = dyn_cast<SubRegion>(K.getRegion()))
  +      SymReaper.markLive(subR);
  +
       VisitBinding(I.getData());
  +  }
   }
   
   void removeDeadBindingsWorker::VisitBinding(SVal V) {
  @@ -2342,6 +2348,7 @@ void removeDeadBindingsWorker::VisitBinding(SVal V) {
     // If V is a region, then add it to the worklist.
     if (const MemRegion *R = V.getAsRegion()) {
       AddToWorkList(R);
  +    SymReaper.markLive(R);
   
       // All regions captured by a block are also live.
       if (const BlockDataRegion *BR = dyn_cast<BlockDataRegion>(R)) {
  diff --git a/lib/StaticAnalyzer/Core/SymbolManager.cpp b/lib/StaticAnalyzer/Core/SymbolManager.cpp
  index df4d22a..793f53e 100644
  --- a/lib/StaticAnalyzer/Core/SymbolManager.cpp
  +++ b/lib/StaticAnalyzer/Core/SymbolManager.cpp
  @@ -391,6 +391,16 @@ void SymbolReaper::markLive(SymbolRef sym) {
   
   void SymbolReaper::markLive(const MemRegion *region) {
     RegionRoots.insert(region);
  +  
  +  // Mark the element index as live.
  +  if (const ElementRegion *ER = dyn_cast<ElementRegion>(region)) {
  +    SVal Idx = ER->getIndex();
  +    for (SymExpr::symbol_iterator SI = Idx.symbol_begin(),
  +         SE = Idx.symbol_end();
  +         SI != SE; ++SI) {
  +      markLive(*SI);
  +    }
  +  }
   }


================
Comment at: lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:177
@@ +176,3 @@
+
+    C.emitReport(llvm::make_unique<BugReport>(*BT, "SYMBOL DEAD", N));
+  }
----------------
Thank you for adding this!!! This can be part of this commit or separate; up to you.

Please, update the debugger checkers document in the docs folder with information on how this should be used.

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:2241
@@ +2240,3 @@
+  for (ClusterBindings::iterator I = C->begin(), E = C->end(); I != E; ++I) {
+
+    // Mark the index symbol of any ElementRegion key as live.
----------------
Please, remove unnecessary spacing here and in a couple of other places.

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:2247
@@ +2246,3 @@
+      if (auto elemR = dyn_cast<ElementRegion>(subR))
+        if (SymbolRef Sym = elemR->getIndex().getAsSymbol())
+          SymReaper.markLive(Sym);
----------------
Why symbol_iterator is not used here?

================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:2249
@@ +2248,3 @@
+          SymReaper.markLive(Sym);
+      subR = dyn_cast<SubRegion>(subR->getSuperRegion());
+    }
----------------
Could you add test cases that explain why we need the while loop here?


http://reviews.llvm.org/D12726