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

[via cfe-commits]

================
@@ -385,6 +390,187 @@ 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()) {
----------------
martinboehme wrote:

> Is this the branch that would be triggered for the initialization of the array member in code like:
[snip]

No. (Neither of the two `InitListExpr`s here is transparent.  More details below.)

> I am mostly interested in what do we expect to happen for the array elements.

AFAIK we don't handle arrays at all yet, neither here nor in the rest of the framework. (For this particular function, see the bit at the top that bails out if this isn't a record type.) So the answer to this particular case is: Nothing.

But in any case, the `InitListExpr`s in the code you posted aren't transparent. (Unfortunately, the AST dumper doesn't output whether an `InitListExpr` is transparent; I locally modified `TextNodeDumper::VisitInitListExpr()` to verify my understanding of when an `InitListExpr` is transparent.)

Here's the documentation of `InitListExpr::isTransparent()`:

```cxx
  /// Is this a transparent initializer list (that is, an InitListExpr that is
  /// purely syntactic, and whose semantics are that of the sole contained
  /// initializer)?
```

This isn't, unfortunately, entirely clear, but I think what this is saying is that an `InitListExpr` is transparent if it contains exactly one initializer and has the same semantics as simply replacing the `InitListExpr` with that initializer, i.e. leaving off the curly braces.

For example, here's a transparent `InitListExpr`:

```cxx
struct S {};

void f() {
  S s = {S()};
}
```

Thinking about this, I realize that the code here is too general -- a transparent initializer list always has exactly one initializer, so there's no need to loop over `inits()`. (See [here](https://github.com/llvm/llvm-project/blob/299b636a8f1c9cb2382f9dce4cdf6ec6330a79c6/clang/lib/AST/ExprConstant.cpp#L10303) for an example in Clang that simply accesses `getInit(0)`.) I'll push a fixup that changes this.

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