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

via cfe-commits cfe-commits at lists.llvm.org
Wed Apr 10 00:11:10 PDT 2024


https://github.com/martinboehme updated https://github.com/llvm/llvm-project/pull/87320

>From 2d2aa88aab3e47e41588397471a90c03bb55d900 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Tue, 2 Apr 2024 08:00:00 +0000
Subject: [PATCH 1/3] [clang][dataflow] Propagate locations from result objects
 to initializers.

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.
---
 .../FlowSensitive/DataflowEnvironment.h       |  64 ++-
 .../FlowSensitive/DataflowEnvironment.cpp     | 402 +++++++++++++-----
 clang/lib/Analysis/FlowSensitive/Transfer.cpp | 172 ++++----
 .../TypeErasedDataflowAnalysis.cpp            |  13 +-
 .../FlowSensitive/DataflowEnvironmentTest.cpp |  43 ++
 .../Analysis/FlowSensitive/TransferTest.cpp   | 172 +++++---
 6 files changed, 582 insertions(+), 284 deletions(-)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index c30bccd06674a4..dc3a9c239552be 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -30,6 +30,7 @@
 #include "llvm/ADT/MapVector.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
+#include <memory>
 #include <type_traits>
 #include <utility>
 
@@ -330,17 +331,6 @@ class Environment {
   /// location of the result object to pass in `this`, even though prvalues are
   /// otherwise not associated with storage locations.
   ///
-  /// FIXME: Currently, this simply returns a stable storage location for `E`,
-  /// but this doesn't do the right thing in scenarios like the following:
-  /// ```
-  /// MyClass c = some_condition()? MyClass(foo) : MyClass(bar);
-  /// ```
-  /// Here, `MyClass(foo)` and `MyClass(bar)` will have two different storage
-  /// locations, when in fact their storage locations should be the same.
-  /// Eventually, we want to propagate storage locations from result objects
-  /// down to the prvalues that initialize them, similar to the way that this is
-  /// done in Clang's CodeGen.
-  ///
   /// Requirements:
   ///  `E` must be a prvalue of record type.
   RecordStorageLocation &
@@ -448,7 +438,13 @@ class Environment {
   /// Initializes the fields (including synthetic fields) of `Loc` with values,
   /// unless values of the field type are not supported or we hit one of the
   /// limits at which we stop producing values.
-  void initializeFieldsWithValues(RecordStorageLocation &Loc);
+  /// If `Type` is provided, initializes only those fields that are modeled for
+  /// `Type`; this is intended for use in cases where `Loc` is a derived type
+  /// and we only want to initialize the fields of a base type.
+  void initializeFieldsWithValues(RecordStorageLocation &Loc, QualType Type);
+  void initializeFieldsWithValues(RecordStorageLocation &Loc) {
+    initializeFieldsWithValues(Loc, Loc.getType());
+  }
 
   /// Assigns `Val` as the value of `Loc` in the environment.
   void setValue(const StorageLocation &Loc, Value &Val);
@@ -639,6 +635,9 @@ class Environment {
   LLVM_DUMP_METHOD void dump(raw_ostream &OS) const;
 
 private:
+  using PrValueToResultObject =
+      llvm::DenseMap<const Expr *, RecordStorageLocation *>;
+
   // The copy-constructor is for use in fork() only.
   Environment(const Environment &) = default;
 
@@ -668,8 +667,10 @@ class Environment {
   /// Initializes the fields (including synthetic fields) of `Loc` with values,
   /// unless values of the field type are not supported or we hit one of the
   /// limits at which we stop producing values (controlled by `Visited`,
-  /// `Depth`, and `CreatedValuesCount`).
-  void initializeFieldsWithValues(RecordStorageLocation &Loc,
+  /// `Depth`, and `CreatedValuesCount`). If `Type` is different from
+  /// `Loc.getType()`, initializes only those fields that are modeled for
+  /// `Type`.
+  void initializeFieldsWithValues(RecordStorageLocation &Loc, QualType Type,
                                   llvm::DenseSet<QualType> &Visited, int Depth,
                                   int &CreatedValuesCount);
 
@@ -688,22 +689,45 @@ class Environment {
   /// and functions referenced in `FuncDecl`. `FuncDecl` must have a body.
   void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl);
 
+  static PrValueToResultObject
+  buildResultObjectMap(DataflowAnalysisContext *DACtx,
+                       const FunctionDecl *FuncDecl,
+                       RecordStorageLocation *ThisPointeeLoc,
+                       RecordStorageLocation *LocForRecordReturnVal);
+
   // `DACtx` is not null and not owned by this object.
   DataflowAnalysisContext *DACtx;
 
-  // FIXME: move the fields `CallStack`, `ReturnVal`, `ReturnLoc` and
-  // `ThisPointeeLoc` into a separate call-context object, shared between
-  // environments in the same call.
+  // FIXME: move the fields `CallStack`, `ResultObjectMap`, `ReturnVal`,
+  // `ReturnLoc` and `ThisPointeeLoc` into a separate call-context object,
+  // shared between environments in the same call.
   // https://github.com/llvm/llvm-project/issues/59005
 
   // `DeclContext` of the block being analysed if provided.
   std::vector<const DeclContext *> CallStack;
 
-  // Value returned by the function (if it has non-reference return type).
+  // Maps from prvalues of record type to their result objects. Shared between
+  // all environments for the same function.
+  // FIXME: It's somewhat unsatisfactory that we have to use a `shared_ptr`
+  // here, though the cost is acceptable: The overhead of a `shared_ptr` is
+  // incurred when it is copied, and this happens only relatively rarely (when
+  // we fork the environment). The need for a `shared_ptr` will go away once we
+  // introduce a shared call-context object (see above).
+  std::shared_ptr<PrValueToResultObject> ResultObjectMap;
+
+  // The following three member variables handle various different types of
+  // return values.
+  // - If the return type is not a reference and not a record: Value returned
+  //   by the function.
   Value *ReturnVal = nullptr;
-  // Storage location of the reference returned by the function (if it has
-  // reference return type).
+  // - If the return type is a reference: Storage location of the reference
+  //   returned by the function.
   StorageLocation *ReturnLoc = nullptr;
+  // - If the return type is a record or the function being analyzed is a
+  //   constructor: Storage location into which the return value should be
+  //   constructed.
+  RecordStorageLocation *LocForRecordReturnVal = nullptr;
+
   // The storage location of the `this` pointee. Should only be null if the
   // function being analyzed is only a function and not a method.
   RecordStorageLocation *ThisPointeeLoc = nullptr;
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index f729d676dd0de8..67d5c444d52cb6 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"
@@ -353,6 +354,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);
 
@@ -385,6 +388,185 @@ 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()) {
+        for (Expr *Init : InitList->inits())
+          PropagateResultObject(Init, 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.
+    llvm::SmallVector<Stmt *> Children(E->child_begin(), E->child_end());
+    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()) {}
@@ -400,17 +582,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);
@@ -443,6 +631,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
@@ -483,13 +677,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);
   }
 }
