[clang][dataflow] Remove declToLocConsistent() assertion.

[martinboehme]

As described here, 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.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: None (martinboehme)

Changes

As described here, 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.

---

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

3 Files Affected:

• (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+1-5)
• (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (-16)
• (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+44)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 56d647f35b08430..402f77d6a4ac3f8 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..c08cb2d7deb2d81 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);
@@ -715,7 +700,6 @@ StorageLocation *Environment::getStorageLocation(const ValueDecl &D) const {
 }
 
 void Environment::removeDecl(const ValueDecl &D) {
-  assert(DeclToLoc.contains(&D));
   DeclToLoc.erase(&D);
 }
 
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index ea36a3f705ee934..33d2fcc8aa69a3c 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

[github-actions]

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

[Xazax-hun]

LG, thanks!

[martinboehme]

Failing format check in clang/docs/ReleaseNotes.rst looks unrelated.