https://github.com/martinboehme updated https://github.com/llvm/llvm-project/pull/69819
>From ad60ea756566853a16b5d4e5ae9ebe61bc7fefbf Mon Sep 17 00:00:00 2001 From: Martin Braenne <mboe...@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 | 16 ------- .../Analysis/FlowSensitive/TransferTest.cpp | 44 +++++++++++++++++++ 3 files changed, 45 insertions(+), 21 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..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 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