@@ -518,6 +717,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()));
 
@@ -528,6 +730,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()));
@@ -556,6 +759,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) {
@@ -599,6 +806,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;
 
@@ -622,8 +832,10 @@ LatticeJoinEffect 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 = LatticeJoinEffect::Unchanged;
 
@@ -655,12 +867,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) {
@@ -729,6 +945,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;
 }
 
@@ -790,50 +1012,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';
   }
 }
 
@@ -847,8 +1048,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));
   }
 
   assert(CanonE.isPRValue());
@@ -926,7 +1126,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);
   }
@@ -958,6 +1159,7 @@ Environment::createLocAndMaybeValue(QualType Ty,
 }
 
 void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
+                                             QualType Type,
                                              llvm::DenseSet<QualType> &Visited,
                                              int Depth,
                                              int &CreatedValuesCount) {
@@ -965,8 +1167,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;
@@ -977,7 +1179,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();
 
@@ -986,14 +1188,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());
@@ -1020,38 +1220,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;
 }
@@ -1070,6 +1268,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";
 
@@ -1100,6 +1300,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";
@@ -1120,6 +1323,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();
@@ -1214,24 +1433,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());
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 0a2e8368d541dd..670eed3b5d6173 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -460,11 +460,9 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     // So make sure we have a value if we didn't propagate one above.
     if (S->isPRValue() && S->getType()->isRecordType()) {
       if (Env.getValue(*S) == nullptr) {
-        Value *Val = Env.createValue(S->getType());
-        // We're guaranteed to always be able to create a value for record
-        // types.
-        assert(Val != nullptr);
-        Env.setValue(*S, *Val);
+        auto &Loc = Env.getResultObjectLocation(*S);
+        Env.initializeFieldsWithValues(Loc);
+        refreshRecordValue(Loc, Env);
       }
     }
   }
