Thread safety analysis: Fix a bug in handling temporary constructors #74020

ziqingluo-90 · 2023-12-01T01:33:35Z

The commit 54bfd04 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.

llvmbot · 2023-12-01T01:34:07Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Ziqing Luo (ziqingluo-90)

Changes

The commit 54bfd04 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) { // &lt;-- 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.

Full diff: https://github.com/llvm/llvm-project/pull/74020.diff

2 Files Affected:

(modified) clang/lib/Analysis/ThreadSafety.cpp (+13-5)
(modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+9)

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;

github-actions · 2023-12-01T01:35:58Z

✅ With the latest revision this PR passed the C/C++ code formatter.

aaronpuchert

I have some suggestions, but in principle this is absolutely right. Thanks for finding this and providing a fix!

The issue is that the map lives within a CFG block.

It didn't cross my mind to check how long BuildLockset lived, I always assumed it lived for the entire function. But you're right, it does not, and is therefore an unsuitable home for the map.

aaronpuchert · 2023-12-03T23:28:08Z

clang/lib/Analysis/ThreadSafety.cpp

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


It's probably sufficient to explain this in the commit message.

aaronpuchert · 2023-12-03T23:29:50Z

clang/lib/Analysis/ThreadSafety.cpp

@@ -2392,6 +2397,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
  for (const auto &Lock : LocksReleased)
    ExpectedFunctionExitSet.removeLock(FactMan, Lock);

+  ConstructedObjectMapTy ConstructedObjects;


I wonder if we should put it into the ThreadSafetyAnalyzer, where we already have the similar LocalVariableMap. Then we don't need to pass it into BuildLockset, because that already has a pointer to the ThreadSafetyAnalyzer.

aaronpuchert · 2023-12-03T23:30:53Z

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

+    if (check(MutexLock{&mu1}) || x) {
+
+    }


The if is probably not needed here, right? We could just write

Suggested change

if (check(MutexLock{&mu1}) || x) {

}

check(MutexLock{&mu1}) || x;

and should have the same CFG artifact.

aaronpuchert · 2023-12-03T23:31:40Z

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

+    if (check(MutexLock{&mu1}) || x) {
+
+    }
+    check(MutexLock{&mu1});


The check here doesn't do anything, right?

aaronpuchert · 2023-12-03T23:32:37Z

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

@@ -1718,6 +1720,13 @@ struct TestScopedLockable {
    MutexLock{&mu1}, a = 5;
  }

+  void temporary2(int x) {


I would suggest a speaking name for the test, like temporary_cfg.

ziqingluo-90 · 2023-12-04T23:02:20Z

@aaronpuchert Thanks for your comment! I have addressed them. Do you mind to take an another look and approve it if it looks good to you?

aaronpuchert · 2023-12-04T23:59:44Z

clang/lib/Analysis/ThreadSafety.cpp

-          if (auto Object = LocksetBuilder.ConstructedObjects.find(
+          if (auto Object = LocksetBuilder.Analyzer->ConstructedObjects.find(
                  TD.getBindTemporaryExpr()->getSubExpr());
-              Object != LocksetBuilder.ConstructedObjects.end()) {
+              Object != LocksetBuilder.Analyzer->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);
+            LocksetBuilder.Analyzer->ConstructedObjects.erase(Object);


We're in ThreadSafetyAnalyzer here, so you should be able to drop LocksetBuilder.Analyzer-> and refer to the member directly.

aaronpuchert · 2023-12-05T00:00:16Z

clang/lib/Analysis/ThreadSafety.cpp

@@ -1530,7 +1532,6 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
 }

 namespace {
-


Nitpick: can you undo the whitespace change?

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.

aaronpuchert

Thanks, looks good to me!

ziqingluo-90 · 2023-12-07T20:55:05Z

Plan to merge this PR tomorrow.

…lvm#74020) 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. (cherry picked from commit a341e17)

llvmbot added clang Clang issues not falling into any other category clang:analysis labels Dec 1, 2023

ziqingluo-90 requested review from aaronpuchert, AaronBallman, zygoloid and haoNoQ December 1, 2023 01:34

aaronpuchert reviewed Dec 3, 2023

View reviewed changes

ziqingluo-90 force-pushed the radar-111818130 branch from ecb13c3 to 00083bb Compare December 4, 2023 22:46

tstellar requested a review from tbaederr December 4, 2023 23:14

aaronpuchert reviewed Dec 5, 2023

View reviewed changes

ziqingluo-90 force-pushed the radar-111818130 branch from 00083bb to 81701bf Compare December 5, 2023 05:55

Thread safety analysis: Fix a bug in handling temporary constructors

9c2718a

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.

ziqingluo-90 force-pushed the radar-111818130 branch from 81701bf to 9c2718a Compare December 5, 2023 06:03

aaronpuchert approved these changes Dec 5, 2023

View reviewed changes

ziqingluo-90 merged commit a341e17 into llvm:main Dec 8, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread safety analysis: Fix a bug in handling temporary constructors #74020

Thread safety analysis: Fix a bug in handling temporary constructors #74020

ziqingluo-90 commented Dec 1, 2023

llvmbot commented Dec 1, 2023 •

edited

github-actions bot commented Dec 1, 2023 •

edited

aaronpuchert left a comment

aaronpuchert Dec 3, 2023

aaronpuchert Dec 3, 2023

aaronpuchert Dec 3, 2023

aaronpuchert Dec 3, 2023

aaronpuchert Dec 3, 2023

ziqingluo-90 commented Dec 4, 2023

aaronpuchert Dec 4, 2023

ziqingluo-90 Dec 5, 2023

aaronpuchert Dec 5, 2023

aaronpuchert left a comment

ziqingluo-90 commented Dec 7, 2023

		@@ -1530,7 +1532,6 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
		}

		namespace {

Thread safety analysis: Fix a bug in handling temporary constructors #74020

Thread safety analysis: Fix a bug in handling temporary constructors #74020

Conversation

ziqingluo-90 commented Dec 1, 2023

llvmbot commented Dec 1, 2023 • edited

github-actions bot commented Dec 1, 2023 • edited

aaronpuchert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziqingluo-90 commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronpuchert left a comment

Choose a reason for hiding this comment

ziqingluo-90 commented Dec 7, 2023

llvmbot commented Dec 1, 2023 •

edited

github-actions bot commented Dec 1, 2023 •

edited