steakhal created this revision.
steakhal added reviewers: NoQ, xazax.hun, martong, ASDenysPetrov, Szelethus,
isuckatcs, vabridgers.
Herald added subscribers: manas, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: All.
steakhal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
The `SymbolReaper` should consider a region //live// if that appears in
the store bindings anywhere - even if the region is wrapped by
`RegionSymbolVals`, `SymbolicRegions` or `LazyCompoundVals`.
This mistake by the `SymbolReaper` materialized in false-positives like
in the following example:
void ptr1(List* n) {
List* n2 = new List(*n); // cctor
if (!n->next) {
if (n2->next) {
clang_analyzer_warnIfReached(); // FP! This should be unreachable.
}
}
delete n2;
}
The store entry of the pointee of `n2` looks something like this:
HeapSymRegion{conj_$1{List *, LC1, S2540, #1}} 0(Default)
lazyCompoundVal{0x5641f1ed8830,Element{SymRegion{reg_$2<List * n>},0
S64b,struct List}}
So, any constraints referring to the `VarRegion` `n` supposed to be kept alive
by that store binding.
Hence, the analyzer should be able to see that `reg_$3<List *
Element{SymRegion{reg_$2<List * n>},0 S64b,struct List}.next> { [0, 0] }` when
it reaches the `n2->next`.
This patch fixes the issue by reusing the `SymbolReaper::markLive(MemRegion)`
for doing the appropriate traversal.
I'm yet to do the performance testing, given that this is actually in
the hot path.
Co-authored-by: Tomasz Kamiński <tomasz.kamin...@sonarsource.com>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D132236
Files:
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
clang/test/Analysis/trivial-copy-struct.cpp
Index: clang/test/Analysis/trivial-copy-struct.cpp
===================================================================
--- clang/test/Analysis/trivial-copy-struct.cpp
+++ clang/test/Analysis/trivial-copy-struct.cpp
@@ -34,3 +34,28 @@
(void)(n1->ptr);
(void)(n2->ptr);
}
+
+struct List {
+ List* next;
+};
+
+void ptr1(List* n) {
+ List* n2 = new List(*n); // cctor
+ if (!n->next) {
+ if (n2->next) {
+ clang_analyzer_warnIfReached(); // unreachable
+ }
+ }
+ delete n2;
+}
+
+void ptr2(List* n) {
+ List* n2 = new List(); // ctor
+ *n2 = *n; // assignment
+ if (!n->next) {
+ if (n2->next) {
+ clang_analyzer_warnIfReached(); // unreachable
+ }
+ }
+ delete n2;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2829,22 +2829,14 @@
}
void RemoveDeadBindingsWorker::VisitBinding(SVal V) {
- // Is it a LazyCompoundVal? All referenced regions are live as well.
- if (Optional<nonloc::LazyCompoundVal> LCS =
- V.getAs<nonloc::LazyCompoundVal>()) {
-
- const RegionStoreManager::SValListTy &Vals = RM.getInterestingValues(*LCS);
-
- for (RegionStoreManager::SValListTy::const_iterator I = Vals.begin(),
- E = Vals.end();
- I != E; ++I)
- VisitBinding(*I);
+ const MemRegion *R = V.getAsRegion();
- return;
- }
+ // Is it a LazyCompoundVal? All referenced regions are live as well.
+ if (auto LCS = V.getAs<nonloc::LazyCompoundVal>())
+ R = LCS->getRegion();
// If V is a region, then add it to the worklist.
- if (const MemRegion *R = V.getAsRegion()) {
+ if (R) {
AddToWorkList(R);
SymReaper.markLive(R);
Index: clang/test/Analysis/trivial-copy-struct.cpp
===================================================================
--- clang/test/Analysis/trivial-copy-struct.cpp
+++ clang/test/Analysis/trivial-copy-struct.cpp
@@ -34,3 +34,28 @@
(void)(n1->ptr);
(void)(n2->ptr);
}
+
+struct List {
+ List* next;
+};
+
+void ptr1(List* n) {
+ List* n2 = new List(*n); // cctor
+ if (!n->next) {
+ if (n2->next) {
+ clang_analyzer_warnIfReached(); // unreachable
+ }
+ }
+ delete n2;
+}
+
+void ptr2(List* n) {
+ List* n2 = new List(); // ctor
+ *n2 = *n; // assignment
+ if (!n->next) {
+ if (n2->next) {
+ clang_analyzer_warnIfReached(); // unreachable
+ }
+ }
+ delete n2;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2829,22 +2829,14 @@
}
void RemoveDeadBindingsWorker::VisitBinding(SVal V) {
- // Is it a LazyCompoundVal? All referenced regions are live as well.
- if (Optional<nonloc::LazyCompoundVal> LCS =
- V.getAs<nonloc::LazyCompoundVal>()) {
-
- const RegionStoreManager::SValListTy &Vals = RM.getInterestingValues(*LCS);
-
- for (RegionStoreManager::SValListTy::const_iterator I = Vals.begin(),
- E = Vals.end();
- I != E; ++I)
- VisitBinding(*I);
+ const MemRegion *R = V.getAsRegion();
- return;
- }
+ // Is it a LazyCompoundVal? All referenced regions are live as well.
+ if (auto LCS = V.getAs<nonloc::LazyCompoundVal>())
+ R = LCS->getRegion();
// If V is a region, then add it to the worklist.
- if (const MemRegion *R = V.getAsRegion()) {
+ if (R) {
AddToWorkList(R);
SymReaper.markLive(R);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
