[clang] [clang][dataflow] Remove `declToLocConsistent()` assertion. (PR #69819)

[via cfe-commits]

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

>From 4d25701821240818f2459a036703b4be3ec93435 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Sat, 21 Oct 2023 08:38:35 +0000
Subject: [PATCH] [clang][dataflow] Remove `declToLocConsistent()` assertion.

As described [here](https://discourse.llvm.org/t/70086/6), there are legitimate
non-bug scenarios where two `DeclToLoc` maps to be joined contain different
storage locations for the same declaration. This patch also adds a test
containing an example of such a situation. (The test fails without the other
changes in this patch.)

With the assertion removed, the existing logic in `intersectDenseMaps()` will
remove the corresponding declaration from the joined DeclToLoc map.

We also remove `removeDecl()`'s precondition (that the declaration must be
associated with a storage location) because this may no longer hold if the
declaration was previously removed during a join, as described above.
---
 .../FlowSensitive/DataflowEnvironment.h       |  6 +--
 .../FlowSensitive/DataflowEnvironment.cpp     | 20 +--------
 .../Analysis/FlowSensitive/TransferTest.cpp   | 44 +++++++++++++++++++
 3 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 9ac2cb90ccc4d4a..33e9f0fba02bc77 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -260,11 +260,7 @@ class Environment {
   /// if `D` isn't assigned a storage location in the environment.
   StorageLocation *getStorageLocation(const ValueDecl &D) const;
 
-  /// Removes the location assigned to `D` in the environment.
-  ///
-  /// Requirements:
-  ///
-  ///  `D` must have a storage location assigned in the environment.
+  /// Removes the location assigned to `D` in the environment (if any).
   void removeDecl(const ValueDecl &D);
 
   /// Assigns `Loc` as the storage location of the glvalue `E` in the
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 01c6cc69e2b9fac..00e56ce51c530b3 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -35,20 +35,6 @@ namespace dataflow {
 static constexpr int MaxCompositeValueDepth = 3;
 static constexpr int MaxCompositeValueSize = 1000;
 
-/// Returns whether all declarations that `DeclToLoc1` and `DeclToLoc2` have in
-/// common map to the same storage location in both maps.
-bool declToLocConsistent(
-    const llvm::DenseMap<const ValueDecl *, StorageLocation *> &DeclToLoc1,
-    const llvm::DenseMap<const ValueDecl *, StorageLocation *> &DeclToLoc2) {
-  for (auto &Entry : DeclToLoc1) {
-    auto It = DeclToLoc2.find(Entry.first);
-    if (It != DeclToLoc2.end() && Entry.second != It->second)
-      return false;
-  }
-
-  return true;
-}
-
 /// Returns a map consisting of key-value entries that are present in both maps.
 template <typename K, typename V>
 llvm::DenseMap<K, V> intersectDenseMaps(const llvm::DenseMap<K, V> &Map1,
@@ -662,7 +648,6 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
   else
     JoinedEnv.ReturnLoc = nullptr;
 
-  assert(declToLocConsistent(EnvA.DeclToLoc, EnvB.DeclToLoc));
   JoinedEnv.DeclToLoc = intersectDenseMaps(EnvA.DeclToLoc, EnvB.DeclToLoc);
 
   JoinedEnv.ExprToLoc = intersectDenseMaps(EnvA.ExprToLoc, EnvB.ExprToLoc);
@@ -714,10 +699,7 @@ StorageLocation *Environment::getStorageLocation(const ValueDecl &D) const {
   return Loc;
 }
 
-void Environment::removeDecl(const ValueDecl &D) {
-  assert(DeclToLoc.contains(&D));
-  DeclToLoc.erase(&D);
-}
+void Environment::removeDecl(const ValueDecl &D) { DeclToLoc.erase(&D); }
 
 void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
   // `DeclRefExpr`s to builtin function types aren't glvalues, for some reason,
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index f5d9c785b63976f..0c2106777560ee6 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -6236,4 +6236,48 @@ TEST(TransferTest, LambdaCaptureThis) {
       });
 }
 
+TEST(TransferTest, DifferentReferenceLocInJoin) {
+  // This test triggers a case where the storage location for a reference-type
+  // variable is different for two states being joined. We used to believe this
+  // could not happen and therefore had an assertion disallowing this; this test
+  // exists to demonstrate that we can handle this condition without a failing
+  // assertion. See also the discussion here:
+  // https://discourse.llvm.org/t/70086/6
+  std::string Code = R"(
+    namespace std {
+      template <class T> struct initializer_list {
+        const T* begin();
+        const T* end();
+      };
+    }
+
+    void target(char* p, char* end) {
+      while (p != end) {
+        if (*p == ' ') {
+          p++;
+          continue;
+        }
+
+        auto && range = {1, 2};
+        for (auto b = range.begin(), e = range.end(); b != e; ++b) {
+        }
+        (void)0;
+        // [[p]]
+      }
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        // Joining environments with different storage locations for the same
+        // declaration results in the declaration being removed from the joined
+        // environment.
+        const ValueDecl *VD = findValueDecl(ASTCtx, "range");
+        ASSERT_EQ(Env.getStorageLocation(*VD), nullptr);
+      });
+}
+
 } // namespace