Revert "[clang][dataflow] Propagate locations from result objects to initializers."

[martinboehme]

Reverts #87320

This is causing buildbots to fail because isOriginalRecordConstructor() is now unused.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (martinboehme)

Changes

Reverts llvm/llvm-project#87320

This is causing buildbots to fail because isOriginalRecordConstructor() is now unused.

---

Patch is 53.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88315.diff

6 Files Affected:

• (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+20-44)
• (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+98-307)
• (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+98-78)
• (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+10-3)
• (modified) clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp (-43)
• (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+57-115)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 706664d7db1c25..9a65f76cdf56bc 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -30,7 +30,6 @@
 #include "llvm/ADT/MapVector.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
-#include <memory>
 #include <type_traits>
 #include <utility>
 
@@ -345,6 +344,17 @@ 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 &
@@ -452,13 +462,7 @@ 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.
-  /// 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());
-  }
+  void initializeFieldsWithValues(RecordStorageLocation &Loc);
 
   /// Assigns `Val` as the value of `Loc` in the environment.
   void setValue(const StorageLocation &Loc, Value &Val);
@@ -649,9 +653,6 @@ 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;
 
@@ -681,10 +682,8 @@ 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`). If `Type` is different from
-  /// `Loc.getType()`, initializes only those fields that are modeled for
-  /// `Type`.
-  void initializeFieldsWithValues(RecordStorageLocation &Loc, QualType Type,
+  /// `Depth`, and `CreatedValuesCount`).
+  void initializeFieldsWithValues(RecordStorageLocation &Loc,
                                   llvm::DenseSet<QualType> &Visited, int Depth,
                                   int &CreatedValuesCount);
 
@@ -703,45 +702,22 @@ 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`, `ResultObjectMap`, `ReturnVal`,
-  // `ReturnLoc` and `ThisPointeeLoc` into a separate call-context object,
-  // shared between environments in the same call.
+  // FIXME: move the fields `CallStack`, `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;
 
-  // 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 returned by the function (if it has non-reference return type).
   Value *ReturnVal = nullptr;
-  // - If the return type is a reference: Storage location of the reference
-  //   returned by the function.
+  // Storage location of the reference returned by the function (if it has
+  // reference return type).
   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 6c796b4ad923e8..1bfa7ebcfd50c9 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -15,7 +15,6 @@
 #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"
@@ -27,8 +26,6 @@
 #include <cassert>
 #include <utility>
 
-#define DEBUG_TYPE "dataflow"
-
 namespace clang {
 namespace dataflow {
 
@@ -357,8 +354,6 @@ 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);
 
@@ -391,186 +386,6 @@ 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()) {}
@@ -586,23 +401,17 @@ void Environment::initialize() {
   if (DeclCtx == nullptr)
     return;
 
-  const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx);
-  if (FuncDecl == nullptr)
-    return;
-
-  assert(FuncDecl->doesThisDeclarationHaveABody());
+  if (const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx)) {
+    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);
@@ -635,12 +444,6 @@ 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
@@ -681,18 +484,13 @@ void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
     if (getStorageLocation(*D) != nullptr)
       continue;
 
-    // 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));
+    setStorageLocation(*D, createObject(*D));
   }
 
   for (const FunctionDecl *FD : Funcs) {
     if (getStorageLocation(*FD) != nullptr)
       continue;
-    auto &Loc = createStorageLocation(*FD);
+    auto &Loc = createStorageLocation(FD->getType());
     setStorageLocation(*FD, Loc);
   }
 }
@@ -721,9 +519,6 @@ 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()));
 
@@ -734,7 +529,6 @@ 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()));
@@ -763,10 +557,6 @@ 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) {
@@ -810,9 +600,6 @@ bool Environment::equivalentTo(const Environment &Other,
   if (ReturnLoc != Other.ReturnLoc)
     return false;
 
-  if (LocForRecordReturnVal != Other.LocForRecordReturnVal)
-    return false;
-
   if (ThisPointeeLoc != Other.ThisPointeeLoc)
     return false;
 
@@ -836,10 +623,8 @@ 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;
 
@@ -871,16 +656,12 @@ 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) {
@@ -949,12 +730,6 @@ 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
- ...
[truncated]