@@ -472,13 +470,25 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
   void VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *S) {
     const Expr *InitExpr = S->getExpr();
     assert(InitExpr != nullptr);
-    propagateValueOrStorageLocation(*InitExpr, *S, Env);
+    if (!(S->getType()->isRecordType() && S->isPRValue()))
+      propagateValueOrStorageLocation(*InitExpr, *S, Env);
   }
 
   void VisitCXXConstructExpr(const CXXConstructExpr *S) {
     const CXXConstructorDecl *ConstructorDecl = S->getConstructor();
     assert(ConstructorDecl != nullptr);
 
+    // `CXXConstructExpr` can have array type if default-initializing an array
+    // of records. We don't handle this specifically beyond potentially inlining
+    // the call.
+    if (!S->getType()->isRecordType()) {
+      transferInlineCall(S, ConstructorDecl);
+      return;
+    }
+
+    RecordStorageLocation &Loc = Env.getResultObjectLocation(*S);
+    Env.setValue(*S, refreshRecordValue(Loc, Env));
+
     if (ConstructorDecl->isCopyOrMoveConstructor()) {
       // It is permissible for a copy/move constructor to have additional
       // parameters as long as they have default arguments defined for them.
@@ -491,24 +501,14 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       if (ArgLoc == nullptr)
         return;
 
-      if (S->isElidable()) {
-        if (Value *Val = Env.getValue(*ArgLoc))
-          Env.setValue(*S, *Val);
-      } else {
-        auto &Val = *cast<RecordValue>(Env.createValue(S->getType()));
-        Env.setValue(*S, Val);
-        copyRecord(*ArgLoc, Val.getLoc(), Env);
-      }
+      // Even if the copy/move constructor call is elidable, we choose to copy
+      // the record in all cases (which isn't wrong, just potentially not
+      // optimal).
+      copyRecord(*ArgLoc, Loc, Env);
       return;
     }
 
-    // `CXXConstructExpr` can have array type if default-initializing an array
-    // of records, and we currently can't create values for arrays. So check if
-    // we've got a record type.
-    if (S->getType()->isRecordType()) {
-      auto &InitialVal = *cast<RecordValue>(Env.createValue(S->getType()));
-      Env.setValue(*S, InitialVal);
-    }
+    Env.initializeFieldsWithValues(Loc, S->getType());
 
     transferInlineCall(S, ConstructorDecl);
   }
@@ -551,19 +551,15 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       if (S->isGLValue()) {
         Env.setStorageLocation(*S, *LocDst);
       } else if (S->getType()->isRecordType()) {
-        // Make sure that we have a `RecordValue` for this expression so that
-        // `Environment::getResultObjectLocation()` is able to return a location
-        // for it.
-        if (Env.getValue(*S) == nullptr)
-          refreshRecordValue(*S, Env);
+        // Assume that the assignment returns the assigned value.
+        copyRecord(*LocDst, Env.getResultObjectLocation(*S), Env);
       }
 
       return;
     }
 
-    // CXXOperatorCallExpr can be prvalues. Call `VisitCallExpr`() to create
-    // a `RecordValue` for them so that `Environment::getResultObjectLocation()`
-    // can return a value.
+    // `CXXOperatorCallExpr` can be a prvalue. Call `VisitCallExpr`() to
+    // initialize the prvalue's fields with values.
     VisitCallExpr(S);
   }
 
