[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-19 Thread Clement Courbet via cfe-commits 

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-19 Thread Clement Courbet via cfe-commits 


@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;

legrosbuffle wrote:

Done.

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-19 Thread Clement Courbet via cfe-commits 

https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/68572

>From f3b0c9708bbf6fb0962fb82b605a7fc0b52d4a5b Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`, which is on by default under 
`-Wthread-safety-reference`.

 - People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td |  4 +-
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 78 --
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 +++
 6 files changed, 163 insertions(+), 28 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 674eb9f4ef2e73f..2e4e22e4f90bee8 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1066,7 +1066,9 @@ def Most : DiagGroup<"most", [
 def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
 def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
 def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyReference  : DiagGroup<"thread-safety-reference",
+ [ThreadSafetyReferenceReturn]>;
 def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 7f39f5e79792c07..fb281773fdbf819 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 58dd7113665b132..7fdf22c2f3919cb 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod = nullptr;
+  const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const 
CapabilityExpr ) {
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast(SExp)) {
-if (!CurrentMethod)
+if (!isa_and_nonnull(CurrentFunction))
   return false;
 const ValueDecl *VD =

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-19 Thread Clement Courbet via cfe-commits 


@@ -2278,7 +2303,7 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
   PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph);
 
   CFGBlockInfo  = BlockInfo[CFGraph->getEntry().getBlockID()];
-  CFGBlockInfo= BlockInfo[CFGraph->getExit().getBlockID()];
+  CFGBlockInfo  = BlockInfo[CFGraph->getExit().getBlockID()];

legrosbuffle wrote:

Done.

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Aaron Puchert via cfe-commits 


@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;

aaronpuchert wrote:

I would suggest to undo the indentation changes here, as the flag is intended 
to be temporary.

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Aaron Puchert via cfe-commits 


@@ -2278,7 +2303,7 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
   PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph);
 
   CFGBlockInfo  = BlockInfo[CFGraph->getEntry().getBlockID()];
-  CFGBlockInfo= BlockInfo[CFGraph->getExit().getBlockID()];
+  CFGBlockInfo  = BlockInfo[CFGraph->getExit().getBlockID()];

aaronpuchert wrote:

This change seems unnecessary, but I don't mind either way.

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Aaron Puchert via cfe-commits 

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

Assuming that this is otherwise identical to the Phab review. I would suggest 
to add some release notes (in a separate change).

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Aaron Puchert via cfe-commits 

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Arthur Eubanks via cfe-commits 

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

the flag changes lgtm and the remaining code was already reviews, so push it!

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits 

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits 

https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/68572

>From db868042066e8b985d20c1b728729ba7102aaefa Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`, which is on by default under 
`-Wthread-safety-reference`.

 - People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td | 12 +--
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 6 files changed, 168 insertions(+), 33 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..a71afdd710c3bf4 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference",
+ [ThreadSafetyReferenceReturn]>;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 58a33e2807b7b0a..4c456a5561dc265 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 58dd7113665b132..54d0e95c6bd79a2 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod = nullptr;
+  const FunctionDecl *CurrentFunction;

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits 


@@ -1061,18 +1061,20 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;

legrosbuffle wrote:

Done, thanks.

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits 

https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/68572

>From 2d90dc0547f90c3a217428b2d3b8d2253ae80973 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td | 12 +--
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 6 files changed, 168 insertions(+), 33 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..a71afdd710c3bf4 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference",
+ [ThreadSafetyReferenceReturn]>;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 58a33e2807b7b0a..4c456a5561dc265 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits 

https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/68572

>From 11f5286f426d082f7fbcb578c0c6cabcd3660453 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td | 12 +--
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 6 files changed, 168 insertions(+), 33 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..a71afdd710c3bf4 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference",
+ [ThreadSafetyReferenceReturn]>;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b211680a0e9b6e9..c777d73dd4a769b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-13 Thread Arthur Eubanks via cfe-commits 


@@ -1061,18 +1061,20 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;

aeubanks wrote:

it's fine to put this in `ThreadSafety` and have it on by default (which it 
should be because it's improving thread safety analysis) since now people can 
turn it off with `-Wno-thread-safety-reference` while cleaning up their codebase

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-09 Thread Clement Courbet via cfe-commits 

legrosbuffle wrote:

@aeubanks FYI

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

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-09 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang


Changes

…… (#68394)"

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.

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


6 Files Affected:

- (modified) clang/include/clang/Analysis/Analyses/ThreadSafety.h (+7-1) 
- (modified) clang/include/clang/Basic/DiagnosticGroups.td (+8-6) 
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+9-1) 
- (modified) clang/lib/Analysis/ThreadSafety.cpp (+54-26) 
- (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+12) 
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+79) 


``diff
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..0462919af660285 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,18 +1061,20 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
   ThreadSafetyPrecise,
   ThreadSafetyReference]>;
 def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
-def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
+def ThreadSafetyBeta : DiagGroup<"thread-safety-beta",
+ [ThreadSafetyReferenceReturn]>;
 
 // Uniqueness Analysis warnings
 def Consumed   : DiagGroup<"consumed">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b211680a0e9b6e9..c777d73dd4a769b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-09 Thread Clement Courbet via cfe-commits 

https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/68572

…… (#68394)"

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.

>From a801efe3a6799df6ecee7ddf1ce77572d756cabb Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td | 14 ++--
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 6 files changed, 169 insertions(+), 34 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..0462919af660285 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,18 +1061,20 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
   ThreadSafetyPrecise,
   ThreadSafetyReference]>;
 def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
-def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
+def ThreadSafetyBeta : DiagGroup<"thread-safety-beta",
+ [ThreadSafetyReferenceReturn]>;
 
 // Uniqueness Analysis warnings
 def Consumed   : DiagGroup<"consumed">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b211680a0e9b6e9..c777d73dd4a769b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;