[clang][dataflow] Add Environment::initializeFieldsWithValues().

[martinboehme]

This function will be useful when we change the behavior of record-type prvalues
so that they directly initialize the associated result object. See also the
comment here for more details:

llvm-project/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

Line 354 in 9e73656

/// FIXME: Currently, this simply returns a stable storage location for `E`,

As part of this patch, we document and assert that synthetic fields may not have
reference type.

There is no practical use case for this: A StorageLocation may not have
reference type, and a synthetic field of the corresponding non-reference type
can serve the same purpose.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (martinboehme)

Changes

This function will be useful when we change the behavior of record-type prvalues
so that they directly initialize the associated result object. See also the
comment here for more details:

llvm-project/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

Line 354 in 9e73656

/// FIXME: Currently, this simply returns a stable storage location for `E`,

As part of this patch, we document and assert that synthetic fields may not have
reference type.

There is no practical use case for this: A StorageLocation may not have
reference type, and a synthetic field of the corresponding non-reference type
can serve the same purpose.

---

Full diff: https://github.com/llvm/llvm-project/pull/81239.diff

3 Files Affected:

• (modified) clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h (+13-2)
• (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+8)
• (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+47-27)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
index 20e45cc27b01f..98bdf037880ab 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
@@ -100,6 +100,8 @@ class DataflowAnalysisContext {
   /// to add to a `RecordStorageLocation` of a given type.
   /// Typically, this is called from the constructor of a `DataflowAnalysis`
   ///
+  /// The field types returned by the callback may not have reference type.
+  ///
   /// To maintain the invariant that all `RecordStorageLocation`s of a given
   /// type have the same fields:
   /// *  The callback must always return the same result for a given type
@@ -205,8 +207,17 @@ class DataflowAnalysisContext {
   /// type.
   llvm::StringMap<QualType> getSyntheticFields(QualType Type) {
     assert(Type->isRecordType());
-    if (SyntheticFieldCallback)
-      return SyntheticFieldCallback(Type);
+    if (SyntheticFieldCallback) {
+      llvm::StringMap<QualType> Result = SyntheticFieldCallback(Type);
+      // Synthetic fields are not allowed to have reference type.
+      assert([&Result] {
+        for (const auto &Entry : Result)
+          if (Entry.getValue()->isReferenceType())
+            return false;
+        return true;
+      }());
+      return Result;
+    }
     return {};
   }
 
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 5c737a561a7c1..0aecc749bf415 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -681,6 +681,14 @@ class Environment {
                                           llvm::DenseSet<QualType> &Visited,
                                           int Depth, int &CreatedValuesCount);
 
+  /// 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,
+                                  llvm::DenseSet<QualType> &Visited, int Depth,
+                                  int &CreatedValuesCount);
+
   /// Shared implementation of `createObject()` overloads.
   /// `D` and `InitExpr` may be null.
   StorageLocation &createObjectInternal(const ValueDecl *D, QualType Ty,
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 24811ded970e8..4a9e545fd3df4 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -887,34 +887,10 @@ Value *Environment::createValueUnlessSelfReferential(
 
   if (Type->isRecordType()) {
     CreatedValuesCount++;
-    llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
-    for (const FieldDecl *Field : DACtx->getModeledFields(Type)) {
-      assert(Field != nullptr);
+    auto &Loc = cast<RecordStorageLocation>(createStorageLocation(Type));
+    initializeFieldsWithValues(Loc, Visited, Depth, CreatedValuesCount);
 
-      QualType FieldType = Field->getType();
-
-      FieldLocs.insert(
-          {Field, &createLocAndMaybeValue(FieldType, Visited, Depth + 1,
-                                          CreatedValuesCount)});
-    }
-
-    RecordStorageLocation::SyntheticFieldMap SyntheticFieldLocs;
-    for (const auto &Entry : DACtx->getSyntheticFields(Type)) {
-      SyntheticFieldLocs.insert(
-          {Entry.getKey(),
-           &createLocAndMaybeValue(Entry.getValue(), Visited, Depth + 1,
-                                   CreatedValuesCount)});
-    }
-
-    RecordStorageLocation &Loc = DACtx->createRecordStorageLocation(
-        Type, std::move(FieldLocs), std::move(SyntheticFieldLocs));
-    RecordValue &RecordVal = create<RecordValue>(Loc);
-
-    // As we already have a storage location for the `RecordValue`, we can and
-    // should associate them in the environment.
-    setValue(Loc, RecordVal);
-
-    return &RecordVal;
+    return &refreshRecordValue(Loc, *this);
   }
 
   return nullptr;
@@ -943,6 +919,50 @@ Environment::createLocAndMaybeValue(QualType Ty,
   return Loc;
 }
 
+void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
+                                             llvm::DenseSet<QualType> &Visited,
+                                             int Depth,
+                                             int &CreatedValuesCount) {
+  auto initField = [&](QualType FieldType, StorageLocation &FieldLoc) {
+    if (FieldType->isRecordType()) {
+      auto &FieldRecordLoc = cast<RecordStorageLocation>(FieldLoc);
+      setValue(FieldRecordLoc, create<RecordValue>(FieldRecordLoc));
+      initializeFieldsWithValues(FieldRecordLoc, Visited, Depth + 1,
+                                  CreatedValuesCount);
+    } else {
+      if (!Visited.insert(FieldType.getCanonicalType()).second)
+        return;
+      if (Value *Val = createValueUnlessSelfReferential(
+              FieldType, Visited, Depth + 1, CreatedValuesCount))
+        setValue(FieldLoc, *Val);
+      Visited.erase(FieldType.getCanonicalType());
+    }
+  };
+
+  for (const auto [Field, FieldLoc] : Loc.children()) {
+    assert(Field != nullptr);
+    QualType FieldType = Field->getType();
+
+    if (FieldType->isReferenceType()) {
+      Loc.setChild(*Field,
+                   &createLocAndMaybeValue(FieldType, Visited, Depth + 1,
+                                           CreatedValuesCount));
+    } else {
+      assert(FieldLoc != nullptr);
+      initField(FieldType, *FieldLoc);
+    }
+  }
+  for (const auto &[FieldName, FieldLoc] : Loc.synthetic_fields()) {
+    assert(FieldLoc != nullptr);
+    QualType FieldType = FieldLoc->getType();
+
+    // Synthetic fields cannot have reference type, so we don't need to deal
+    // with this case.
+    assert(!FieldType->isReferenceType());
+    initField(FieldType, Loc.getSyntheticField(FieldName));
+  }
+}
+
 StorageLocation &Environment::createObjectInternal(const ValueDecl *D,
                                                    QualType Ty,
                                                    const Expr *InitExpr) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[martinboehme]

CI failure appears to be an infrastructure failure

[Xazax-hun]

Minor nit: Maybe we can save a lookup here. The insert above should return an iterator, so we should be able to remove the element using that iterator.

[martinboehme]

This doesn't, unfortunately, work, as createValueUnlessSelfReferential() may insert more entries into Visited, which would invalidate the iterator returned by insert().

[martinboehme]

Reverting: Causes build errors.

/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:942:19: error: loop variable '[Field, FieldLoc]' creates a copy from type 'const llvm::detail::DenseMapPair<const clang::ValueDecl *, clang::dataflow::StorageLocation *>' [-Werror,-Wrange-loop-construct]
  942 |   for (const auto [Field, FieldLoc] : Loc.children()) {