[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)

[via cfe-commits]

https://github.com/martinboehme updated https://github.com/llvm/llvm-project/pull/65319:

>From fc70e2f878425661df8bb26ae7b7ec1648387fbd Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Mon, 11 Sep 2023 07:21:56 +0000
Subject: [PATCH] [clang][dataflow] Merge `RecordValue`s with different
 locations correctly.

Now that prvalue expressions map directly to values (see
https://reviews.llvm.org/D158977), it's no longer guaranteed that `RecordValue`s
associated with the same expression will always have the same storage location.

In other words, D158977 invalidated the assertion in `mergeDistinctValues()`.
The newly added test causes this assertion to fail without the other changes in
the patch.

This patch fixes the issue. However, the real fix will be to eliminate the
`StorageLocation` from `RecordValue` entirely.
---
 .../FlowSensitive/DataflowEnvironment.cpp     | 25 +++----
 .../FlowSensitive/DataflowEnvironmentTest.cpp | 69 +++++++++++++++++++
 2 files changed, 82 insertions(+), 12 deletions(-)

diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index f5eb469e7bb3d4e..e13f880896fc071 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -125,18 +125,19 @@ static Value *mergeDistinctValues(QualType Type, Value &Val1,
 
   Value *MergedVal = nullptr;
   if (auto *RecordVal1 = dyn_cast<RecordValue>(&Val1)) {
-    [[maybe_unused]] auto *RecordVal2 = cast<RecordValue>(&Val2);
-
-    // Values to be merged are always associated with the same location in
-    // `LocToVal`. The location stored in `RecordVal` should therefore also
-    // be the same.
-    assert(&RecordVal1->getLoc() == &RecordVal2->getLoc());
-
-    // `RecordVal1` and `RecordVal2` may have different properties associated
-    // with them. Create a new `RecordValue` without any properties so that we
-    // soundly approximate both values. If a particular analysis needs to merge
-    // properties, it should do so in `DataflowAnalysis::merge()`.
-    MergedVal = &MergedEnv.create<RecordValue>(RecordVal1->getLoc());
+    auto *RecordVal2 = cast<RecordValue>(&Val2);
+
+    if (&RecordVal1->getLoc() == &RecordVal2->getLoc())
+      // `RecordVal1` and `RecordVal2` may have different properties associated
+      // with them. Create a new `RecordValue` with the same location but
+      // without any properties so that we soundly approximate both values. If a
+      // particular analysis needs to merge properties, it should do so in
+      // `DataflowAnalysis::merge()`.
+      MergedVal = &MergedEnv.create<RecordValue>(RecordVal1->getLoc());
+    else
+      // If the locations for the two records are different, need to create a
+      // completely new value.
+      MergedVal = MergedEnv.createValue(Type);
   } else {
     MergedVal = MergedEnv.createValue(Type);
   }
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index a318d9ab7391aa1..6e37011a052c5e4 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -96,6 +96,75 @@ TEST_F(EnvironmentTest, CreateValueRecursiveType) {
   EXPECT_THAT(PV, NotNull());
 }
 
+TEST_F(EnvironmentTest, JoinRecords) {
+  using namespace ast_matchers;
+
+  std::string Code = R"cc(
+    struct S {};
+    // Need to use the type somewhere so that the `QualType` gets created;
+    S s;
+  )cc";
+
+  auto Unit =
+      tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"});
+  auto &Context = Unit->getASTContext();
+
+  ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+  auto Results =
+      match(qualType(hasDeclaration(recordDecl(hasName("S")))).bind("SType"),
+            Context);
+  const QualType *TyPtr = selectFirst<QualType>("SType", Results);
+  ASSERT_THAT(TyPtr, NotNull());
+  QualType Ty = *TyPtr;
+  ASSERT_FALSE(Ty.isNull());
+
+  auto *ConstructExpr = CXXConstructExpr::CreateEmpty(Context, 0);
+  ConstructExpr->setType(Ty);
+  ConstructExpr->setValueKind(VK_PRValue);
+
+  // Two different `RecordValue`s with the same location are joined into a
+  // third `RecordValue` with that same location.
+  {
+    Environment Env1(DAContext);
+    auto &Val1 = *cast<RecordValue>(Env1.createValue(Ty));
+    RecordStorageLocation &Loc = Val1.getLoc();
+    Env1.setValue(*ConstructExpr, Val1);
+
+    Environment Env2(DAContext);
+    auto &Val2 = Env2.create<RecordValue>(Loc);
+    Env2.setValue(Loc, Val2);
+    Env2.setValue(*ConstructExpr, Val2);
+
+    Environment::ValueModel Model;
+    Environment EnvJoined = Environment::join(Env1, Env2, Model);
+    auto *JoinedVal = cast<RecordValue>(EnvJoined.getValue(*ConstructExpr));
+    EXPECT_NE(JoinedVal, &Val1);
+    EXPECT_NE(JoinedVal, &Val2);
+    EXPECT_EQ(&JoinedVal->getLoc(), &Loc);
+  }
+
+  // Two different `RecordValue`s with different locations are joined into a
+  // third `RecordValue` with a location different from the other two.
+  {
+    Environment Env1(DAContext);
+    auto &Val1 = *cast<RecordValue>(Env1.createValue(Ty));
+    Env1.setValue(*ConstructExpr, Val1);
+
+    Environment Env2(DAContext);
+    auto &Val2 = *cast<RecordValue>(Env2.createValue(Ty));
+    Env2.setValue(*ConstructExpr, Val2);
+
+    Environment::ValueModel Model;
+    Environment EnvJoined = Environment::join(Env1, Env2, Model);
+    auto *JoinedVal = cast<RecordValue>(EnvJoined.getValue(*ConstructExpr));
+    EXPECT_NE(JoinedVal, &Val1);
+    EXPECT_NE(JoinedVal, &Val2);
+    EXPECT_NE(&JoinedVal->getLoc(), &Val1.getLoc());
+    EXPECT_NE(&JoinedVal->getLoc(), &Val2.getLoc());
+  }
+}
+
 TEST_F(EnvironmentTest, InitGlobalVarsFun) {
   using namespace ast_matchers;