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

[NoQ]

Summary

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), 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), 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).

[Charusso]

You have made a great use of that mistake by me. So that is why everything is so perfect, because it fires, just it should fire later. Thanks you!

[xazax.hun]

LG! Thanks!

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call,
   if (auto RTC = getCurrentCFGElement().getAs<CFGCXXRecordTypedCall>()) {
     // Conjure a temporary if the function returns an object by value.
     SVal Target;
     assert(RTC->getStmt() == Call.getOriginExpr());
     EvalCallOptions CallOpts; // FIXME: We won't really need those.
     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 {
     // Conjure a symbol if the return value is unknown.
 
     // See if we need to conjure a heap pointer instead of
     // a regular unknown pointer.
     bool IsHeapPointer = false;

cfe/trunk/test/Analysis/rvo.cpp

+// 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@-1{{Potential leak of memory pointed to by 'b2.p'}}
+}