[PATCH] D63968: [analyzer] Fix target region invalidation when returning into a ctor initializer.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet.
Herald added a project: clang.

Enabling pointer escape notification for the return value invalidation when conservatively evaluating calls that return C++ objects by value was supposed to be pointless. Indeed, we're looking at a freshly conjured value; why would any checker already track it at this point? Yet it does cause a change in results, so i decided to reduce and investigate a reproducer @Charusso sent me.

When i was thinking about it previously (as of D44131 <https://reviews.llvm.org/D44131>), i was imagining a function that constructs a temporary that would later be copied to its target destination. In this case there's indeed no need to notify checkers of pointer escape.

However, once RVO was implemented (D47671 <https://reviews.llvm.org/D47671>), the target region is no longer necessarily a temporary; it may be an arbitrary memory region. In particular, it may be a non-base region, such as `FieldRegion` if the construction context is a `ConstructorInitializerConstructionContext`. In this case the invalidation covers not only the target region but its whole base region that may already contain some values that might as well be tracked by the checkers.

The right solution, therefore, is to restrict invalidation so that it didn't propagate to the base region, but only wipe the target region itself. It probably doesn't work perfectly because RegionStore doesn't enjoy this sort of stuff, but it should be something.

In the newly added test case the previous behavior was to immediately forget about `b1.p` and `b2.p`, therefore evals on lines 21 and 23 yielded both `TRUE` and `FALSE` each (due to eagerly-assume) and the leak was diagnosed on line 22 (even though the pointer is used later).


Repository:
  rC Clang

https://reviews.llvm.org/D63968

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/rvo.cpp


Index: clang/test/Analysis/rvo.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/rvo.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core,cplusplus \
+// RUN:                    -analyzer-checker debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct A {
+  int x;
+};
+
+A getA();
+
+struct B {
+  int *p;
+  A a;
+
+  B(int *p) : p(p), a(getA()) {}
+};
+
+void foo() {
+  B b1(nullptr);
+  clang_analyzer_eval(b1.p == nullptr); // expected-warning{{TRUE}}
+  B b2(new int); // No leak yet!
+  clang_analyzer_eval(b2.p == nullptr); // expected-warning{{FALSE}}
+  // expected-warning at -1{{Potential leak of memory pointed to by 'b2.p'}}
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -634,12 +634,19 @@
     std::tie(State, Target) =
         prepareForObjectConstruction(Call.getOriginExpr(), State, LCtx,
                                      RTC->getConstructionContext(), CallOpts);
-    assert(Target.getAsRegion());
-    // Invalidate the region so that it didn't look uninitialized. Don't notify
-    // the checkers.
-    State = State->invalidateRegions(Target.getAsRegion(), E, Count, LCtx,
+    const MemRegion *TargetR = Target.getAsRegion();
+    assert(TargetR);
+    // Invalidate the region so that it didn't look uninitialized. If this is
+    // a field or element constructor, we do not want to invalidate
+    // the whole structure. Pointer escape is meaningless because
+    // the structure is a product of conservative evaluation
+    // and therefore contains nothing interesting at this point.
+    RegionAndSymbolInvalidationTraits ITraits;
+    ITraits.setTrait(TargetR,
+        RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
+    State = State->invalidateRegions(TargetR, E, Count, LCtx,
                                      /* CausedByPointerEscape=*/false, nullptr,
-                                     &Call, nullptr);
+                                     &Call, &ITraits);
 
     R = State->getSVal(Target.castAs<Loc>(), E->getType());
   } else {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D63968.207183.patch
Type: text/x-patch
Size: 2322 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190629/44875da3/attachment.bin>