[clang][dataflow] Propagate locations from result objects to initializers. (#87320)

Previously, we were propagating storage locations the other way around,
i.e.
from initializers to result objects, using `RecordValue::getLoc()`. This
gave
the wrong behavior in some cases -- see the newly added or fixed tests
in this
patch.

In addition, this patch now unblocks removing the `RecordValue` class
entirely,
as we no longer need `RecordValue::getLoc()`.

With this patch, the test `TransferTest.DifferentReferenceLocInJoin`
started to
fail because the framework now always uses the same storge location for
a
`MaterializeTemporaryExpr`, meaning that the code under test no longer
set up
the desired state where a variable of reference type is mapped to two
different
storage locations in environments being joined. Rather than trying to
modify
this test to set up the test condition again, I have chosen to replace
the test
with an equivalent test in DataflowEnvironmentTest.cpp that sets up the
test
condition directly; because this test is more direct, it will also be
less
brittle in the face of future changes.

1 files changed, 307 insertions, 98 deletions

diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 1bfa7eb..6c796b4 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -15,6 +15,7 @@
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Type.h"
 #include "clang/Analysis/FlowSensitive/DataflowLattice.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
@@ -26,6 +27,8 @@
 #include <cassert>
 #include <utility>
 
+#define DEBUG_TYPE "dataflow"
+
 namespace clang {
 namespace dataflow {
 
@@ -354,6 +357,8 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
   for (auto *Child : S.children())
     if (Child != nullptr)
       getFieldsGlobalsAndFuncs(*Child, Fields, Vars, Funcs);
+  if (const auto *DefaultArg = dyn_cast<CXXDefaultArgExpr>(&S))
+    getFieldsGlobalsAndFuncs(*DefaultArg->getExpr(), Fields, Vars, Funcs);
   if (const auto *DefaultInit = dyn_cast<CXXDefaultInitExpr>(&S))
     getFieldsGlobalsAndFuncs(*DefaultInit->getExpr(), Fields, Vars, Funcs);
 
@@ -386,6 +391,186 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
   }
 }
 
+namespace {
+
+// Visitor that builds a map from record prvalues to result objects.
+// This traverses the body of the function to be analyzed; for each result
+// object that it encounters, it propagates the storage location of the result
+// object to all record prvalues that can initialize it.
+class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
+public:
+  // `ResultObjectMap` will be filled with a map from record prvalues to result
+  // object. If the function being analyzed returns a record by value,
+  // `LocForRecordReturnVal` is the location to which this record should be
+  // written; otherwise, it is null.
+  explicit ResultObjectVisitor(
+      llvm::DenseMap<const Expr *, RecordStorageLocation *> &ResultObjectMap,
+      RecordStorageLocation *LocForRecordReturnVal,
+      DataflowAnalysisContext &DACtx)
+      : ResultObjectMap(ResultObjectMap),
+        LocForRecordReturnVal(LocForRecordReturnVal), DACtx(DACtx) {}
+
+  bool shouldVisitImplicitCode() { return true; }
+
+  bool shouldVisitLambdaBody() const { return false; }
+
+  // Traverse all member and base initializers of `Ctor`. This function is not
+  // called by `RecursiveASTVisitor`; it should be called manually if we are
+  // analyzing a constructor. `ThisPointeeLoc` is the storage location that
+  // `this` points to.
+  void TraverseConstructorInits(const CXXConstructorDecl *Ctor,
+                                RecordStorageLocation *ThisPointeeLoc) {
+    assert(ThisPointeeLoc != nullptr);
+    for (const CXXCtorInitializer *Init : Ctor->inits()) {
+      Expr *InitExpr = Init->getInit();
+      if (FieldDecl *Field = Init->getMember();
+          Field != nullptr && Field->getType()->isRecordType()) {
+        PropagateResultObject(InitExpr, cast<RecordStorageLocation>(
+                                            ThisPointeeLoc->getChild(*Field)));
+      } else if (Init->getBaseClass()) {
+        PropagateResultObject(InitExpr, ThisPointeeLoc);
+      }
+
+      // Ensure that any result objects within `InitExpr` (e.g. temporaries)
+      // are also propagated to the prvalues that initialize them.
+      TraverseStmt(InitExpr);
+
+      // If this is a `CXXDefaultInitExpr`, also propagate any result objects
+      // within the default expression.
+      if (auto *DefaultInit = dyn_cast<CXXDefaultInitExpr>(InitExpr))
+        TraverseStmt(DefaultInit->getExpr());
+    }
+  }
+
+  bool TraverseBindingDecl(BindingDecl *BD) {
+    // `RecursiveASTVisitor` doesn't traverse holding variables for
+    // `BindingDecl`s by itself, so we need to tell it to.
+    if (VarDecl *HoldingVar = BD->getHoldingVar())
+      TraverseDecl(HoldingVar);
+    return RecursiveASTVisitor<ResultObjectVisitor>::TraverseBindingDecl(BD);
+  }
+
+  bool VisitVarDecl(VarDecl *VD) {
+    if (VD->getType()->isRecordType() && VD->hasInit())
+      PropagateResultObject(
+          VD->getInit(),
+          &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*VD)));
+    return true;
+  }
+
+  bool VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE) {
+    if (MTE->getType()->isRecordType())
+      PropagateResultObject(
+          MTE->getSubExpr(),
+          &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*MTE)));
+    return true;
+  }
+
+  bool VisitReturnStmt(ReturnStmt *Return) {
+    Expr *RetValue = Return->getRetValue();
+    if (RetValue != nullptr && RetValue->getType()->isRecordType() &&
+        RetValue->isPRValue())
+      PropagateResultObject(RetValue, LocForRecordReturnVal);
+    return true;
+  }
+
+  bool VisitExpr(Expr *E) {
+    // Clang's AST can have record-type prvalues without a result object -- for
+    // example as full-expressions contained in a compound statement or as
+    // arguments of call expressions. We notice this if we get here and a
+    // storage location has not yet been associated with `E`. In this case,
+    // treat this as if it was a `MaterializeTemporaryExpr`.
+    if (E->isPRValue() && E->getType()->isRecordType() &&
+        !ResultObjectMap.contains(E))
+      PropagateResultObject(
+          E, &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*E)));
+    return true;
+  }
+
+  // Assigns `Loc` as the result object location of `E`, then propagates the
+  // location to all lower-level prvalues that initialize the same object as
+  // `E` (or one of its base classes or member variables).
+  void PropagateResultObject(Expr *E, RecordStorageLocation *Loc) {
+    if (!E->isPRValue() || !E->getType()->isRecordType()) {
+      assert(false);
+      // Ensure we don't propagate the result object if we hit this in a
+      // release build.
+      return;
+    }
+
+    ResultObjectMap[E] = Loc;
+
+    // The following AST node kinds are "original initializers": They are the
+    // lowest-level AST node that initializes a given object, and nothing
+    // below them can initialize the same object (or part of it).
+    if (isa<CXXConstructExpr>(E) || isa<CallExpr>(E) || isa<LambdaExpr>(E) ||
+        isa<CXXDefaultArgExpr>(E) || isa<CXXDefaultInitExpr>(E) ||
+        isa<CXXStdInitializerListExpr>(E)) {
+      return;
+    }
+
+    if (auto *InitList = dyn_cast<InitListExpr>(E)) {
+      if (!InitList->isSemanticForm())
+        return;
+      if (InitList->isTransparent()) {
+        PropagateResultObject(InitList->getInit(0), Loc);
+        return;
+      }
+
+      RecordInitListHelper InitListHelper(InitList);
+
+      for (auto [Base, Init] : InitListHelper.base_inits()) {
+        assert(Base->getType().getCanonicalType() ==
+               Init->getType().getCanonicalType());
+
+        // Storage location for the base class is the same as that of the
+        // derived class because we "flatten" the object hierarchy and put all
+        // fields in `RecordStorageLocation` of the derived class.
+        PropagateResultObject(Init, Loc);
+      }
+
+      for (auto [Field, Init] : InitListHelper.field_inits()) {
+        // Fields of non-record type are handled in
+        // `TransferVisitor::VisitInitListExpr()`.
+        if (!Field->getType()->isRecordType())
+          continue;
+        PropagateResultObject(
+            Init, cast<RecordStorageLocation>(Loc->getChild(*Field)));
+      }
+      return;
+    }
+
+    if (auto *Op = dyn_cast<BinaryOperator>(E); Op && Op->isCommaOp()) {
+      PropagateResultObject(Op->getRHS(), Loc);
+      return;
+    }
+
+    if (auto *Cond = dyn_cast<AbstractConditionalOperator>(E)) {
+      PropagateResultObject(Cond->getTrueExpr(), Loc);
+      PropagateResultObject(Cond->getFalseExpr(), Loc);
+      return;
+    }
+
+    // All other expression nodes that propagate a record prvalue should have
+    // exactly one child.
+    SmallVector<Stmt *, 1> Children(E->child_begin(), E->child_end());
+    LLVM_DEBUG({
+      if (Children.size() != 1)
+        E->dump();
+    });
+    assert(Children.size() == 1);
+    for (Stmt *S : Children)
+      PropagateResultObject(cast<Expr>(S), Loc);
+  }
+
+private:
+  llvm::DenseMap<const Expr *, RecordStorageLocation *> &ResultObjectMap;
+  RecordStorageLocation *LocForRecordReturnVal;
+  DataflowAnalysisContext &DACtx;
+};
+
+} // namespace
+
 Environment::Environment(DataflowAnalysisContext &DACtx)
     : DACtx(&DACtx),
       FlowConditionToken(DACtx.arena().makeFlowConditionToken()) {}
