[clang] Thread safety analysis: Fix a bug in handling temporary constructors (PR #74020)

[Ziqing Luo via cfe-commits]

https://github.com/ziqingluo-90 updated https://github.com/llvm/llvm-project/pull/74020

>From 9c2718abce31d61c079e0945313fe14ad0f334b3 Mon Sep 17 00:00:00 2001
From: ziqingluo-90 <ziqing at udel.edu>
Date: Mon, 4 Dec 2023 14:37:10 -0800
Subject: [PATCH] Thread safety analysis: Fix a bug in handling temporary
 constructors

Extends the lifetime of the map `ConstructedObjects` to be of the
whole CFG so that the map can connect temporary Ctor and Dtor in
different CFG blocks.
---
 clang/lib/Analysis/ThreadSafety.cpp           | 26 +++++++++----------
 .../SemaCXX/warn-thread-safety-analysis.cpp   |  8 ++++++
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 7fdf22c2f3919..e25b843c9bf83 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1010,6 +1010,8 @@ class ThreadSafetyAnalyzer {
   ThreadSafetyHandler &Handler;
   const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
+  // Maps constructed objects to `this` placeholder prior to initialization.
+  llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
   FactManager FactMan;
   std::vector<CFGBlockInfo> BlockInfo;
 
@@ -1543,8 +1545,6 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
   FactSet FSet;
   // The fact set for the function on exit.
   const FactSet &FunctionExitFSet;
-  /// Maps constructed objects to `this` placeholder prior to initialization.
-  llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
@@ -1808,7 +1808,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
       std::pair<til::LiteralPtr *, StringRef> Placeholder =
           Analyzer->SxBuilder.createThisPlaceholder(Exp);
       [[maybe_unused]] auto inserted =
-          ConstructedObjects.insert({Exp, Placeholder.first});
+          Analyzer->ConstructedObjects.insert({Exp, Placeholder.first});
       assert(inserted.second && "Are we visiting the same expression again?");
       if (isa<CXXConstructExpr>(Exp))
         Self = Placeholder.first;
@@ -2128,10 +2128,10 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
         E = EWC->getSubExpr()->IgnoreParens();
       E = UnpackConstruction(E);
 
-      if (auto Object = ConstructedObjects.find(E);
-          Object != ConstructedObjects.end()) {
+      if (auto Object = Analyzer->ConstructedObjects.find(E);
+          Object != Analyzer->ConstructedObjects.end()) {
         Object->second->setClangDecl(VD);
-        ConstructedObjects.erase(Object);
+        Analyzer->ConstructedObjects.erase(Object);
       }
     }
   }
@@ -2140,11 +2140,11 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
 void BuildLockset::VisitMaterializeTemporaryExpr(
     const MaterializeTemporaryExpr *Exp) {
   if (const ValueDecl *ExtD = Exp->getExtendingDecl()) {
-    if (auto Object =
-            ConstructedObjects.find(UnpackConstruction(Exp->getSubExpr()));
-        Object != ConstructedObjects.end()) {
+    if (auto Object = Analyzer->ConstructedObjects.find(
+            UnpackConstruction(Exp->getSubExpr()));
+        Object != Analyzer->ConstructedObjects.end()) {
       Object->second->setClangDecl(ExtD);
-      ConstructedObjects.erase(Object);
+      Analyzer->ConstructedObjects.erase(Object);
     }
   }
 }
@@ -2487,15 +2487,15 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
 
           // Clean up constructed object even if there are no attributes to
           // keep the number of objects in limbo as small as possible.
-          if (auto Object = LocksetBuilder.ConstructedObjects.find(
+          if (auto Object = ConstructedObjects.find(
                   TD.getBindTemporaryExpr()->getSubExpr());
-              Object != LocksetBuilder.ConstructedObjects.end()) {
+              Object != ConstructedObjects.end()) {
             const auto *DD = TD.getDestructorDecl(AC.getASTContext());
             if (DD->hasAttrs())
               // TODO: the location here isn't quite correct.
               LocksetBuilder.handleCall(nullptr, DD, Object->second,
                                         TD.getBindTemporaryExpr()->getEndLoc());
-            LocksetBuilder.ConstructedObjects.erase(Object);
+            ConstructedObjects.erase(Object);
           }
           break;
         }
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 205cfa284f6c9..dfb966d3b5902 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1702,6 +1702,8 @@ struct TestScopedLockable {
 
   bool getBool();
 
+  bool lock2Bool(MutexLock);
+
   void foo1() {
     MutexLock mulock(&mu1);
     a = 5;
@@ -1718,6 +1720,12 @@ struct TestScopedLockable {
     MutexLock{&mu1}, a = 5;
   }
 
+  void temporary_cfg(int x) {
+    // test the case where a pair of temporary Ctor and Dtor is in different CFG blocks
+    lock2Bool(MutexLock{&mu1}) || x;
+    MutexLock{&mu1};  // no-warn
+  }
+
   void lifetime_extension() {
     const MutexLock &mulock = MutexLock(&mu1);
     a = 5;