[clang][dataflow] Add `DataflowEnvironment::createObject()`.

[mboehme]

Summary

This consolidates the code used in various places to initialize objects (usually for variables) into one central location.

It will also help reduce the number of changes needed when we make the upcoming API changes to AggregateStorageLocation and StructValue.

Depends On D155074

[ymandel]

Looks great!

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

621

At least document the nullability of D and InitExpr?

[mboehme]

Rebased to head

[mboehme]

Add comment for createObjectInternal()

clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

public:
   /// necessary storage locations and values for indirections until it finds a
   /// non-pointer/non-reference type.
   ///
   /// Requirements:
   ///
   ///  `Type` must not be null.
   Value *createValue(QualType Type);
 
+  /// Creates an object (i.e. a storage location with an associated value) of
+  /// type `Ty`. If `InitExpr` is non-null and has a value associated with it,
+  /// initializes the object with this value. Otherwise, initializes the object
+  /// with a value created using `createValue()`.
+  StorageLocation &createObject(QualType Ty, const Expr *InitExpr = nullptr) {
+    return createObjectInternal(nullptr, Ty, InitExpr);
+  }
+
+  /// Creates an object for the variable declaration `D`. If `D` has an
+  /// initializer and this initializer is associated with a value, initializes
+  /// the object with this value.  Otherwise, initializes the object with a
+  /// value created using `createValue()`. Uses the storage location returned by
+  /// `DataflowAnalysisContext::getStableStorageLocation(D)`.
+  StorageLocation &createObject(const VarDecl &D) {
+    return createObjectInternal(&D, D.getType(), D.getInit());
+  }
+
+  /// Creates an object for the variable declaration `D`. If `InitExpr` is
+  /// non-null and has a value associated with it, initializes the object with
+  /// this value. Otherwise, initializes the object with a value created using
+  /// `createValue()`.  Uses the storage location returned by
+  /// `DataflowAnalysisContext::getStableStorageLocation(D)`.
+  StorageLocation &createObject(const VarDecl &D, const Expr *InitExpr) {
+    return createObjectInternal(&D, D.getType(), InitExpr);
+  }
+
   /// Assigns `Val` as the value of `Loc` in the environment.
   void setValue(const StorageLocation &Loc, Value &Val);
 
   /// Clears any association between `Loc` and a value in the environment.
   void clearValue(const StorageLocation &Loc);
 
   /// Assigns `Val` as the value of the prvalue `E` in the environment.
   ///
private:
   ///
   /// Requirements:
   ///
   ///  `Type` must not be null.
   Value *createValueUnlessSelfReferential(QualType Type,
                                           llvm::DenseSet<QualType> &Visited,
                                           int Depth, int &CreatedValuesCount);
 
+  /// Shared implementation of `createObject()` overloads.
+  /// `D` and `InitExpr` may be null.
+  StorageLocation &createObjectInternal(const VarDecl *D, QualType Ty,
+                                        const Expr *InitExpr);
+
   StorageLocation &skip(StorageLocation &Loc, SkipPast SP) const;
   const StorageLocation &skip(const StorageLocation &Loc, SkipPast SP) const;
 
   /// Shared implementation of `pushCall` overloads. Note that unlike
   /// `pushCall`, this member is invoked on the environment of the callee, not
   /// of the caller.
   void pushCallInternal(const FunctionDecl *FuncDecl,
                         ArrayRef<const Expr *> Args);

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