@@ -401,17 +586,23 @@ void Environment::initialize() {
   if (DeclCtx == nullptr)
     return;
 
-  if (const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx)) {
-    assert(FuncDecl->doesThisDeclarationHaveABody());
+  const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx);
+  if (FuncDecl == nullptr)
+    return;
+
+  assert(FuncDecl->doesThisDeclarationHaveABody());
 
-    initFieldsGlobalsAndFuncs(FuncDecl);
+  initFieldsGlobalsAndFuncs(FuncDecl);
 
-    for (const auto *ParamDecl : FuncDecl->parameters()) {
-      assert(ParamDecl != nullptr);
-      setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr));
-    }
+  for (const auto *ParamDecl : FuncDecl->parameters()) {
+    assert(ParamDecl != nullptr);
+    setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr));
   }
 
+  if (FuncDecl->getReturnType()->isRecordType())
+    LocForRecordReturnVal = &cast<RecordStorageLocation>(
+        createStorageLocation(FuncDecl->getReturnType()));
+
   if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(DeclCtx)) {
     auto *Parent = MethodDecl->getParent();
     assert(Parent != nullptr);
@@ -444,6 +635,12 @@ void Environment::initialize() {
         initializeFieldsWithValues(ThisLoc);
     }
   }
+
+  // We do this below the handling of `CXXMethodDecl` above so that we can
+  // be sure that the storage location for `this` has been set.
+  ResultObjectMap = std::make_shared<PrValueToResultObject>(
+      buildResultObjectMap(DACtx, FuncDecl, getThisPointeeStorageLocation(),
+                           LocForRecordReturnVal));
 }
 
 // FIXME: Add support for resetting globals after function calls to enable