@@ -580,11 +576,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     }
   }
 
-  void VisitCXXTemporaryObjectExpr(const CXXTemporaryObjectExpr *S) {
-    if (Value *Val = Env.createValue(S->getType()))
-      Env.setValue(*S, *Val);
-  }
-
   void VisitCallExpr(const CallExpr *S) {
     // Of clang's builtins, only `__builtin_expect` is handled explicitly, since
     // others (like trap, debugtrap, and unreachable) are handled by CFG
@@ -612,13 +603,14 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     } else if (const FunctionDecl *F = S->getDirectCallee()) {
       transferInlineCall(S, F);
 
-      // If this call produces a prvalue of record type, make sure that we have
-      // a `RecordValue` for it. This is required so that
-      // `Environment::getResultObjectLocation()` is able to return a location
-      // for this `CallExpr`.
+      // If this call produces a prvalue of record type, initialize its fields
+      // with values.
       if (S->getType()->isRecordType() && S->isPRValue())
-        if (Env.getValue(*S) == nullptr)
-          refreshRecordValue(*S, Env);
+        if (Env.getValue(*S) == nullptr) {
+          RecordStorageLocation &Loc = Env.getResultObjectLocation(*S);
+          Env.initializeFieldsWithValues(Loc);
+          Env.setValue(*S, refreshRecordValue(Loc, Env));
+        }
     }
   }
 
@@ -666,8 +658,10 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     // `getLogicOperatorSubExprValue()`.
     if (S->isGLValue())
       Env.setStorageLocation(*S, Env.createObject(S->getType()));
-    else if (Value *Val = Env.createValue(S->getType()))
-      Env.setValue(*S, *Val);
+    else if (!S->getType()->isRecordType()) {
+      if (Value *Val = Env.createValue(S->getType()))
+        Env.setValue(*S, *Val);
+    }
   }
 
   void VisitInitListExpr(const InitListExpr *S) {
@@ -688,71 +682,51 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
       return;
     }
 
-    llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
-    RecordInitListHelper InitListHelper(S);
+    RecordStorageLocation &Loc = Env.getResultObjectLocation(*S);
+    Env.setValue(*S, refreshRecordValue(Loc, Env));
 
-    for (auto [Base, Init] : InitListHelper.base_inits()) {
-      assert(Base->getType().getCanonicalType() ==
-             Init->getType().getCanonicalType());
-      auto *BaseVal = Env.get<RecordValue>(*Init);
-      if (!BaseVal)
-        BaseVal = cast<RecordValue>(Env.createValue(Init->getType()));
-      // Take ownership of the fields of the `RecordValue` for the base class
-      // and incorporate them into the "flattened" set of fields for the
-      // derived class.
-      auto Children = BaseVal->getLoc().children();
-      FieldLocs.insert(Children.begin(), Children.end());
-    }
+    // Initialization of base classes and fields of record type happens when we
+    // visit the nested `CXXConstructExpr` or `InitListExpr` for that base class
+    // or field. We therefore only need to deal with fields of non-record type
+    // here.
 
-    for (auto [Field, Init] : InitListHelper.field_inits()) {
-      assert(
-          // The types are same, or
-          Field->getType().getCanonicalType().getUnqualifiedType() ==
-              Init->getType().getCanonicalType().getUnqualifiedType() ||
-          // The field's type is T&, and initializer is T
-          (Field->getType()->isReferenceType() &&
-           Field->getType().getCanonicalType()->getPointeeType() ==
-               Init->getType().getCanonicalType()));
-      auto& Loc = Env.createObject(Field->getType(), Init);
-      FieldLocs.insert({Field, &Loc});
-    }
+    RecordInitListHelper InitListHelper(S);
 
