r353350 - Revert "[analyzer] Remove the "postponed" hack, deal with derived symbols..."
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 6 15:56:43 PST 2019
Author: dergachev
Date: Wed Feb 6 15:56:43 2019
New Revision: 353350
URL: http://llvm.org/viewvc/llvm-project?rev=353350&view=rev
Log:
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
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
cfe/trunk/test/Analysis/malloc.c
cfe/trunk/test/Analysis/symbol-reaper.c
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h?rev=353350&r1=353349&r2=353350&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h Wed Feb 6 15:56:43 2019
@@ -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));
}
Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=353350&r1=353349&r2=353350&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Wed Feb 6 15:56:43 2019
@@ -2386,10 +2386,7 @@ RegionStoreManager::bindAggregate(Region
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::VisitAdde
}
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::VisitAdde
// 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::VisitBind
// 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::VisitBind
// 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);
-
- if (B.contains(SR))
- AddToWorkList(SR);
-
- const SymbolicRegion *SHR =
- RM.getRegionManager().getSymbolicHeapRegion(SD);
- if (B.contains(SHR))
- AddToWorkList(SHR);
+bool RemoveDeadBindingsWorker::UpdatePostponed() {
+ // See if any postponed SymbolicRegions are actually live now, after
+ // having done a scan.
+ bool Changed = false;
+
+ 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::removeDeadB
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
Modified: cfe/trunk/test/Analysis/malloc.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.c?rev=353350&r1=353349&r2=353350&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/malloc.c (original)
+++ cfe/trunk/test/Analysis/malloc.c Wed Feb 6 15:56:43 2019
@@ -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.
Modified: cfe/trunk/test/Analysis/symbol-reaper.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/symbol-reaper.c?rev=353350&r1=353349&r2=353350&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/symbol-reaper.c (original)
+++ cfe/trunk/test/Analysis/symbol-reaper.c Wed Feb 6 15:56:43 2019
@@ -133,3 +133,28 @@ void test_zombie_referenced_only_through
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}}
+}
More information about the cfe-commits
mailing list