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

[Ziqing Luo via cfe-commits]

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

The commit 54bfd04846156dbd5e0a6b88f539c3d4569a455f introduces general handling of constructor and destructor calls.  Consider a pair of TEMPORARY constructor and destructor calls of a "SCOPED_CAPABILITY" object.  At the constructor call, a resource representing the object itself is added to the state.  But since it is a temporary object, the analyzer may not be able to find an AST part to identify the resource (As opposed to the normal case of using `VarDecl` as an identifier). Later at the destructor call, the resource needs to be removed from the state.  Since the resource cannot be identified by anything other than itself, the analyzer maintains a map from constructor calls to their created resources so that the analyzer could get the proper resource for removal at the destructor call via look up the map.  The issue is that the map lives within a CFG block.  That means in case the pair of temporary constructor and destructor calls are not in one CFG block, the analyzer is not able to remove the proper resource at the destructor call.  Although the two calls stay in one CFG block usually, they separate in the following example:

```
if (f(MutexLock{lock}) || x) { // <-- the temporary Ctor and Dtor of MutexLock are in different CFG blocks

}
```

The fix is simple. It extends the life of the map to that of the entire CFG.

>From ecb13c381d99010dc04a2da4e945dfd1132777fd Mon Sep 17 00:00:00 2001
From: ziqingluo-90 <ziqing at udel.edu>
Date: Thu, 30 Nov 2023 14:15:03 -0800
Subject: [PATCH] Thread safety analysis: Fix a bug in handling temporary
 constructors & destructors

The commit 54bfd04846156dbd5e0a6b88f539c3d4569a455f introduces general
handling of constructor and destructor calls.  Consider a pair of
TEMPORARY constructor and destructor calls of a "SCOPED_CAPABILITY"
object.  At the constructor call, a resource representing the object
itself is added to the state.  But since it is a temporary object, the
analyzer may not be able to find an AST part to identify the resource
(As opposed to the normal case of using `VarDecl` as an identifier).
Later at the destructor call, the resource needs to be removed from
the state.  Since the resource cannot be identified by anything other
than itself, the analyzer maintains a map from constructor calls to
their created resources so that the analyzer could get the proper
resource for removal at the destructor call via look up the map.  The
issue is that the map lives within a CFG block.  That means in case
the pair of temporary constructor and destructor calls are not in one
CFG block, the analyzer is not able to remove the proper resource at
the destructor call.  Although the two calls stay in one CFG block
usually, they separate in the following example:

```
if (f(MutexLock{lock}) || x) { // <-- the temporary Ctor and Dtor of MutexLock are in different CFG blocks

}
```

The fix is simple. It extends the life of the map to that of the
entire CFG.
---
 clang/lib/Analysis/ThreadSafety.cpp            | 18 +++++++++++++-----
 .../SemaCXX/warn-thread-safety-analysis.cpp    |  9 +++++++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 7fdf22c2f3919cb..9549d1b398dfa08 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1530,7 +1530,7 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
 }
 
 namespace {
-
+using ConstructedObjectMapTy = llvm::SmallDenseMap<const Expr *, til::LiteralPtr *>;
 /// We use this class to visit different types of expressions in
 /// CFGBlocks, and build up the lockset.
 /// An expression may cause us to add or remove locks from the lockset, or else
@@ -1544,7 +1544,10 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
   // 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;
+  /// The map lives longer than this builder so this is a reference to an
+  /// existing one. The map lives so long as the CFG while this builder is for
+  /// one CFG block.
+  ConstructedObjectMapTy &ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
@@ -1569,9 +1572,11 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
 
 public:
   BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info,
-               const FactSet &FunctionExitFSet)
+               const FactSet &FunctionExitFSet,
+               ConstructedObjectMapTy &ConstructedObjects)
       : ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
-        FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
+        FunctionExitFSet(FunctionExitFSet),
+        ConstructedObjects(ConstructedObjects), LVarCtx(Info.EntryContext),
         CtxIndex(Info.EntryIndex) {}
 
   void VisitUnaryOperator(const UnaryOperator *UO);
@@ -2392,6 +2397,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   for (const auto &Lock : LocksReleased)
     ExpectedFunctionExitSet.removeLock(FactMan, Lock);
 
+  ConstructedObjectMapTy ConstructedObjects;
+
   for (const auto *CurrBlock : *SortedGraph) {
     unsigned CurrBlockID = CurrBlock->getBlockID();
     CFGBlockInfo *CurrBlockInfo = &BlockInfo[CurrBlockID];
@@ -2451,7 +2458,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
     if (!CurrBlockInfo->Reachable)
       continue;
 
-    BuildLockset LocksetBuilder(this, *CurrBlockInfo, ExpectedFunctionExitSet);
+    BuildLockset LocksetBuilder(this, *CurrBlockInfo, ExpectedFunctionExitSet,
+                                ConstructedObjects);
 
     // Visit all the statements in the basic block.
     for (const auto &BI : *CurrBlock) {
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index 205cfa284f6c9c9..691b8733d58fd03 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 check(MutexLock);
+
   void foo1() {
     MutexLock mulock(&mu1);
     a = 5;
@@ -1718,6 +1720,13 @@ struct TestScopedLockable {
     MutexLock{&mu1}, a = 5;
   }
 
+  void temporary2(int x) {
+    if (check(MutexLock{&mu1}) || x) {
+
+    }
+    check(MutexLock{&mu1});
+  }
+
   void lifetime_extension() {
     const MutexLock &mulock = MutexLock(&mu1);
     a = 5;