[PATCH] D132236: [analyzer] Fix liveness of LazyCompoundVals

[Balázs Benics via Phabricator via cfe-commits]

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.kaminski at 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);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D132236.454011.patch
Type: text/x-patch
Size: 1842 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220819/37b9f600/attachment.bin>