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

2023-12-08 Thread Ziqing Luo via cfe-commits 

https://github.com/ziqingluo-90 closed 
https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-07 Thread Ziqing Luo via cfe-commits 

ziqingluo-90 wrote:

Plan to merge this PR tomorrow.

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-05 Thread Aaron Puchert via cfe-commits 

https://github.com/aaronpuchert approved this pull request.

Thanks, looks good to me!

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-04 Thread Ziqing Luo via cfe-commits 


@@ -2487,15 +2486,15 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
 
   // 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 = 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);

ziqingluo-90 wrote:

Done

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-04 Thread 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 
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 
   const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
+  // Maps constructed objects to `this` placeholder prior to initialization.
+  llvm::SmallDenseMap ConstructedObjects;
   FactManager FactMan;
   std::vector BlockInfo;
 
@@ -1543,8 +1545,6 @@ class BuildLockset : public 
ConstStmtVisitor {
   FactSet FSet;
   // The fact set for the function on exit.
   const FactSet 
-  /// Maps constructed objects to `this` placeholder prior to initialization.
-  llvm::SmallDenseMap ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
@@ -1808,7 +1808,7 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
   std::pair 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(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 ) {
 
   // 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();
 a = 5;
@@ -1718,6 +1720,12 @@ struct TestScopedLockable {
 MutexLock{}, 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{}) || x;
+MutexLock{};  // no-warn
+  }
+
   void

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

2023-12-04 Thread Ziqing Luo via cfe-commits 

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

>From 81701bfc9052eed567abb76ac05d44cb99298303 Mon Sep 17 00:00:00 2001
From: ziqingluo-90 
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   | 27 +--
 .../SemaCXX/warn-thread-safety-analysis.cpp   |  8 ++
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 7fdf22c2f3919..f3c07faa90eb6 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1010,6 +1010,8 @@ class ThreadSafetyAnalyzer {
   ThreadSafetyHandler 
   const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
+  // Maps constructed objects to `this` placeholder prior to initialization.
+  llvm::SmallDenseMap ConstructedObjects;
   FactManager FactMan;
   std::vector BlockInfo;
 
@@ -1530,7 +1532,6 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
 }
 
 namespace {
-
 /// 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
@@ -1543,8 +1544,6 @@ class BuildLockset : public 
ConstStmtVisitor {
   FactSet FSet;
   // The fact set for the function on exit.
   const FactSet 
-  /// Maps constructed objects to `this` placeholder prior to initialization.
-  llvm::SmallDenseMap ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
@@ -1808,7 +1807,7 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
   std::pair 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(Exp))
 Self = Placeholder.first;
@@ -2128,10 +2127,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 +2139,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 +2486,15 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
 
   // 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();
 a = 5;
@@

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

2023-12-04 Thread Aaron Puchert via cfe-commits 


@@ -2487,15 +2486,15 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
 
   // 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 = 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);

aaronpuchert wrote:

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

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-04 Thread Aaron Puchert via cfe-commits 


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

aaronpuchert wrote:

Nitpick: can you undo the whitespace change?

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-04 Thread Ziqing Luo via cfe-commits 

ziqingluo-90 wrote:

@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?

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-04 Thread Ziqing Luo via cfe-commits 

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

>From 00083bb1573561361ff0782f425ebec2a5218f34 Mon Sep 17 00:00:00 2001
From: ziqingluo-90 
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   | 27 +--
 .../SemaCXX/warn-thread-safety-analysis.cpp   |  8 ++
 2 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 7fdf22c2f3919..b65d4dfab8dea 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1010,6 +1010,8 @@ class ThreadSafetyAnalyzer {
   ThreadSafetyHandler 
   const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
+  // Maps constructed objects to `this` placeholder prior to initialization.
+  llvm::SmallDenseMap ConstructedObjects;
   FactManager FactMan;
   std::vector BlockInfo;
 
@@ -1530,7 +1532,6 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
 }
 
 namespace {
-
 /// 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
@@ -1543,8 +1544,6 @@ class BuildLockset : public 
ConstStmtVisitor {
   FactSet FSet;
   // The fact set for the function on exit.
   const FactSet 
-  /// Maps constructed objects to `this` placeholder prior to initialization.
-  llvm::SmallDenseMap ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
@@ -1808,7 +1807,7 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
   std::pair 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(Exp))
 Self = Placeholder.first;
@@ -2128,10 +2127,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 +2139,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 +2486,15 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
 
   // 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 = 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);
   }
   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

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

2023-12-03 Thread Aaron Puchert via cfe-commits 


@@ -1718,6 +1720,13 @@ struct TestScopedLockable {
 MutexLock{}, a = 5;
   }
 
+  void temporary2(int x) {
+if (check(MutexLock{}) || x) {
+
+}

aaronpuchert wrote:

The `if` is probably not needed here, right? We could just write
```suggestion
check(MutexLock{}) || x;
```
and should have the same CFG artifact.

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-03 Thread Aaron Puchert via cfe-commits 


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

aaronpuchert wrote:

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

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-03 Thread Aaron Puchert via cfe-commits 

https://github.com/aaronpuchert commented:

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.

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-03 Thread Aaron Puchert via cfe-commits 


@@ -1718,6 +1720,13 @@ struct TestScopedLockable {
 MutexLock{}, a = 5;
   }
 
+  void temporary2(int x) {

aaronpuchert wrote:

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

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-03 Thread Aaron Puchert via cfe-commits 


@@ -1718,6 +1720,13 @@ struct TestScopedLockable {
 MutexLock{}, a = 5;
   }
 
+  void temporary2(int x) {
+if (check(MutexLock{}) || x) {
+
+}
+check(MutexLock{});

aaronpuchert wrote:

The `check` here doesn't do anything, right?

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-03 Thread Aaron Puchert via cfe-commits 

https://github.com/aaronpuchert edited 
https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-12-03 Thread Aaron Puchert via cfe-commits 


@@ -1544,7 +1544,10 @@ class BuildLockset : public 
ConstStmtVisitor {
   // The fact set for the function on exit.
   const FactSet 
   /// Maps constructed objects to `this` placeholder prior to initialization.
-  llvm::SmallDenseMap 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.

aaronpuchert wrote:

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

https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-11-30 Thread via cfe-commits 

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff 668a4a238045e7f300b771f7e4cfa723d8bde237 
ecb13c381d99010dc04a2da4e945dfd1132777fd -- clang/lib/Analysis/ThreadSafety.cpp 
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
``





View the diff from clang-format here.


``diff
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 9549d1b398..401b1071db 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1530,7 +1530,8 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
 }
 
 namespace {
-using ConstructedObjectMapTy = llvm::SmallDenseMap;
+using ConstructedObjectMapTy =
+llvm::SmallDenseMap;
 /// 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

``




https://github.com/llvm/llvm-project/pull/74020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2023-11-30 Thread via cfe-commits 

llvmbot wrote:



@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Ziqing Luo (ziqingluo-90)


Changes

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.

---
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
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;
 /// 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 {
   // The fact set for the function on exit.
   const FactSet 
   /// Maps constructed objects to `this` placeholder prior to initialization.
-  llvm::SmallDenseMap 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 
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
@@ -1569,9 +1572,11 @@ class BuildLockset : public 
ConstStmtVisitor {
 
 public:
   BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo ,
-   const FactSet )
+   const FactSet ,
+   ConstructedObjectMapTy )
   : ConstStmtVisitor(), 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 ) {
   for (const auto  : LocksReleased)
 ExpectedFunctionExitSet.removeLock(FactMan, Lock);
 
+  ConstructedObjectMapTy ConstructedObjects;
+
   for (const auto *CurrBlock : *SortedGraph) {
 unsigned CurrBlockID = CurrBlock->getBlockID();
 CFGBlockInfo *CurrBlockInfo = [CurrBlockID];
@@ -2451,7 +2458,8 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
 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  : *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();
 a = 5;
@@ -1718,6 +1720,13 @@ struct TestScopedLockable {
 MutexLock{}, a = 5;
   }
 
+  void temporary2(int x) {
+if (check(MutexLock{}) || x) {
+
+}
+check(MutexLock{});
+  }
+
   void lifetime_extension() {
 const MutexLock  = MutexLock();
 a = 5;

``

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

2023-11-30 Thread 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 
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;
 /// 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 {
   // The fact set for the function on exit.
   const FactSet 
   /// Maps constructed objects to `this` placeholder prior to initialization.
-  llvm::SmallDenseMap 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 
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
@@ -1569,9 +1572,11 @@ class BuildLockset : public 
ConstStmtVisitor {
 
 public:
   BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo ,
-   const FactSet )
+   const FactSet ,
+   ConstructedObjectMapTy )
   : ConstStmtVisitor(), 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 ) {