@@ -484,13 +681,18 @@ void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
     if (getStorageLocation(*D) != nullptr)
       continue;
 
-    setStorageLocation(*D, createObject(*D));
+    // We don't run transfer functions on the initializers of global variables,
+    // so they won't be associated with a value or storage location. We
+    // therefore intentionally don't pass an initializer to `createObject()`;
+    // in particular, this ensures that `createObject()` will initialize the
+    // fields of record-type variables with values.
+    setStorageLocation(*D, createObject(*D, nullptr));
   }
 
   for (const FunctionDecl *FD : Funcs) {
     if (getStorageLocation(*FD) != nullptr)
       continue;
-    auto &Loc = createStorageLocation(FD->getType());
+    auto &Loc = createStorageLocation(*FD);
     setStorageLocation(*FD, Loc);
   }
 }
@@ -519,6 +721,9 @@ Environment Environment::pushCall(const CallExpr *Call) const {
     }
   }
 
+  if (Call->getType()->isRecordType() && Call->isPRValue())
+    Env.LocForRecordReturnVal = &Env.getResultObjectLocation(*Call);
+
   Env.pushCallInternal(Call->getDirectCallee(),
                        llvm::ArrayRef(Call->getArgs(), Call->getNumArgs()));
 
@@ -529,6 +734,7 @@ Environment Environment::pushCall(const CXXConstructExpr *Call) const {
   Environment Env(*this);
 
   Env.ThisPointeeLoc = &Env.getResultObjectLocation(*Call);
+  Env.LocForRecordReturnVal = &Env.getResultObjectLocation(*Call);
 
   Env.pushCallInternal(Call->getConstructor(),
                        llvm::ArrayRef(Call->getArgs(), Call->getNumArgs()));
@@ -557,6 +763,10 @@ void Environment::pushCallInternal(const FunctionDecl *FuncDecl,
     const VarDecl *Param = *ParamIt;
     setStorageLocation(*Param, createObject(*Param, Args[ArgIndex]));
   }
+
+  ResultObjectMap = std::make_shared<PrValueToResultObject>(
+      buildResultObjectMap(DACtx, FuncDecl, getThisPointeeStorageLocation(),
+                           LocForRecordReturnVal));
 }
 
 void Environment::popCall(const CallExpr *Call, const Environment &CalleeEnv) {
@@ -600,6 +810,9 @@ bool Environment::equivalentTo(const Environment &Other,
   if (ReturnLoc != Other.ReturnLoc)
     return false;
 
+  if (LocForRecordReturnVal != Other.LocForRecordReturnVal)
+    return false;
+
   if (ThisPointeeLoc != Other.ThisPointeeLoc)
     return false;
 
@@ -623,8 +836,10 @@ LatticeEffect Environment::widen(const Environment &PrevEnv,
   assert(DACtx == PrevEnv.DACtx);
   assert(ReturnVal == PrevEnv.ReturnVal);
   assert(ReturnLoc == PrevEnv.ReturnLoc);
+  assert(LocForRecordReturnVal == PrevEnv.LocForRecordReturnVal);
   assert(ThisPointeeLoc == PrevEnv.ThisPointeeLoc);
   assert(CallStack == PrevEnv.CallStack);
+  assert(ResultObjectMap == PrevEnv.ResultObjectMap);
 
   auto Effect = LatticeEffect::Unchanged;
 
@@ -656,12 +871,16 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
                               Environment::ValueModel &Model,
                               ExprJoinBehavior ExprBehavior) {
   assert(EnvA.DACtx == EnvB.DACtx);
+  assert(EnvA.LocForRecordReturnVal == EnvB.LocForRecordReturnVal);
   assert(EnvA.ThisPointeeLoc == EnvB.ThisPointeeLoc);
   assert(EnvA.CallStack == EnvB.CallStack);
+  assert(EnvA.ResultObjectMap == EnvB.ResultObjectMap);
 
   Environment JoinedEnv(*EnvA.DACtx);
 
   JoinedEnv.CallStack = EnvA.CallStack;
+  JoinedEnv.ResultObjectMap = EnvA.ResultObjectMap;
+  JoinedEnv.LocForRecordReturnVal = EnvA.LocForRecordReturnVal;
   JoinedEnv.ThisPointeeLoc = EnvA.ThisPointeeLoc;
 
   if (EnvA.ReturnVal == nullptr || EnvB.ReturnVal == nullptr) {
@@ -730,6 +949,12 @@ StorageLocation &Environment::createStorageLocation(const Expr &E) {
 
 void Environment::setStorageLocation(const ValueDecl &D, StorageLocation &Loc) {
   assert(!DeclToLoc.contains(&D));
+  // The only kinds of declarations that may have a "variable" storage location
+  // are declarations of reference type and `BindingDecl`. For all other
+  // declaration, the storage location should be the stable storage location
+  // returned by `createStorageLocation()`.
+  assert(D.getType()->isReferenceType() || isa<BindingDecl>(D) ||
+         &Loc == &createStorageLocation(D));
   DeclToLoc[&D] = &Loc;
 }
 
@@ -791,50 +1016,29 @@ Environment::getResultObjectLocation(const Expr &RecordPRValue) const {
   assert(RecordPRValue.getType()->isRecordType());
   assert(RecordPRValue.isPRValue());
 
-  // Returns a storage location that we can use if assertions fail.
-  auto FallbackForAssertFailure =
-      [this, &RecordPRValue]() -> RecordStorageLocation & {
+  assert(ResultObjectMap != nullptr);
+  RecordStorageLocation *Loc = ResultObjectMap->lookup(&RecordPRValue);
+  assert(Loc != nullptr);
+  // In release builds, use the "stable" storage location if the map lookup
+  // failed.
+  if (Loc == nullptr)
     return cast<RecordStorageLocation>(
         DACtx->getStableStorageLocation(RecordPRValue));
-  };
-
-  if (isOriginalRecordConstructor(RecordPRValue)) {
-    auto *Val = cast_or_null<RecordValue>(getValue(RecordPRValue));
-    // The builtin transfer function should have created a `RecordValue` for all
-    // original record constructors.
-    assert(Val);
-    if (!Val)
-      return FallbackForAssertFailure();
-    return Val->getLoc();
-  }
-
-  if (auto *Op = dyn_cast<BinaryOperator>(&RecordPRValue);
-      Op && Op->isCommaOp()) {
-    return getResultObjectLocation(*Op->getRHS());
-  }
-
-  // All other expression nodes that propagate a record prvalue should have
-  // exactly one child.
-  llvm::SmallVector<const Stmt *> children(RecordPRValue.child_begin(),
-                                           RecordPRValue.child_end());
-  assert(children.size() == 1);
-  if (children.empty())
-    return FallbackForAssertFailure();
-
-  return getResultObjectLocation(*cast<Expr>(children[0]));
+  return *Loc;
 }
 
 PointerValue &Environment::getOrCreateNullPointerValue(QualType PointeeType) {
   return DACtx->getOrCreateNullPointerValue(PointeeType);
 }
 
-void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc) {
+void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
+                                             QualType Type) {
   llvm::DenseSet<QualType> Visited;
   int CreatedValuesCount = 0;
-  initializeFieldsWithValues(Loc, Visited, 0, CreatedValuesCount);
+  initializeFieldsWithValues(Loc, Type, Visited, 0, CreatedValuesCount);
   if (CreatedValuesCount > MaxCompositeValueSize) {
-    llvm::errs() << "Attempting to initialize a huge value of type: "
-                 << Loc.getType() << '\n';
+    llvm::errs() << "Attempting to initialize a huge value of type: " << Type
+                 << '\n';
   }
 }
 
@@ -848,8 +1052,7 @@ void Environment::setValue(const Expr &E, Value &Val) {
   const Expr &CanonE = ignoreCFGOmittedNodes(E);
 
   if (auto *RecordVal = dyn_cast<RecordValue>(&Val)) {
-    assert(isOriginalRecordConstructor(CanonE) ||
-           &RecordVal->getLoc() == &getResultObjectLocation(CanonE));
+    assert(&RecordVal->getLoc() == &getResultObjectLocation(CanonE));
     (void)RecordVal;
   }
 
@@ -928,7 +1131,8 @@ Value *Environment::createValueUnlessSelfReferential(
   if (Type->isRecordType()) {
     CreatedValuesCount++;
     auto &Loc = cast<RecordStorageLocation>(createStorageLocation(Type));
-    initializeFieldsWithValues(Loc, Visited, Depth, CreatedValuesCount);
+    initializeFieldsWithValues(Loc, Loc.getType(), Visited, Depth,
+                               CreatedValuesCount);
 
     return &refreshRecordValue(Loc, *this);
   }
@@ -960,6 +1164,7 @@ Environment::createLocAndMaybeValue(QualType Ty,
 }
 
 void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
+                                             QualType Type,
                                              llvm::DenseSet<QualType> &Visited,
                                              int Depth,
                                              int &CreatedValuesCount) {
@@ -967,8 +1172,8 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
     if (FieldType->isRecordType()) {
       auto &FieldRecordLoc = cast<RecordStorageLocation>(FieldLoc);
       setValue(FieldRecordLoc, create<RecordValue>(FieldRecordLoc));
-      initializeFieldsWithValues(FieldRecordLoc, Visited, Depth + 1,
-                                 CreatedValuesCount);
+      initializeFieldsWithValues(FieldRecordLoc, FieldRecordLoc.getType(),
+                                 Visited, Depth + 1, CreatedValuesCount);
     } else {
       if (!Visited.insert(FieldType.getCanonicalType()).second)
         return;
@@ -979,7 +1184,7 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
     }
   };
 
-  for (const auto &[Field, FieldLoc] : Loc.children()) {
+  for (const FieldDecl *Field : DACtx->getModeledFields(Type)) {
     assert(Field != nullptr);
     QualType FieldType = Field->getType();
 
@@ -988,14 +1193,12 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
                    &createLocAndMaybeValue(FieldType, Visited, Depth + 1,
                                            CreatedValuesCount));
     } else {
+      StorageLocation *FieldLoc = Loc.getChild(*Field);
       assert(FieldLoc != nullptr);
       initField(FieldType, *FieldLoc);
     }
   }
-  for (const auto &[FieldName, FieldLoc] : Loc.synthetic_fields()) {
-    assert(FieldLoc != nullptr);
-    QualType FieldType = FieldLoc->getType();
-
+  for (const auto &[FieldName, FieldType] : DACtx->getSyntheticFields(Type)) {
     // Synthetic fields cannot have reference type, so we don't need to deal
     // with this case.
     assert(!FieldType->isReferenceType());
@@ -1022,38 +1225,36 @@ StorageLocation &Environment::createObjectInternal(const ValueDecl *D,
     return createObjectInternal(D, Ty.getNonReferenceType(), nullptr);
   }
 
-  Value *Val = nullptr;
-  if (InitExpr) {
-    // In the (few) cases where an expression is intentionally
-    // "uninterpreted", `InitExpr` is not associated with a value.  There are
-    // two ways to handle this situation: propagate the status, so that
-    // uninterpreted initializers result in uninterpreted variables, or
-    // provide a default value. We choose the latter so that later refinements
-    // of the variable can be used for reasoning about the surrounding code.
-    // For this reason, we let this case be handled by the `createValue()`
-    // call below.
-    //
-    // FIXME. If and when we interpret all language cases, change this to
-    // assert that `InitExpr` is interpreted, rather than supplying a
-    // default value (assuming we don't update the environment API to return
-    // references).
-    Val = getValue(*InitExpr);
-
-    if (!Val && isa<ImplicitValueInitExpr>(InitExpr) &&
-        InitExpr->getType()->isPointerType())
-      Val = &getOrCreateNullPointerValue(InitExpr->getType()->getPointeeType());
-  }
-  if (!Val)
-    Val = createValue(Ty);
-
-  if (Ty->isRecordType())
-    return cast<RecordValue>(Val)->getLoc();
-
   StorageLocation &Loc =
       D ? createStorageLocation(*D) : createStorageLocation(Ty);
 
-  if (Val)
-    setValue(Loc, *Val);
+  if (Ty->isRecordType()) {
+    auto &RecordLoc = cast<RecordStorageLocation>(Loc);
+    if (!InitExpr)
+      initializeFieldsWithValues(RecordLoc);
+    refreshRecordValue(RecordLoc, *this);
+  } else {
+    Value *Val = nullptr;
+    if (InitExpr)
+      // In the (few) cases where an expression is intentionally
+      // "uninterpreted", `InitExpr` is not associated with a value.  There are
+      // two ways to handle this situation: propagate the status, so that
+      // uninterpreted initializers result in uninterpreted variables, or
+      // provide a default value. We choose the latter so that later refinements
+      // of the variable can be used for reasoning about the surrounding code.
+      // For this reason, we let this case be handled by the `createValue()`
+      // call below.
+      //
+      // FIXME. If and when we interpret all language cases, change this to
+      // assert that `InitExpr` is interpreted, rather than supplying a
+      // default value (assuming we don't update the environment API to return
+      // references).
+      Val = getValue(*InitExpr);
+    if (!Val)
+      Val = createValue(Ty);
+    if (Val)
+      setValue(Loc, *Val);
+  }
 
   return Loc;
 }
@@ -1072,6 +1273,8 @@ bool Environment::allows(const Formula &F) const {
 
 void Environment::dump(raw_ostream &OS) const {
   llvm::DenseMap<const StorageLocation *, std::string> LocToName;
+  if (LocForRecordReturnVal != nullptr)
+    LocToName[LocForRecordReturnVal] = "(returned record)";
   if (ThisPointeeLoc != nullptr)
     LocToName[ThisPointeeLoc] = "this";
 
@@ -1102,6 +1305,9 @@ void Environment::dump(raw_ostream &OS) const {
       if (auto Iter = LocToName.find(ReturnLoc); Iter != LocToName.end())
         OS << " (" << Iter->second << ")";
       OS << "\n";
+    } else if (Func->getReturnType()->isRecordType() ||
+               isa<CXXConstructorDecl>(Func)) {
+      OS << "LocForRecordReturnVal: " << LocForRecordReturnVal << "\n";
     } else if (!Func->getReturnType()->isVoidType()) {
       if (ReturnVal == nullptr)
         OS << "ReturnVal: nullptr\n";
@@ -1122,6 +1328,22 @@ void Environment::dump() const {
   dump(llvm::dbgs());
 }
 
+Environment::PrValueToResultObject Environment::buildResultObjectMap(
+    DataflowAnalysisContext *DACtx, const FunctionDecl *FuncDecl,
+    RecordStorageLocation *ThisPointeeLoc,
+    RecordStorageLocation *LocForRecordReturnVal) {
+  assert(FuncDecl->doesThisDeclarationHaveABody());
+
+  PrValueToResultObject Map;
+
+  ResultObjectVisitor Visitor(Map, LocForRecordReturnVal, *DACtx);
+  if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(FuncDecl))
+    Visitor.TraverseConstructorInits(Ctor, ThisPointeeLoc);
+  Visitor.TraverseStmt(FuncDecl->getBody());
+
+  return Map;
+}
+
 RecordStorageLocation *getImplicitObjectLocation(const CXXMemberCallExpr &MCE,
                                                  const Environment &Env) {
   Expr *ImplicitObject = MCE.getImplicitObjectArgument();
@@ -1216,24 +1438,11 @@ RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env) {
 RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) {
   assert(Expr.getType()->isRecordType());
 
-  if (Expr.isPRValue()) {
-    if (auto *ExistingVal = Env.get<RecordValue>(Expr)) {
-      auto &NewVal = Env.create<RecordValue>(ExistingVal->getLoc());
-      Env.setValue(Expr, NewVal);
-      Env.setValue(NewVal.getLoc(), NewVal);
-      return NewVal;
-    }
+  if (Expr.isPRValue())
+    refreshRecordValue(Env.getResultObjectLocation(Expr), Env);
 
-    auto &NewVal = *cast<RecordValue>(Env.createValue(Expr.getType()));
-    Env.setValue(Expr, NewVal);
-    return NewVal;
-  }
-
-  if (auto *Loc = Env.get<RecordStorageLocation>(Expr)) {
-    auto &NewVal = Env.create<RecordValue>(*Loc);
-    Env.setValue(*Loc, NewVal);
-    return NewVal;
-  }
+  if (auto *Loc = Env.get<RecordStorageLocation>(Expr))
+    refreshRecordValue(*Loc, Env);
 
   auto &NewVal = *cast<RecordValue>(Env.createValue(Expr.getType()));
   Env.setStorageLocation(Expr, NewVal.getLoc());