diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index c128ee4ea85c9..73f747ff88cf4 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 004b7711a869d..56246066e4aa1 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 9dc528567038a..1ea40927ac58a 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 &DeclToLoc1, + const llvm::DenseMap &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 llvm::DenseMap intersectDenseMaps(const llvm::DenseMap &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 01b397787430a..6b167891c1a3a 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(), 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().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 e8cbca7564603..2e8d38490fdb8 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(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 3fc1bb6692acf..2425bb8711bdb 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 {