StorageLocation &DataflowAnalysisContext::createStorageLocation(QualType Type) {
   }
   return arena().create<ScalarStorageLocation>(Type);
 }
 
 StorageLocation &
 DataflowAnalysisContext::getStableStorageLocation(const VarDecl &D) {
   if (auto *Loc = getStorageLocation(D))
     return *Loc;
-  auto &Loc = createStorageLocation(D.getType());
+  auto &Loc = createStorageLocation(D.getType().getNonReferenceType());
   setStorageLocation(D, Loc);
   return Loc;
 }
 
 StorageLocation &
 DataflowAnalysisContext::getStableStorageLocation(const Expr &E) {
   if (auto *Loc = getStorageLocation(E))
     return *Loc;

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
 
   // These have to be added before the lines that follow to ensure that
   // `create*` work correctly for structs.
   DACtx->addModeledFields(Fields);
 
   for (const VarDecl *D : Vars) {
     if (getStorageLocation(*D) != nullptr)
       continue;
-    auto &Loc = createStorageLocation(D->getType().getNonReferenceType());
-    setStorageLocation(*D, Loc);
-    if (auto *Val = createValue(D->getType().getNonReferenceType()))
-      setValue(Loc, *Val);
+
+    setStorageLocation(*D, createObject(*D));
   }
 
   for (const FunctionDecl *FD : Funcs) {
     if (getStorageLocation(*FD) != nullptr)
       continue;
     auto &Loc = createStorageLocation(FD->getType());
     setStorageLocation(*FD, Loc);
   }
Environment::Environment(DataflowAnalysisContext &DACtx,
 
   if (const auto *FuncDecl = dyn_cast<FunctionDecl>(&DeclCtx)) {
     assert(FuncDecl->getBody() != nullptr);
 
     initFieldsGlobalsAndFuncs(FuncDecl);
 
     for (const auto *ParamDecl : FuncDecl->parameters()) {
       assert(ParamDecl != nullptr);
-      // References aren't objects, so the reference itself doesn't have a
-      // storage location. Instead, the storage location for a reference refers
-      // directly to an object of the referenced type -- so strip off any
-      // reference from the type.
-      auto &ParamLoc =
-          createStorageLocation(ParamDecl->getType().getNonReferenceType());
-      setStorageLocation(*ParamDecl, ParamLoc);
-      if (Value *ParamVal =
-              createValue(ParamDecl->getType().getNonReferenceType()))
-          setValue(ParamLoc, *ParamVal);
+      setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr));
     }
   }
 
   if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) {
     auto *Parent = MethodDecl->getParent();
     assert(Parent != nullptr);
     if (Parent->isLambda())
       MethodDecl = dyn_cast<CXXMethodDecl>(Parent->getDeclContext());
void Environment::pushCallInternal(const FunctionDecl *FuncDecl,
   initFieldsGlobalsAndFuncs(FuncDecl);
 
   const auto *ParamIt = FuncDecl->param_begin();
 
   // FIXME: Parameters don't always map to arguments 1:1; examples include
   // overloaded operators implemented as member functions, and parameter packs.
   for (unsigned ArgIndex = 0; ArgIndex < Args.size(); ++ParamIt, ++ArgIndex) {
     assert(ParamIt != FuncDecl->param_end());
-
-    const Expr *Arg = Args[ArgIndex];
-    auto *ArgLoc = getStorageLocation(*Arg, SkipPast::Reference);
-    if (ArgLoc == nullptr)
-      continue;
-
     const VarDecl *Param = *ParamIt;
-
-    QualType ParamType = Param->getType();
-    if (ParamType->isReferenceType()) {
-      setStorageLocation(*Param, *ArgLoc);
-    } else {
-      auto &Loc = createStorageLocation(*Param);
-      setStorageLocation(*Param, Loc);
-
-      if (auto *ArgVal = getValue(*ArgLoc)) {
-        setValue(Loc, *ArgVal);
-      } else if (Value *Val = createValue(ParamType)) {
-        setValue(Loc, *Val);
-      }
-    }
+    setStorageLocation(*Param, createObject(*Param, Args[ArgIndex]));
   }
 }
 
 void Environment::popCall(const CallExpr *Call, const Environment &CalleeEnv) {
   // We ignore `DACtx` because it's already the same in both. We don't want the
   // callee's `DeclCtx`, `ReturnVal`, `ReturnLoc` or `ThisPointeeLoc`. We don't
   // bring back `DeclToLoc` and `ExprToLoc` because we want to be able to later
   // analyze the same callee in a different context, and `setStorageLocation`
if (Type->isRecordType()) {
     }
 
     return &arena().create<StructValue>(std::move(FieldValues));
   }
 
   return nullptr;
 }
 