-    // In the case of a union, we don't in general have initializers for all
-    // of the fields. Create storage locations for the remaining fields (but
-    // don't associate them with values).
-    if (Type->isUnionType()) {
-      for (const FieldDecl *Field :
-           Env.getDataflowAnalysisContext().getModeledFields(Type)) {
-        if (auto [it, inserted] = FieldLocs.insert({Field, nullptr}); inserted)
-          it->second = &Env.createStorageLocation(Field->getType());
+    for (auto [Field, Init] : InitListHelper.field_inits()) {
+      if (Field->getType()->isRecordType())
+        continue;
+      if (Field->getType()->isReferenceType()) {
+        assert(Field->getType().getCanonicalType()->getPointeeType() ==
+               Init->getType().getCanonicalType());
+        Loc.setChild(*Field, &Env.createObject(Field->getType(), Init));
+        continue;
       }
+      assert(Field->getType().getCanonicalType().getUnqualifiedType() ==
+             Init->getType().getCanonicalType().getUnqualifiedType());
+      StorageLocation *FieldLoc = Loc.getChild(*Field);
+      // Locations for non-reference fields must always be non-null.
+      assert(FieldLoc != nullptr);
+      Value *Val = Env.getValue(*Init);
+      if (Val == nullptr && isa<ImplicitValueInitExpr>(Init) &&
+          Init->getType()->isPointerType())
+        Val =
+            &Env.getOrCreateNullPointerValue(Init->getType()->getPointeeType());
+      if (Val == nullptr)
+        Val = Env.createValue(Field->getType());
+      if (Val != nullptr)
+        Env.setValue(*FieldLoc, *Val);
     }
 
-    // Check that we satisfy the invariant that a `RecordStorageLoation`
-    // contains exactly the set of modeled fields for that type.
-    // `ModeledFields` includes fields from all the bases, but only the
-    // modeled ones. However, if a class type is initialized with an
-    // `InitListExpr`, all fields in the class, including those from base
-    // classes, are included in the set of modeled fields. The code above
-    // should therefore populate exactly the modeled fields.
-    assert(containsSameFields(
-        Env.getDataflowAnalysisContext().getModeledFields(Type), FieldLocs));
-
-    RecordStorageLocation::SyntheticFieldMap SyntheticFieldLocs;
-    for (const auto &Entry :
-         Env.getDataflowAnalysisContext().getSyntheticFields(Type)) {
-      SyntheticFieldLocs.insert(
-          {Entry.getKey(), &Env.createObject(Entry.getValue())});
+    for (const auto &[FieldName, FieldLoc] : Loc.synthetic_fields()) {
+      QualType FieldType = FieldLoc->getType();
+      if (FieldType->isRecordType()) {
+        Env.initializeFieldsWithValues(*cast<RecordStorageLocation>(FieldLoc));
+      } else {
+        if (Value *Val = Env.createValue(FieldType))
+          Env.setValue(*FieldLoc, *Val);
+      }
     }
 
-    auto &Loc = Env.getDataflowAnalysisContext().createRecordStorageLocation(
-        Type, std::move(FieldLocs), std::move(SyntheticFieldLocs));
-    RecordValue &RecordVal = Env.create<RecordValue>(Loc);
-
-    Env.setValue(Loc, RecordVal);
-
-    Env.setValue(*S, RecordVal);
-
     // FIXME: Implement array initialization.
   }
 
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 595f70f819ddb5..1b73c5d6830161 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -369,17 +369,10 @@ builtinTransferInitializer(const CFGInitializer &Elt,
     ParentLoc->setChild(*Member, InitExprLoc);
   } else if (auto *InitExprVal = Env.getValue(*InitExpr)) {
     assert(MemberLoc != nullptr);
-    if (Member->getType()->isRecordType()) {
-      auto *InitValStruct = cast<RecordValue>(InitExprVal);
-      // FIXME: Rather than performing a copy here, we should really be
-      // initializing the field in place. This would require us to propagate the
-      // storage location of the field to the AST node that creates the
-      // `RecordValue`.
-      copyRecord(InitValStruct->getLoc(),
-                 *cast<RecordStorageLocation>(MemberLoc), Env);
-    } else {
+    // Record-type initializers construct themselves directly into the result
+    // object, so there is no need to handle them here.
+    if (!Member->getType()->isRecordType())
       Env.setValue(*MemberLoc, *InitExprVal);
-    }
   }
 }
 
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 465a8e21690c4a..cc20623f881ff4 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -24,6 +24,7 @@ namespace {
 
 using namespace clang;
 using namespace dataflow;
+using ::clang::dataflow::test::findValueDecl;
 using ::clang::dataflow::test::getFieldValue;
 using ::testing::Contains;
 using ::testing::IsNull;
@@ -199,6 +200,48 @@ TEST_F(EnvironmentTest, JoinRecords) {
   }
 }
 
+TEST_F(EnvironmentTest, DifferentReferenceLocInJoin) {
+  // This tests the case where the storage location for a reference-type
+  // variable is different for two states being joined. We used to believe this
+  // could not happen and therefore had an assertion disallowing this; this test
+  // exists to demonstrate that we can handle this condition without a failing
+  // assertion. See also the discussion here:
+  // https://discourse.llvm.org/t/70086/6
+
+  using namespace ast_matchers;
+
+  std::string Code = R"cc(
+    void f(int &ref) {}
+  )cc";
+
+  auto Unit =
+      tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"});
+  auto &Context = Unit->getASTContext();
+
+  ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+  const ValueDecl *Ref = findValueDecl(Context, "ref");
+
+  Environment Env1(DAContext);
+  StorageLocation &Loc1 = Env1.createStorageLocation(Context.IntTy);
+  Env1.setStorageLocation(*Ref, Loc1);
+
+  Environment Env2(DAContext);
+  StorageLocation &Loc2 = Env2.createStorageLocation(Context.IntTy);
+  Env2.setStorageLocation(*Ref, Loc2);
+
+  EXPECT_NE(&Loc1, &Loc2);
+
+  Environment::ValueModel Model;
+  Environment EnvJoined =
+      Environment::join(Env1, Env2, Model, Environment::DiscardExprState);
+
+  // Joining environments with different storage locations for the same
+  // declaration results in the declaration being removed from the joined
+  // environment.
+  EXPECT_EQ(EnvJoined.getStorageLocation(*Ref), nullptr);
+}
+
 TEST_F(EnvironmentTest, InitGlobalVarsFun) {
   using namespace ast_matchers;
 
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index ca055a462a2866..00dafb2988c690 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1582,10 +1582,9 @@ TEST(TransferTest, FieldsDontHaveValuesInConstructorWithBaseClass) {
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
-        // FIXME: The field of the base class should already have been
-        // initialized with a value by the base constructor. This test documents
-        // the current buggy behavior.
-        EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "BaseVal",
+        // The field of the base class should already have been initialized with
+        // a value by the base constructor.
+        EXPECT_NE(getFieldValue(Env.getThisPointeeStorageLocation(), "BaseVal",
                                 ASTCtx, Env),
                   nullptr);
         EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "Val",
@@ -2998,8 +2997,12 @@ TEST(TransferTest, ResultObjectLocation) {
 
 TEST(TransferTest, ResultObjectLocationForDefaultArgExpr) {
   std::string Code = R"(
-    struct S {};
-    void funcWithDefaultArg(S s = S());
+    struct Inner {};
+    struct Outer {
+        Inner I = {};
+    };
+
+    void funcWithDefaultArg(Outer O = {});
     void target() {
       funcWithDefaultArg();
       // [[p]]
@@ -3058,13 +3061,7 @@ TEST(TransferTest, ResultObjectLocationForDefaultInitExpr) {
 
         RecordStorageLocation &Loc = Env.getResultObjectLocation(*DefaultInit);
 
-        // FIXME: The result object location for the `CXXDefaultInitExpr` should
-        // be the location of the member variable being initialized, but we
-        // don't do this correctly yet; see also comments in
-        // `builtinTransferInitializer()`.
-        // For the time being, we just document the current erroneous behavior
-        // here (this should be `EXPECT_EQ` when the behavior is fixed).
-        EXPECT_NE(&Loc, Env.getThisPointeeStorageLocation()->getChild(*SField));
+        EXPECT_EQ(&Loc, Env.getThisPointeeStorageLocation()->getChild(*SField));
       });
 }
 
@@ -3101,6 +3098,79 @@ TEST(TransferTest, ResultObjectLocationForCXXOperatorCallExpr) {
       });
 }
 
+TEST(TransferTest, ResultObjectLocationForStdInitializerListExpr) {
+  std::string Code = R"(
+    namespace std {
+    template <typename T>
+    struct initializer_list {};
+    } // namespace std
+
+    void target() {
+      std::initializer_list<int> list = {1};
+      // [[p]]
+    }
+  )";
+
+  using ast_matchers::cxxStdInitializerListExpr;
+  using ast_matchers::match;
+  using ast_matchers::selectFirst;
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto *StdInitList = selectFirst<CXXStdInitializerListExpr>(
+            "std_init_list",
+            match(cxxStdInitializerListExpr().bind("std_init_list"), ASTCtx));
+        ASSERT_NE(StdInitList, nullptr);
+
+        EXPECT_EQ(&Env.getResultObjectLocation(*StdInitList),
+                  &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "list"));
+      });
+}
+
+TEST(TransferTest, ResultObjectLocationPropagatesThroughConditionalOperator) {
+  std::string Code = R"(
+    struct A {
+      A(int);
+    };
+
+    void target(bool b) {
+      A a = b ? A(0) : A(1);
+      (void)0; // [[p]]
+    }
+  )";
+  using ast_matchers::cxxConstructExpr;
+  using ast_matchers::equals;
+  using ast_matchers::hasArgument;
+  using ast_matchers::integerLiteral;
+  using ast_matchers::match;
+  using ast_matchers::selectFirst;
+  using ast_matchers::traverse;
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto *ConstructExpr0 = selectFirst<CXXConstructExpr>(
+            "construct",
+            match(cxxConstructExpr(hasArgument(0, integerLiteral(equals(0))))
+                      .bind("construct"),
+                  ASTCtx));
+        auto *ConstructExpr1 = selectFirst<CXXConstructExpr>(
+            "construct",
+            match(cxxConstructExpr(hasArgument(0, integerLiteral(equals(1))))
+                      .bind("construct"),
+                  ASTCtx));
+
+        auto &ALoc = getLocForDecl<StorageLocation>(ASTCtx, Env, "a");
+        EXPECT_EQ(&Env.getResultObjectLocation(*ConstructExpr0), &ALoc);
+        EXPECT_EQ(&Env.getResultObjectLocation(*ConstructExpr1), &ALoc);
+      });
+}
+
 TEST(TransferTest, StaticCast) {
   std::string Code = R"(
     void target(int Foo) {
@@ -5886,6 +5956,38 @@ TEST(TransferTest, ContextSensitiveReturnRecord) {
       {BuiltinOptions{ContextSensitiveOptions{}}});
 }
 
+TEST(TransferTest, ContextSensitiveReturnSelfReferentialRecord) {
+  std::string Code = R"(
+    struct S {
+      S() { self = this; }
+      S *self;
+    };
+
+    S makeS() {
+      // RVO guarantees that this will be constructed directly into `MyS`.
+      return S();
+    }
+
+    void target() {
+      S MyS = makeS();
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto &MySLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "MyS");
+
+        auto *SelfVal =
+            cast<PointerValue>(getFieldValue(&MySLoc, "self", ASTCtx, Env));
+        EXPECT_EQ(&SelfVal->getPointeeLoc(), &MySLoc);
+      },
+      {BuiltinOptions{ContextSensitiveOptions{}}});
+}
+
 TEST(TransferTest, ContextSensitiveMethodLiteral) {
   std::string Code = R"(
     class MyClass {
@@ -6830,50 +6932,6 @@ TEST(TransferTest, LambdaCaptureThis) {
       });
 }
 
