[clang][dataflow] Remove declarations from DeclToLoc when their lifetime ends. (#67300)

After https://reviews.llvm.org/D153273, we're now able to use
`CFGLifetimeEnds`
together with the other CFG options we use.

Author: martinboehme

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.
   ///

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();

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,

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;
   }
 }

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());
       });
 }
 

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 {