[clang] [clang][dataflow] Remove declarations from `DeclToLoc` when their lifetime ends. (PR #67300)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 25 02:08:31 PDT 2023
https://github.com/martinboehme created https://github.com/llvm/llvm-project/pull/67300
After https://reviews.llvm.org/D153273, we're now able to use `CFGLifetimeEnds`
together with the other CFG options we use.
>From baa1bbf7bfe684fc81b37fa3e1795170f5ee6156 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Mon, 25 Sep 2023 09:07:34 +0000
Subject: [PATCH] [clang][dataflow] Remove declarations from `DeclToLoc` when
their lifetime ends.
After https://reviews.llvm.org/D153273, we're now able to use `CFGLifetimeEnds`
together with the other CFG options we use.
---
.../FlowSensitive/DataflowEnvironment.h | 7 ++++++
.../FlowSensitive/ControlFlowContext.cpp | 1 +
.../FlowSensitive/DataflowEnvironment.cpp | 24 +++++++++++++++----
.../TypeErasedDataflowAnalysis.cpp | 22 ++++++++---------
.../Analysis/FlowSensitive/TransferTest.cpp | 5 +---
.../TypeErasedDataflowAnalysisTest.cpp | 3 ++-
6 files changed, 41 insertions(+), 21 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index c128ee4ea85c928..73f747ff88cf447 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -260,6 +260,13 @@ 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.
+ void removeDecl(const ValueDecl &D);
+
/// Assigns `Loc` as the storage location of the glvalue `E` in the
/// environment.
///
diff --git a/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp b/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
index 004b7711a869d17..56246066e4aa13a 100644
--- a/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
+++ b/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp
@@ -97,6 +97,7 @@ ControlFlowContext::build(const Decl &D, Stmt &S, ASTContext &C) {
Options.AddTemporaryDtors = true;
Options.AddInitializers = true;
Options.AddCXXDefaultInitExprInCtors = true;
+ Options.AddLifetime = true;
// Ensure that all sub-expressions in basic blocks are evaluated.
Options.setAllAlwaysAdd();
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 9dc528567038ac4..1ea40927ac58adc 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -35,6 +35,20 @@ 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,
@@ -636,10 +650,7 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
else
JoinedEnv.ReturnLoc = nullptr;
- // FIXME: Once we're able to remove declarations from `DeclToLoc` when their
- // lifetime ends, add an assertion that there aren't any entries in
- // `DeclToLoc` and `Other.DeclToLoc` that map the same declaration to
- // different storage locations.
+ assert(declToLocConsistent(EnvA.DeclToLoc, EnvB.DeclToLoc));
JoinedEnv.DeclToLoc = intersectDenseMaps(EnvA.DeclToLoc, EnvB.DeclToLoc);
JoinedEnv.ExprToLoc = intersectDenseMaps(EnvA.ExprToLoc, EnvB.ExprToLoc);
@@ -691,6 +702,11 @@ StorageLocation *Environment::getStorageLocation(const ValueDecl &D) const {
return Loc;
}
+void Environment::removeDecl(const ValueDecl &D) {
+ assert(DeclToLoc.contains(&D));
+ DeclToLoc.erase(&D);
+}
+
void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
// `DeclRefExpr`s to builtin function types aren't glvalues, for some reason,
// but we still want to be able to associate a `StorageLocation` with them,
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 01b397787430ac6..6b167891c1a3ac8 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -439,19 +439,17 @@ static void builtinTransfer(const CFGElement &Elt,
case CFGElement::Initializer:
builtinTransferInitializer(Elt.castAs<CFGInitializer>(), State);
break;
+ case CFGElement::LifetimeEnds:
+ // Removing declarations when their lifetime ends serves two purposes:
+ // - Eliminate unnecessary clutter from `Environment::DeclToLoc`
+ // - Allow us to assert that, when joining two `Environment`s, the two
+ // `DeclToLoc` maps never contain entries that map the same declaration to
+ // different storage locations.
+ if (const ValueDecl *VD = Elt.castAs<CFGLifetimeEnds>().getVarDecl())
+ State.Env.removeDecl(*VD);
+ break;
default:
- // FIXME: Evaluate other kinds of `CFGElement`, including:
- // - When encountering `CFGLifetimeEnds`, remove the declaration from
- // `Environment::DeclToLoc`. This would serve two purposes:
- // a) Eliminate unnecessary clutter from `Environment::DeclToLoc`
- // b) Allow us to implement an assertion that, when joining two
- // `Environments`, the two `DeclToLoc` maps never contain entries that
- // map the same declaration to different storage locations.
- // Unfortunately, however, we can't currently process `CFGLifetimeEnds`
- // because the corresponding CFG option `AddLifetime` is incompatible with
- // the option 'AddImplicitDtors`, which we already use. We will first
- // need to modify the CFG implementation to make these two options
- // compatible before we can process `CFGLifetimeEnds`.
+ // FIXME: Evaluate other kinds of `CFGElement`
break;
}
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index e8cbca756460369..2e8d38490fdb864 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2957,10 +2957,7 @@ TEST(TransferTest, VarDeclInDoWhile) {
const auto *BarVal = cast<IntegerValue>(EnvInLoop.getValue(*BarDecl));
EXPECT_EQ(BarVal, FooPointeeVal);
- // FIXME: This assertion documents current behavior, but we would prefer
- // declarations to be removed from the environment when their lifetime
- // ends. Once this is the case, change this assertion accordingly.
- ASSERT_THAT(EnvAfterLoop.getValue(*BarDecl), BarVal);
+ ASSERT_THAT(EnvAfterLoop.getValue(*BarDecl), IsNull());
});
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 3fc1bb6692acf0b..2425bb8711bdbaf 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -108,7 +108,8 @@ TEST(DataflowAnalysisTest, DiagnoseFunctionDiagnoserCalledOnEachElement) {
// `diagnoseFunction` provides no guarantees about the order in which elements
// are visited, so we use `UnorderedElementsAre`.
EXPECT_THAT_EXPECTED(Result, llvm::HasValue(UnorderedElementsAre(
- "0\n", "int x = 0;\n", "x\n", "++x\n")));
+ "0\n", "int x = 0;\n", "x\n", "++x\n",
+ " (Lifetime ends)\n")));
}
struct NonConvergingLattice {
More information about the cfe-commits
mailing list