From d17f455a6348806c73641e742af08b0a974e13d5 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme@google.com>
Date: Mon, 17 Jul 2023 06:31:36 +0000
Subject: [PATCH] [clang][dataflow] Add `refreshStructValue()`.

Besides being a useful abstraction, this function will help insulate existing clients of the framework from upcoming changes to the API of `StructValue` and `AggregateStorageLocation`.

Depends On D155202

Reviewed By: ymandel, xazax.hun

Differential Revision: https://reviews.llvm.org/D155204
---
 .../Analysis/FlowSensitive/DataflowEnvironment.h   | 24 +++++++++++++++++++++
 .../Analysis/FlowSensitive/DataflowEnvironment.cpp | 25 ++++++++++++++++++++++
 .../TypeErasedDataflowAnalysisTest.cpp             | 14 ++----------
 3 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index dc82cce..9d99b67 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -389,6 +389,12 @@ public:
   /// necessary storage locations and values for indirections until it finds a
   /// non-pointer/non-reference type.
   ///
+  /// If `Type` is one of the following types, this function will always return
+  /// a non-null pointer:
+  /// - `bool`
+  /// - Any integer type
+  /// - Any class, struct, or union type
+  ///
   /// Requirements:
   ///
   ///  `Type` must not be null.
@@ -692,6 +698,24 @@ AggregateStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
 /// order in which they appear in `InitListExpr::inits()`.
 std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD);
 
+/// Associates a new `StructValue` with `Loc` and returns the new value.
+/// It is not defined whether the field values remain the same or not.
+///
+/// This function is primarily intended for use by checks that set custom
+/// properties on `StructValue`s to model the state of these values. Such checks
+/// should avoid modifying the properties of an existing `StructValue` because
+/// these changes would be visible to other `Environment`s that share the same
+/// `StructValue`. Instead, call `refreshStructValue()`, then set the properties
+/// on the new `StructValue` that it returns. Typical usage:
+///
+///   refreshStructValue(Loc, Env).setProperty("my_prop", MyPropValue);
+StructValue &refreshStructValue(AggregateStorageLocation &Loc,
+                                Environment &Env);
+
+/// Associates a new `StructValue` with `Expr` and returns the new value.
+/// See also documentation for the overload above.
+StructValue &refreshStructValue(const Expr &Expr, Environment &Env);
+
 } // namespace dataflow
 } // namespace clang
 
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index fdd6b59..5d301b8 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -987,5 +987,30 @@ std::vector<FieldDecl *> getFieldsForInitListExpr(const RecordDecl *RD) {
   return Fields;
 }
 
+StructValue &refreshStructValue(AggregateStorageLocation &Loc,
+                                Environment &Env) {
+  auto &NewVal = *cast<StructValue>(Env.createValue(Loc.getType()));
+  Env.setValue(Loc, NewVal);
+  return NewVal;
+}
+
+StructValue &refreshStructValue(const Expr &Expr, Environment &Env) {
+  assert(Expr.getType()->isRecordType());
+
+  auto &NewVal = *cast<StructValue>(Env.createValue(Expr.getType()));
+
+  if (Expr.isPRValue()) {
+    Env.setValueStrict(Expr, NewVal);
+  } else {
+    StorageLocation *Loc = Env.getStorageLocationStrict(Expr);
+    if (Loc == nullptr) {
+      Loc = &Env.createStorageLocation(Expr);
+    }
+    Env.setValue(*Loc, NewVal);
+  }
+
+  return NewVal;
+}
+
 } // namespace dataflow
 } // namespace clang
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 11b346f..462ffb5 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -370,13 +370,6 @@ TEST_F(NoreturnDestructorTest, ConditionalOperatorNestedBranchReturns) {
   // FIXME: Called functions at point `p` should contain only "foo".
 }
 
-StructValue &createNewStructValue(AggregateStorageLocation &Loc,
-                                  Environment &Env) {
-  auto &Val = *cast<StructValue>(Env.createValue(Loc.getType()));
-  Env.setValue(Loc, Val);
-  return Val;
-}
-
 // Models an analysis that uses flow conditions.
 class SpecialBoolAnalysis final
     : public DataflowAnalysis<SpecialBoolAnalysis, NoopLattice> {
@@ -407,7 +400,7 @@ public:
       auto &ObjectLoc =
           *cast<AggregateStorageLocation>(getImplicitObjectLocation(*E, Env));
 
-      createNewStructValue(ObjectLoc, Env)
+      refreshStructValue(ObjectLoc, Env)
           .setProperty("is_set", Env.getBoolLiteralValue(true));
     }
   }
@@ -562,10 +555,7 @@ public:
       auto *Object = E->getArg(0);
       assert(Object != nullptr);
 
-      auto &ObjectLoc = *cast<AggregateStorageLocation>(
-          Env.getStorageLocation(*Object, SkipPast::Reference));
-
-      createNewStructValue(ObjectLoc, Env)
+      refreshStructValue(*Object, Env)
           .setProperty("has_value", Env.getBoolLiteralValue(true));
     }
   }
-- 
2.7.4