+StorageLocation &Environment::createObjectInternal(const VarDecl *D,
+                                                   QualType Ty,
+                                                   const Expr *InitExpr) {
+  if (Ty->isReferenceType()) {
+    // Although variables of reference type always need to be initialized, it
+    // can happen that we can't see the initializer, so `InitExpr` may still
+    // be null.
+    if (InitExpr) {
+      if (auto *InitExprLoc =
+              getStorageLocation(*InitExpr, SkipPast::Reference))
+        return *InitExprLoc;
+    }
+
+    // Even though we have an initializer, we might not get an
+    // InitExprLoc, for example if the InitExpr is a CallExpr for which we
+    // don't have a function body. In this case, we just invent a storage
+    // location and value -- it's the best we can do.
+    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 = getValueStrict(*InitExpr);
+  if (!Val)
+    Val = createValue(Ty);
+
+  StorageLocation &Loc =
+      D ? createStorageLocation(*D) : createStorageLocation(Ty);
+
+  if (Val)
+    setValue(Loc, *Val);
+
+  return Loc;
+}
+
 StorageLocation &Environment::skip(StorageLocation &Loc, SkipPast SP) const {
   switch (SP) {
   case SkipPast::None:
     return Loc;
   case SkipPast::Reference:
     // References cannot be chained so we only need to skip past one level of
     // indirection.
     if (auto *Val = dyn_cast_or_null<ReferenceValue>(getValue(Loc)))

clang/lib/Analysis/FlowSensitive/Transfer.cpp

void VisitDeclStmt(const DeclStmt *S) {
     ProcessVarDecl(D);
   }
 
   void ProcessVarDecl(const VarDecl &D) {
     // Static local vars are already initialized in `Environment`.
     if (D.hasGlobalStorage())
       return;
 
-    if (D.getType()->isReferenceType()) {
-      // If this is the holding variable for a `BindingDecl`, we may already
-      // have a storage location set up -- so check. (See also explanation below
-      // where we process the `BindingDecl`.)
-      if (Env.getStorageLocation(D) == nullptr) {
-        const Expr *InitExpr = D.getInit();
-        assert(InitExpr != nullptr);
-        if (auto *InitExprLoc =
-                Env.getStorageLocation(*InitExpr, SkipPast::Reference)) {
-          Env.setStorageLocation(D, *InitExprLoc);
-        } else {
-          // Even though we have an initializer, we might not get an
-          // InitExprLoc, for example if the InitExpr is a CallExpr for which we
-          // don't have a function body. In this case, we just invent a storage
-          // location and value -- it's the best we can do.
-          StorageLocation &Loc =
-              Env.createStorageLocation(D.getType().getNonReferenceType());
-          Env.setStorageLocation(D, Loc);
-          if (Value *Val = Env.createValue(D.getType().getNonReferenceType()))
-            Env.setValue(Loc, *Val);
-        }
-      }
-    } else {
-      // Not a reference type.
-
-      assert(Env.getStorageLocation(D) == nullptr);
-      StorageLocation &Loc = Env.createStorageLocation(D);
-      Env.setStorageLocation(D, Loc);
-
-      const Expr *InitExpr = D.getInit();
-      if (InitExpr == nullptr) {
-        // No initializer expression - associate `Loc` with a new value.
-        if (Value *Val = Env.createValue(D.getType()))
-          Env.setValue(Loc, *Val);
-        return;
-      }
+    if (D.getType()->isReferenceType() && Env.getStorageLocation(D) != nullptr)
 
-      if (auto *InitExprVal = Env.getValueStrict(*InitExpr))
-        Env.setValue(Loc, *InitExprVal);
+    assert(Env.getStorageLocation(D) == nullptr);
 
-      if (Env.getValue(Loc) == nullptr) {
-        // We arrive here in (the few) cases where an expression is
-        // intentionally "uninterpreted". 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.
-        //
-        // 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).
-        if (Value *Val = Env.createValue(D.getType()))
-          Env.setValue(Loc, *Val);
-      }
-    }
+    Env.setStorageLocation(D, Env.createObject(D));
 
     // `DecompositionDecl` must be handled after we've interpreted the loc
     // itself, because the binding expression refers back to the
     // `DecompositionDecl` (even though it has no written name).
     if (const auto *Decomp = dyn_cast<DecompositionDecl>(&D)) {
       // If VarDecl is a DecompositionDecl, evaluate each of its bindings. This
       // needs to be evaluated after initializing the values in the storage for
       // VarDecl, as the bindings refer to them.