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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
