[analyzer] An attempt to fix pr19539 - crashes on temporaries life-extended via members

[NoQ]

Summary

1. Re-use approach used in codegen. MaterializeTemporaryExpr may be positioned in a strange manner, above the member access to the temporary, which makes it a bit tricky to find the expression that actually corresponds to the temporary object. FIXME: Hmm, probably we should re-use this approach in CFG as well (cf. D22419).
2. Create the temporary region that corresponds to the full temporary object, rather than to the sub-object. This was a bug.
3. If lifetime extension occurs, use the temporary object region's path-sensitive type, rather than reference variable type, in order to call the correct destructor.

[alexander-shaposhnikov]

test/Analysis/lifetime-extension.cpp

11 ↗

(On Diff #78476)

what is the role of S in this test ?

[NoQ]

test/Analysis/lifetime-extension.cpp

11 ↗

(On Diff #78476)

I copy-pasted this from the original bug report. The only purpose of S is to provide a non-trivial destructor. I also thought it'd be a nice twist to the test to make the destructor non-obvious, but it doesn't really matter. Removed.

[dcoughlin]

This is great!

lib/StaticAnalyzer/Core/ExprEngine.cpp

205 ↗

(On Diff #78478)

I think it would good to show an AST representation in the comment to make it clear what "out of place" means. This could be a very simplified version of the dumped AST for a temporary with a field access.

[NoQ]

Update the comment.

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

if (!Result) {
     assert(!V.getAs<Loc>() || Loc::isLocType(Result->getType()) ||
            Result->getType()->isMemberPointerType());
   }
 
   ProgramStateManager &StateMgr = State->getStateManager();
   MemRegionManager &MRMgr = StateMgr.getRegionManager();
   StoreManager &StoreMgr = StateMgr.getStoreManager();
 
-  // We need to be careful about treating a derived type's value as
-  // bindings for a base type. Unless we're creating a temporary pointer region,
-  // start by stripping and recording base casts.
-  SmallVector<const CastExpr *, 4> Casts;
-  const Expr *Inner = Ex->IgnoreParens();
-  if (!Loc::isLocType(Result->getType())) {
-    while (const CastExpr *CE = dyn_cast<CastExpr>(Inner)) {
-      if (CE->getCastKind() == CK_DerivedToBase ||
-          CE->getCastKind() == CK_UncheckedDerivedToBase)
-        Casts.push_back(CE);
-      else if (CE->getCastKind() != CK_NoOp)
-        break;
+  // MaterializeTemporaryExpr may appear out of place, after a few field and
+  // base-class accesses have been made to the object, even though semantically
+  // it is the whole object that gets materialized and lifetime-extended.
+  //
+  // For example:
+  //
+  //   `-MaterializeTemporaryExpr
+  //     `-MemberExpr
+  //       `-CXXTemporaryObjectExpr
+  //
+  // instead of the more natural
+  //
+  //   `-MemberExpr
+  //     `-MaterializeTemporaryExpr
+  //       `-CXXTemporaryObjectExpr
+  //
+  // Use the usual methods for obtaining the expression of the base object,
+  // and record the adjustments that we need to make to obtain the sub-object
+  // that the whole expression 'Ex' refers to. This trick is usual,
+  // in the sense that CodeGen takes a similar route.
 
-      Inner = CE->getSubExpr()->IgnoreParens();
-    }
-  }
+  SmallVector<const Expr *, 2> CommaLHSs;
+  SmallVector<SubobjectAdjustment, 2> Adjustments;
+
+  const Expr *Init = Ex->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
 
-  // Create a temporary object region for the inner expression (which may have
-  // a more derived type) and bind the value into it.
   const TypedValueRegion *TR = nullptr;
   if (const MaterializeTemporaryExpr *MT =
           dyn_cast<MaterializeTemporaryExpr>(Result)) {
     StorageDuration SD = MT->getStorageDuration();
     // If this object is bound to a reference with static storage duration, we
     // put it in a different region to prevent "address leakage" warnings.
     if (SD == SD_Static || SD == SD_Thread)
-        TR = MRMgr.getCXXStaticTempObjectRegion(Inner);
+      TR = MRMgr.getCXXStaticTempObjectRegion(Init);
   }
   if (!TR)
-    TR = MRMgr.getCXXTempObjectRegion(Inner, LC);
+    TR = MRMgr.getCXXTempObjectRegion(Init, LC);
 
   SVal Reg = loc::MemRegionVal(TR);
 
+  // Make the necessary adjustments to obtain the sub-object.
+  for (auto I = Adjustments.rbegin(), E = Adjustments.rend(); I != E; ++I) {
+    const SubobjectAdjustment &Adj = *I;
+    switch (Adj.Kind) {
+    case SubobjectAdjustment::DerivedToBaseAdjustment:
+      Reg = StoreMgr.evalDerivedToBase(Reg, Adj.DerivedToBase.BasePath);
+      break;
+    case SubobjectAdjustment::FieldAdjustment:
+      Reg = StoreMgr.getLValueField(Adj.Field, Reg);
+      break;
+    case SubobjectAdjustment::MemberPointerAdjustment:
+      // FIXME: Unimplemented.
+      State->bindDefault(Reg, UnknownVal());
+      return State;
+    }
+  }
+
+  // Try to recover some path sensitivity in case we couldn't compute the value.
   if (V.isUnknown())
     V = getSValBuilder().conjureSymbolVal(Result, LC, TR->getValueType(),
                                           currBldrCtx->blockCount());
+  // Bind the value of the expression to the sub-object region, and then bind
+  // the sub-object region to our expression.
   State = State->bindLoc(Reg, V);
-
-  // Re-apply the casts (from innermost to outermost) for type sanity.
-  for (SmallVectorImpl<const CastExpr *>::reverse_iterator I = Casts.rbegin(),
-                                                           E = Casts.rend();
-       I != E; ++I) {
-    Reg = StoreMgr.evalDerivedToBase(Reg, *I);
-  }
-
   State = State->BindExpr(Result, LC, Reg);
   return State;
 }
 
 //===----------------------------------------------------------------------===//
 // Top-level transfer function logic (Dispatcher).
 //===----------------------------------------------------------------------===//
 
void ExprEngine::ProcessAutomaticObjDtor(const CFGAutomaticObjDtor Dtor,
                                          ExplodedNodeSet &Dst) {
   const VarDecl *varDecl = Dtor.getVarDecl();
   QualType varType = varDecl->getType();
 
   ProgramStateRef state = Pred->getState();
   SVal dest = state->getLValue(varDecl, Pred->getLocationContext());
   const MemRegion *Region = dest.castAs<loc::MemRegionVal>().getRegion();
 
-  if (const ReferenceType *refType = varType->getAs<ReferenceType>()) {
-    varType = refType->getPointeeType();
-    Region = state->getSVal(Region).getAsRegion();
+  if (varType->isReferenceType()) {
+    Region = state->getSVal(Region).getAsRegion()->getBaseRegion();
+    varType = cast<TypedValueRegion>(Region)->getValueType();
   }
 
   VisitCXXDestructor(varType, Region, Dtor.getTriggerStmt(), /*IsBase=*/ false,
                      Pred, Dst);
 }
 
 void ExprEngine::ProcessDeleteDtor(const CFGDeleteDtor Dtor,
                                    ExplodedNode *Pred,

cfe/trunk/test/Analysis/lifetime-extension.cpp

+// RUN: %clang_cc1 -Wno-unused -std=c++11 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(bool);
+
+namespace pr17001_call_wrong_destructor {
+bool x;
+struct A {
+  int *a;
+  A() {}
+  ~A() {}
+};
+struct B : public A {
+  B() {}
+  ~B() { x = true; }
+};
+
+void f() {
+  {
+    const A &a = B();
+  }
+  clang_analyzer_eval(x); // expected-warning{{TRUE}}
+}
+} // end namespace pr17001_call_wrong_destructor
+
+namespace pr19539_crash_on_destroying_an_integer {
+struct A {
+  int i;
+  int j[2];
+  A() : i(1) {
+    j[0] = 2;
+    j[1] = 3;
+  }
+  ~A() {}
+};
+
+void f() {
+  const int &x = A().i; // no-crash
+  const int &y = A().j[1]; // no-crash
+  const int &z = (A().j[1], A().j[0]); // no-crash
+
+  // FIXME: All of these should be TRUE, but constructors aren't inlined.
+  clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
+}
+} // end namespace pr19539_crash_on_destroying_an_integer