-TEST(TransferTest, DifferentReferenceLocInJoin) {
-  // This test triggers a case where the storage location for a reference-type
-  // variable is different for two states being joined. We used to believe this
-  // could not happen and therefore had an assertion disallowing this; this test
-  // exists to demonstrate that we can handle this condition without a failing
-  // assertion. See also the discussion here:
-  // https://discourse.llvm.org/t/70086/6
-  std::string Code = R"(
-    namespace std {
-      template <class T> struct initializer_list {
-        const T* begin();
-        const T* end();
-      };
-    }
-
-    void target(char* p, char* end) {
-      while (p != end) {
-        if (*p == ' ') {
-          p++;
-          continue;
-        }
-
-        auto && range = {1, 2};
-        for (auto b = range.begin(), e = range.end(); b != e; ++b) {
-        }
-        (void)0;
-        // [[p]]
-      }
-    }
-  )";
-  runDataflow(
-      Code,
-      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
-         ASTContext &ASTCtx) {
-        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
-
-        // Joining environments with different storage locations for the same
-        // declaration results in the declaration being removed from the joined
-        // environment.
-        const ValueDecl *VD = findValueDecl(ASTCtx, "range");
-        ASSERT_EQ(Env.getStorageLocation(*VD), nullptr);
-      });
-}
-
 // This test verifies correct modeling of a relational dependency that goes
 // through unmodeled functions (the simple `cond()` in this case).
 TEST(TransferTest, ConditionalRelation) {

>From cdaf3b22c5b8d94e0157f4945e3e9905e21ffbc0 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Mon, 8 Apr 2024 13:32:50 +0000
Subject: [PATCH 2/3] fixup! [clang][dataflow] Propagate locations from result
 objects to initializers.

---
 .../lib/Analysis/FlowSensitive/DataflowEnvironment.cpp |  8 ++++++--
 clang/lib/Analysis/FlowSensitive/Transfer.cpp          | 10 ++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 67d5c444d52cb6..bb1ddd535241ef 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -27,6 +27,8 @@
 #include <cassert>
 #include <utility>
 
+#define DEBUG_TYPE "dataflow"
+
 namespace clang {
 namespace dataflow {
 
@@ -552,8 +554,10 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
     // All other expression nodes that propagate a record prvalue should have
     // exactly one child.
     llvm::SmallVector<Stmt *> Children(E->child_begin(), E->child_end());
-    if (Children.size() != 1)
-      E->dump();
+    LLVM_DEBUG({
+      if (Children.size() != 1)
+        E->dump();
+    });
     assert(Children.size() == 1);
     for (Stmt *S : Children)
       PropagateResultObject(cast<Expr>(S), Loc);
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 670eed3b5d6173..88a9c0eccbebc0 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -470,8 +470,14 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
   void VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *S) {
     const Expr *InitExpr = S->getExpr();
     assert(InitExpr != nullptr);
-    if (!(S->getType()->isRecordType() && S->isPRValue()))
-      propagateValueOrStorageLocation(*InitExpr, *S, Env);
+
+    // If this is a prvalue of record type, the handler for `*InitExpr` (if one
+    // exists) will initialize the result object; there is no value to propgate
+    // here.
+    if (S->getType()->isRecordType() && S->isPRValue())
+      return;
+
+    propagateValueOrStorageLocation(*InitExpr, *S, Env);
   }
 
   void VisitCXXConstructExpr(const CXXConstructExpr *S) {

>From eb3392d1101726f9b125376a7ca9ac17da335469 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 10 Apr 2024 07:10:48 +0000
Subject: [PATCH 3/3] fixup! fixup! [clang][dataflow] Propagate locations from
 result objects to initializers.

---
 clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index bb1ddd535241ef..def1daa18f4e17 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -512,8 +512,7 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
       if (!InitList->isSemanticForm())
         return;
       if (InitList->isTransparent()) {
-        for (Expr *Init : InitList->inits())
-          PropagateResultObject(Init, Loc);
+        PropagateResultObject(InitList->getInit(0), Loc);
         return;
       }
 
@@ -553,7 +552,7 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
 
     // All other expression nodes that propagate a record prvalue should have
     // exactly one child.
-    llvm::SmallVector<Stmt *> Children(E->child_begin(), E->child_end());
+    SmallVector<Stmt *, 1> Children(E->child_begin(), E->child_end());
     LLVM_DEBUG({
       if (Children.size() != 1)
         E->dump();



More information about the cfe-commits mailing list