[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

aaronpuchert wrote:

Submitted as c1e7e4500c6e3b921f5e0cda8ba8d8d66e086db6.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Malek Ben Slimane via cfe-commits]

https://github.com/malek203 updated 
https://github.com/llvm/llvm-project/pull/110523

>From f5b5d4262318c4a2048c145aea923c1108ba65f2 Mon Sep 17 00:00:00 2001
From: Malek Ben Slimane 
Date: Wed, 11 Sep 2024 22:07:45 +0200
Subject: [PATCH] Thread Safety Analysis: Support passing scoped locks between
 functions with appropriate annotations

This is helpful when multiple functions operate on the same
capabilities, but we still want to use scoped lockable types for
readability and exception safety.
- Introduce support for thread safety annotations on function parameters
  marked with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring
  correct usage.
- Enhance the analysis to recognize and handle parameters annotated for
  thread safety, extending the scope of analysis to track these across
  function boundries.
- Verify that the underlying mutexes of function arguments match the
  expectations set by the annotations.

Limitation: This does not work when the attribute arguments are class
members, because attributes on function parameters are parsed
differently from attributes on functions.
---
 clang/docs/ReleaseNotes.rst   |  25 ++
 clang/docs/ThreadSafetyAnalysis.rst   |  50 ++-
 .../clang/Analysis/Analyses/ThreadSafety.h|  36 ++
 clang/include/clang/Basic/Attr.td |   8 +-
 .../clang/Basic/DiagnosticSemaKinds.td|  15 +
 clang/lib/Analysis/ThreadSafety.cpp   | 175 +++-
 clang/lib/Sema/AnalysisBasedWarnings.cpp  |  40 ++
 clang/lib/Sema/SemaDeclAttr.cpp   |  41 ++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 372 ++
 .../SemaCXX/warn-thread-safety-parsing.cpp|  42 +-
 10 files changed, 774 insertions(+), 30 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e511614fcf2451..63154451a09cc1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -336,6 +336,31 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- The :doc:`ThreadSafetyAnalysis` now supports passing scoped capabilities 
into functions:
+  an attribute on the scoped capability parameter indicates both the expected 
associated capabilities and,
+  like in the case of attributes on the function declaration itself, their 
state before and after the call.
+
+  .. code-block:: c++
+
+#include "mutex.h"
+
+Mutex mu1, mu2;
+int a GUARDED_BY(mu1);
+
+void require(MutexLocker& scope REQUIRES(mu1)) {
+  scope.Unlock();
+  a = 0; // Warning!  Requires mu1.
+  scope.Lock();
+}
+
+void testParameter() {
+  MutexLocker scope(&mu1), scope2(&mu2);
+  require(scope2); // Warning! Mutex managed by 'scope2' is 'mu2' instead 
of 'mu1'
+  require(scope); // OK.
+  scope.Unlock();
+  require(scope); // Warning!  Requires mu1.
+}
+
 Improvements to Clang's time-trace
 --
 
diff --git a/clang/docs/ThreadSafetyAnalysis.rst 
b/clang/docs/ThreadSafetyAnalysis.rst
index cc4089b97b4923..a5c3f7937dc049 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -187,10 +187,13 @@ REQUIRES(...), REQUIRES_SHARED(...)
 
 *Previously*: ``EXCLUSIVE_LOCKS_REQUIRED``, ``SHARED_LOCKS_REQUIRED``
 
-``REQUIRES`` is an attribute on functions or methods, which
+``REQUIRES`` is an attribute on functions, methods or function parameters of
+reference to :ref:`scoped_capability`-annotated type, which
 declares that the calling thread must have exclusive access to the given
 capabilities.  More than one capability may be specified.  The capabilities
 must be held on entry to the function, *and must still be held on exit*.
+Additionally, if the attribute is on a function parameter, it declares that
+the scoped capability manages the specified capabilities in the given order.
 
 ``REQUIRES_SHARED`` is similar, but requires only shared access.
 
@@ -211,6 +214,20 @@ must be held on entry to the function, *and must still be 
held on exit*.
 mu1.Unlock();
   }
 
+  void require(MutexLocker& scope REQUIRES(mu1)) {
+scope.Unlock();
+a = 0; // Warning!  Requires mu1.
+scope.Lock();
+  }
+
+  void testParameter() {
+MutexLocker scope(&mu1), scope2(&mu2);
+require(scope2); // Warning! Mutex managed by 'scope2' is 'mu2' instead of 
'mu1'
+require(scope); // OK.
+scope.Unlock();
+require(scope); // Warning!  Requires mu1.
+  }
+
 
 ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), RELEASE_SHARED(...), 
RELEASE_GENERIC(...)
 
--
@@ -218,10 +235,13 @@ ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), 
RELEASE_SHARED(...), RELEASE_GE
 *Previously*: ``EXCLUSIVE_LOCK_FUNCTION``, ``SHARED_LOCK_FUNCTION``,
 ``UNLOCK_FUNCTION``
 
-``ACQUIRE`` and ``ACQUIRE_SHARED`` are attributes on f

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -336,6 +336,31 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- The `Thread Safety Analysis 
`_ now supports passing 
scoped capabilities into functions:

aaronpuchert wrote:

Can you check if this works?
```suggestion
- The :doc:`ThreadSafetyAnalysis` now supports passing scoped capabilities into 
functions:
```

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Malek Ben Slimane via cfe-commits]

@@ -336,6 +336,40 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- The Thread Safety Analysis (#GH110523) now supports passing scoped 
capabilities into functions:
+  an attribute on the scoped capability parameter indicates both the expected 
associated capabilities and,
+  like in the case of attributes on the function declaration itself, their 
state before and after the call.
+
+  .. code-block:: c++
+
+#include "mutex.h"
+
+class MutexUnlocker {
+  Mutex* mu;
+
+public:
+  MutexUnlocker(Mutex* m) RELEASE(m) : mu(m)  { mu->Unlock(); }
+  ~MutexUnlocker() ACQUIRE(mu) { mu->Lock(); }
+};
+
+Mutex mu1, mu2;
+int a GUARDED_BY(mu1);
+
+void require(MutexLocker& scope REQUIRES(mu1)) {
+  scope.Unlock();
+  a = 0; // Warning!  Requires mu1.
+  scope.Lock();
+}
+
+void testParameter() {
+  MutexLocker scope(&mu1);
+  MutexLocker scope2(&mu2);
+  require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead 
of 'mu1'

malek203 wrote:

That's right and I will fix it in the Thread Safety Analysis documentation too.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Malek Ben Slimane via cfe-commits]

https://github.com/malek203 updated 
https://github.com/llvm/llvm-project/pull/110523

>From 07b0285587df72ada43705641f06cdce4280c483 Mon Sep 17 00:00:00 2001
From: Malek Ben Slimane 
Date: Wed, 11 Sep 2024 22:07:45 +0200
Subject: [PATCH] Thread Safety Analysis: Support passing scoped locks between
 functions with appropriate annotations

This is helpful when multiple functions operate on the same
capabilities, but we still want to use scoped lockable types for
readability and exception safety.
- Introduce support for thread safety annotations on function parameters
  marked with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring
  correct usage.
- Enhance the analysis to recognize and handle parameters annotated for
  thread safety, extending the scope of analysis to track these across
  function boundries.
- Verify that the underlying mutexes of function arguments match the
  expectations set by the annotations.

Limitation: This does not work when the attribute arguments are class
members, because attributes on function parameters are parsed
differently from attributes on functions.
---
 clang/docs/ReleaseNotes.rst   |  25 ++
 clang/docs/ThreadSafetyAnalysis.rst   |  50 ++-
 .../clang/Analysis/Analyses/ThreadSafety.h|  36 ++
 clang/include/clang/Basic/Attr.td |   8 +-
 .../clang/Basic/DiagnosticSemaKinds.td|  15 +
 clang/lib/Analysis/ThreadSafety.cpp   | 175 +++-
 clang/lib/Sema/AnalysisBasedWarnings.cpp  |  40 ++
 clang/lib/Sema/SemaDeclAttr.cpp   |  41 ++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 372 ++
 .../SemaCXX/warn-thread-safety-parsing.cpp|  42 +-
 10 files changed, 774 insertions(+), 30 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e511614fcf2451..e06b0bd31da234 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -336,6 +336,31 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- The `Thread Safety Analysis 
`_ now supports passing 
scoped capabilities into functions:
+  an attribute on the scoped capability parameter indicates both the expected 
associated capabilities and,
+  like in the case of attributes on the function declaration itself, their 
state before and after the call.
+
+  .. code-block:: c++
+
+#include "mutex.h"
+
+Mutex mu1, mu2;
+int a GUARDED_BY(mu1);
+
+void require(MutexLocker& scope REQUIRES(mu1)) {
+  scope.Unlock();
+  a = 0; // Warning!  Requires mu1.
+  scope.Lock();
+}
+
+void testParameter() {
+  MutexLocker scope(&mu1), scope2(&mu2);
+  require(scope2); // Warning! Mutex managed by 'scope2' is 'mu2' instead 
of 'mu1'
+  require(scope); // OK.
+  scope.Unlock();
+  require(scope); // Warning!  Requires mu1.
+}
+
 Improvements to Clang's time-trace
 --
 
diff --git a/clang/docs/ThreadSafetyAnalysis.rst 
b/clang/docs/ThreadSafetyAnalysis.rst
index cc4089b97b4923..a5c3f7937dc049 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -187,10 +187,13 @@ REQUIRES(...), REQUIRES_SHARED(...)
 
 *Previously*: ``EXCLUSIVE_LOCKS_REQUIRED``, ``SHARED_LOCKS_REQUIRED``
 
-``REQUIRES`` is an attribute on functions or methods, which
+``REQUIRES`` is an attribute on functions, methods or function parameters of
+reference to :ref:`scoped_capability`-annotated type, which
 declares that the calling thread must have exclusive access to the given
 capabilities.  More than one capability may be specified.  The capabilities
 must be held on entry to the function, *and must still be held on exit*.
+Additionally, if the attribute is on a function parameter, it declares that
+the scoped capability manages the specified capabilities in the given order.
 
 ``REQUIRES_SHARED`` is similar, but requires only shared access.
 
@@ -211,6 +214,20 @@ must be held on entry to the function, *and must still be 
held on exit*.
 mu1.Unlock();
   }
 
+  void require(MutexLocker& scope REQUIRES(mu1)) {
+scope.Unlock();
+a = 0; // Warning!  Requires mu1.
+scope.Lock();
+  }
+
+  void testParameter() {
+MutexLocker scope(&mu1), scope2(&mu2);
+require(scope2); // Warning! Mutex managed by 'scope2' is 'mu2' instead of 
'mu1'
+require(scope); // OK.
+scope.Unlock();
+require(scope); // Warning!  Requires mu1.
+  }
+
 
 ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), RELEASE_SHARED(...), 
RELEASE_GENERIC(...)
 
--
@@ -218,10 +235,13 @@ ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), 
RELEASE_SHARED(...), RELEASE_GE
 *Previously*: ``EXCLUSIVE_LOCK_FUNCTION``, ``SHARED_LOCK_FUNCTION``,
 ``UNLOCK_FUNCTION``
 

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -336,6 +336,40 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- The Thread Safety Analysis (#GH110523) now supports passing scoped 
capabilities into functions:
+  an attribute on the scoped capability parameter indicates both the expected 
associated capabilities and,
+  like in the case of attributes on the function declaration itself, their 
state before and after the call.
+
+  .. code-block:: c++
+
+#include "mutex.h"
+
+class MutexUnlocker {
+  Mutex* mu;
+
+public:
+  MutexUnlocker(Mutex* m) RELEASE(m) : mu(m)  { mu->Unlock(); }
+  ~MutexUnlocker() ACQUIRE(mu) { mu->Lock(); }
+};
+
+Mutex mu1, mu2;
+int a GUARDED_BY(mu1);
+
+void require(MutexLocker& scope REQUIRES(mu1)) {
+  scope.Unlock();
+  a = 0; // Warning!  Requires mu1.
+  scope.Lock();
+}
+
+void testParameter() {
+  MutexLocker scope(&mu1);
+  MutexLocker scope2(&mu2);

aaronpuchert wrote:

To condense it a bit:
```suggestion
  MutexLocker scope(&mu1), scope2(&mu2);
```

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -336,6 +336,40 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- The Thread Safety Analysis (#GH110523) now supports passing scoped 
capabilities into functions:
+  an attribute on the scoped capability parameter indicates both the expected 
associated capabilities and,
+  like in the case of attributes on the function declaration itself, their 
state before and after the call.
+
+  .. code-block:: c++
+
+#include "mutex.h"
+
+class MutexUnlocker {
+  Mutex* mu;
+
+public:
+  MutexUnlocker(Mutex* m) RELEASE(m) : mu(m)  { mu->Unlock(); }
+  ~MutexUnlocker() ACQUIRE(mu) { mu->Lock(); }
+};

aaronpuchert wrote:

This seems to be unused, did you add it by accident?

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -336,6 +336,40 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- The Thread Safety Analysis (#GH110523) now supports passing scoped 
capabilities into functions:
+  an attribute on the scoped capability parameter indicates both the expected 
associated capabilities and,
+  like in the case of attributes on the function declaration itself, their 
state before and after the call.
+
+  .. code-block:: c++
+
+#include "mutex.h"
+
+class MutexUnlocker {
+  Mutex* mu;
+
+public:
+  MutexUnlocker(Mutex* m) RELEASE(m) : mu(m)  { mu->Unlock(); }
+  ~MutexUnlocker() ACQUIRE(mu) { mu->Lock(); }
+};
+
+Mutex mu1, mu2;
+int a GUARDED_BY(mu1);
+
+void require(MutexLocker& scope REQUIRES(mu1)) {
+  scope.Unlock();
+  a = 0; // Warning!  Requires mu1.
+  scope.Lock();
+}
+
+void testParameter() {
+  MutexLocker scope(&mu1);
+  MutexLocker scope2(&mu2);
+  require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead 
of 'mu1'

aaronpuchert wrote:

```suggestion
  require(scope2); // Warning! Mutex managed by 'scope2' is 'mu2' instead 
of 'mu1'
```
or does the warning print the parameter name?

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

https://github.com/aaronpuchert commented:

@AaronBallman, please also have a look at the Release Note.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Malek Ben Slimane via cfe-commits]

@@ -3984,6 +3984,10 @@ def warn_thread_attribute_decl_not_lockable : Warning<
 def warn_thread_attribute_decl_not_pointer : Warning<
   "%0 only applies to pointer types; type here is %1">,
   InGroup, DefaultIgnore;
+def warn_thread_attribute_not_on_scoped_lockable_param : Warning<
+  "%0 attribute applies to function parameters only if their type is a "
+  "reference to a 'scoped_lockable'-annotated type">,
+  InGroup, DefaultIgnore;

malek203 wrote:

The `DefaultIgnore` setting aligns with the fact that the warnings 
`-Wthread-safety-analysis` and `-Wthread-safety-attributes` are not enabled by 
default.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Malek Ben Slimane via cfe-commits]

@@ -1915,6 +1936,101 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
 }
   }
 
+  std::optional Args;
+  if (Exp) {
+if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else
+  llvm_unreachable("Unknown call kind");

malek203 wrote:

`CXXMemberCallExpr` inherits from `CallExpr`, so it is already covered.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Malek Ben Slimane via cfe-commits]

https://github.com/malek203 updated 
https://github.com/llvm/llvm-project/pull/110523

>From 70b03bd0532f55f25f51adda0604b10d4b2150e0 Mon Sep 17 00:00:00 2001
From: Malek Ben Slimane 
Date: Wed, 11 Sep 2024 22:07:45 +0200
Subject: [PATCH] Thread Safety Analysis: Support passing scoped locks between
 functions with appropriate annotations

This is helpful when multiple functions operate on the same
capabilities, but we still want to use scoped lockable types for
readability and exception safety.
- Introduce support for thread safety annotations on function parameters
  marked with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring
  correct usage.
- Enhance the analysis to recognize and handle parameters annotated for
  thread safety, extending the scope of analysis to track these across
  function boundries.
- Verify that the underlying mutexes of function arguments match the
  expectations set by the annotations.

Limitation: This does not work when the attribute arguments are class
members, because attributes on function parameters are parsed
differently from attributes on functions.
---
 clang/docs/ReleaseNotes.rst   |  34 ++
 clang/docs/ThreadSafetyAnalysis.rst   |  51 ++-
 .../clang/Analysis/Analyses/ThreadSafety.h|  36 ++
 clang/include/clang/Basic/Attr.td |   8 +-
 .../clang/Basic/DiagnosticSemaKinds.td|  15 +
 clang/lib/Analysis/ThreadSafety.cpp   | 175 +++-
 clang/lib/Sema/AnalysisBasedWarnings.cpp  |  40 ++
 clang/lib/Sema/SemaDeclAttr.cpp   |  41 ++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 372 ++
 .../SemaCXX/warn-thread-safety-parsing.cpp|  42 +-
 10 files changed, 784 insertions(+), 30 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e511614fcf2451..dbd98edd731c98 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -336,6 +336,40 @@ Improvements to Clang's diagnostics
   local variables passed to function calls using the ``[[clang::musttail]]``
   attribute.
 
+- The Thread Safety Analysis (#GH110523) now supports passing scoped 
capabilities into functions:
+  an attribute on the scoped capability parameter indicates both the expected 
associated capabilities and,
+  like in the case of attributes on the function declaration itself, their 
state before and after the call.
+
+  .. code-block:: c++
+
+#include "mutex.h"
+
+class MutexUnlocker {
+  Mutex* mu;
+
+public:
+  MutexUnlocker(Mutex* m) RELEASE(m) : mu(m)  { mu->Unlock(); }
+  ~MutexUnlocker() ACQUIRE(mu) { mu->Lock(); }
+};
+
+Mutex mu1, mu2;
+int a GUARDED_BY(mu1);
+
+void require(MutexLocker& scope REQUIRES(mu1)) {
+  scope.Unlock();
+  a = 0; // Warning!  Requires mu1.
+  scope.Lock();
+}
+
+void testParameter() {
+  MutexLocker scope(&mu1);
+  MutexLocker scope2(&mu2);
+  require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead 
of 'mu1'
+  require(scope); // OK.
+  scope.Unlock();
+  require(scope); // Warning!  Requires mu1.
+}
+
 Improvements to Clang's time-trace
 --
 
diff --git a/clang/docs/ThreadSafetyAnalysis.rst 
b/clang/docs/ThreadSafetyAnalysis.rst
index cc4089b97b4923..a312e8e1f5924b 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -187,10 +187,13 @@ REQUIRES(...), REQUIRES_SHARED(...)
 
 *Previously*: ``EXCLUSIVE_LOCKS_REQUIRED``, ``SHARED_LOCKS_REQUIRED``
 
-``REQUIRES`` is an attribute on functions or methods, which
+``REQUIRES`` is an attribute on functions, methods or function parameters of
+reference to :ref:`scoped_capability`-annotated type, which
 declares that the calling thread must have exclusive access to the given
 capabilities.  More than one capability may be specified.  The capabilities
 must be held on entry to the function, *and must still be held on exit*.
+Additionally, if the attribute is on a function parameter, it declares that
+the scoped capability manages the specified capabilities in the given order.
 
 ``REQUIRES_SHARED`` is similar, but requires only shared access.
 
@@ -211,6 +214,21 @@ must be held on entry to the function, *and must still be 
held on exit*.
 mu1.Unlock();
   }
 
+  void require(MutexLocker& scope REQUIRES(mu1)) {
+scope.Unlock();
+a = 0; // Warning!  Requires mu1.
+scope.Lock();
+  }
+
+  void testParameter() {
+MutexLocker scope(&mu1);
+MutexLocker scope2(&mu2);
+require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead of 
'mu1'
+require(scope); // OK.
+scope.Unlock();
+require(scope); // Warning!  Requires mu1.
+  }
+
 
 ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), RELEASE_SHARED(...), 
RELEASE_GENERIC(...)
 
--
@@ -218,10 +236,13 @

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman commented:

Also, please add a release note to `clang/docs/ReleaseNotes.rst` so users know 
about the new functionality. Thank you for working on this!

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Ballman via cfe-commits]

@@ -3984,6 +3984,10 @@ def warn_thread_attribute_decl_not_lockable : Warning<
 def warn_thread_attribute_decl_not_pointer : Warning<
   "%0 only applies to pointer types; type here is %1">,
   InGroup, DefaultIgnore;
+def warn_thread_attribute_not_on_scoped_lockable_param : Warning<
+  "%0 attribute applies to function parameters only if their type is a "
+  "reference to a 'scoped_lockable'-annotated type">,
+  InGroup, DefaultIgnore;

AaronBallman wrote:

Why is this diagnostic off by default? It seems like this is a case users 
wouldn't want the attribute to be silently ignored.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Ballman via cfe-commits]

@@ -418,6 +431,18 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl 
*D,
   }
 }
 
+static bool checkFunParamsAreScopedLockable(Sema &S,
+const ParmVarDecl *ParamDecl,
+const ParsedAttr &AL) {
+  QualType ParamType = ParamDecl->getType();
+  if (const auto *RefType = ParamType->getAs())
+if (checkRecordTypeForScopedCapability(S, RefType->getPointeeType()))
+  return true;

AaronBallman wrote:

```suggestion
  if (const auto *RefType = ParamType->getAs(); RefType &&
  checkRecordTypeForScopedCapability(S, RefType->getPointeeType()))
return true;
```

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Ballman via cfe-commits]

@@ -1915,6 +1936,101 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
 }
   }
 
+  std::optional Args;
+  if (Exp) {
+if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else
+  llvm_unreachable("Unknown call kind");

AaronBallman wrote:

`CXXMemberCallExpr`?

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Ballman via cfe-commits]

@@ -645,6 +670,10 @@ static void handleLockReturnedAttr(Sema &S, Decl *D, const 
ParsedAttr &AL) {
 }
 
 static void handleLocksExcludedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (const auto *ParmDecl = dyn_cast(D))
+if (!checkFunParamsAreScopedLockable(S, ParmDecl, AL))
+  return;

AaronBallman wrote:

```suggestion
  if (const auto *ParmDecl = dyn_cast(D); ParmDecl &&
  !checkFunParamsAreScopedLockable(S, ParmDecl, AL))
return;
```
Same suggestion applies a few times below as well.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Ballman via cfe-commits]

@@ -4039,6 +4043,17 @@ def warn_acquired_before : Warning<
 def warn_acquired_before_after_cycle : Warning<
   "cycle in acquired_before/after dependencies, starting with '%0'">,
   InGroup, DefaultIgnore;
+def warn_unmatched_underlying_mutexes : Warning<
+  "%0 managed by '%1' is '%3' instead of '%2'">,
+  InGroup, DefaultIgnore;
+def warn_expect_more_underlying_mutexes : Warning<
+  "%0 '%2' not managed by '%1'">,
+  InGroup, DefaultIgnore;
+def warn_expect_fewer_underlying_mutexes : Warning<
+  "did not expect %0 '%2' to be managed by '%1'">,
+  InGroup, DefaultIgnore;

AaronBallman wrote:

Should these really be `DefaultIgnore`? In all of these cases, it seems like 
users would want to know about the problems?

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Ballman via cfe-commits]

@@ -1915,6 +1936,101 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
 }
   }
 
+  std::optional Args;
+  if (Exp) {
+if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else
+  llvm_unreachable("Unknown call kind");
+  }
+  const FunctionDecl *CalledFunction = dyn_cast(D);

AaronBallman wrote:

```suggestion
  const auto *CalledFunction = dyn_cast(D);
```

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

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

This looks great! Let's wait a bit for the other reviewers, but otherwise I'm 
ready to merge it.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Malek Ben Slimane via cfe-commits]

https://github.com/malek203 updated 
https://github.com/llvm/llvm-project/pull/110523

>From b66684cb44abcf3b0a191303d1dc01e6aa0d8dec Mon Sep 17 00:00:00 2001
From: Malek Ben Slimane 
Date: Wed, 11 Sep 2024 22:07:45 +0200
Subject: [PATCH] Thread Safety Analysis: Support passing scoped locks between
 functions with appropriate annotations

This is helpful when multiple functions operate on the same
capabilities, but we still want to use scoped lockable types for
readability and exception safety.
- Introduce support for thread safety annotations on function parameters
  marked with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring
  correct usage.
- Enhance the analysis to recognize and handle parameters annotated for
  thread safety, extending the scope of analysis to track these across
  function boundries.
- Verify that the underlying mutexes of function arguments match the
  expectations set by the annotations.

Limitation: This does not work when the attribute arguments are class
members, because attributes on function parameters are parsed
differently from attributes on functions.
---
 clang/docs/ThreadSafetyAnalysis.rst   |  51 ++-
 .../clang/Analysis/Analyses/ThreadSafety.h|  36 ++
 clang/include/clang/Basic/Attr.td |   8 +-
 .../clang/Basic/DiagnosticSemaKinds.td|  15 +
 clang/lib/Analysis/ThreadSafety.cpp   | 175 -
 clang/lib/Sema/AnalysisBasedWarnings.cpp  |  40 ++
 clang/lib/Sema/SemaDeclAttr.cpp   |  40 ++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 361 ++
 .../SemaCXX/warn-thread-safety-parsing.cpp|  42 +-
 9 files changed, 738 insertions(+), 30 deletions(-)

diff --git a/clang/docs/ThreadSafetyAnalysis.rst 
b/clang/docs/ThreadSafetyAnalysis.rst
index cc4089b97b4923..a312e8e1f5924b 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -187,10 +187,13 @@ REQUIRES(...), REQUIRES_SHARED(...)
 
 *Previously*: ``EXCLUSIVE_LOCKS_REQUIRED``, ``SHARED_LOCKS_REQUIRED``
 
-``REQUIRES`` is an attribute on functions or methods, which
+``REQUIRES`` is an attribute on functions, methods or function parameters of
+reference to :ref:`scoped_capability`-annotated type, which
 declares that the calling thread must have exclusive access to the given
 capabilities.  More than one capability may be specified.  The capabilities
 must be held on entry to the function, *and must still be held on exit*.
+Additionally, if the attribute is on a function parameter, it declares that
+the scoped capability manages the specified capabilities in the given order.
 
 ``REQUIRES_SHARED`` is similar, but requires only shared access.
 
@@ -211,6 +214,21 @@ must be held on entry to the function, *and must still be 
held on exit*.
 mu1.Unlock();
   }
 
+  void require(MutexLocker& scope REQUIRES(mu1)) {
+scope.Unlock();
+a = 0; // Warning!  Requires mu1.
+scope.Lock();
+  }
+
+  void testParameter() {
+MutexLocker scope(&mu1);
+MutexLocker scope2(&mu2);
+require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead of 
'mu1'
+require(scope); // OK.
+scope.Unlock();
+require(scope); // Warning!  Requires mu1.
+  }
+
 
 ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), RELEASE_SHARED(...), 
RELEASE_GENERIC(...)
 
--
@@ -218,10 +236,13 @@ ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), 
RELEASE_SHARED(...), RELEASE_GE
 *Previously*: ``EXCLUSIVE_LOCK_FUNCTION``, ``SHARED_LOCK_FUNCTION``,
 ``UNLOCK_FUNCTION``
 
-``ACQUIRE`` and ``ACQUIRE_SHARED`` are attributes on functions or methods
-declaring that the function acquires a capability, but does not release it.
+``ACQUIRE`` and ``ACQUIRE_SHARED`` are attributes on functions, methods
+or function parameters of reference to :ref:`scoped_capability`-annotated type,
+which declare that the function acquires a capability, but does not release it.
 The given capability must not be held on entry, and will be held on exit
 (exclusively for ``ACQUIRE``, shared for ``ACQUIRE_SHARED``).
+Additionally, if the attribute is on a function parameter, it declares that
+the scoped capability manages the specified capabilities in the given order.
 
 ``RELEASE``, ``RELEASE_SHARED``, and ``RELEASE_GENERIC`` declare that the
 function releases the given capability.  The capability must be held on entry
@@ -249,6 +270,14 @@ shared for ``RELEASE_GENERIC``), and will no longer be 
held on exit.
 myObject.doSomething();  // Warning, mu is not locked.
   }
 
+  void release(MutexLocker& scope RELEASE(mu)) {
+  }  // Warning!  Need to unlock mu.
+
+  void testParameter() {
+MutexLocker scope(&mu);
+release(scope);
+  }
+
 If no argument is passed to ``ACQUIRE`` or ``RELEASE``, then the argument is
 assumed to be ``this``, and the analysis will not check the body of the
 

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -1915,6 +1936,101 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
 }
   }
 
+  std::optional Args;
+  if (Exp) {
+if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else
+  llvm_unreachable("Unknown call kind");
+  }
+  const FunctionDecl *CalledFunction = dyn_cast(D);
+  if (CalledFunction && Args.has_value()) {
+for (auto [Param, Arg] : zip(CalledFunction->parameters(), *Args)) {
+  CapExprSet DeclaredLocks;
+  for (const Attr *At : Param->attrs()) {
+switch (At->getKind()) {
+case attr::AcquireCapability: {
+  const auto *A = cast(At);
+  Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
+  : ExclusiveLocksToAdd,
+A, Exp, D, Self);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::ReleaseCapability: {
+  const auto *A = cast(At);
+  if (A->isGeneric())
+Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, Self);
+  else if (A->isShared())
+Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
+  else
+Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::RequiresCapability: {
+  const auto *A = cast(At);
+  for (auto *Arg : A->args())
+Analyzer->warnIfMutexNotHeld(FSet, D, Exp,
+ A->isShared() ? AK_Read : AK_Written,
+ Arg, POK_FunctionCall, Self, Loc);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::LocksExcluded: {
+  const auto *A = cast(At);
+  for (auto *Arg : A->args())
+Analyzer->warnIfMutexHeld(FSet, D, Exp, Arg, Self, Loc);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+default:
+  break;
+}
+  }
+  if (DeclaredLocks.size() == 0)
+continue;
+  CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(Arg, nullptr);

aaronpuchert wrote:

We should probably just use `translate` here. This is not an attribute 
expression. If you look at `translateAttrExpr`, it additionally handles 
wildcards, negative capabilities, and so on, but those only exist in attribute 
arguments.

We can build the `CapabilityExpr` as below, or even better, build it on-the-fly 
for `findLock`.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -3171,6 +3171,372 @@ void lockUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
 
 } // end namespace ScopedUnlock
 
+namespace PassingScope {
+
+class SCOPED_LOCKABLE RelockableScope {
+public:
+  RelockableScope(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  void Release() UNLOCK_FUNCTION();
+  void Acquire() EXCLUSIVE_LOCK_FUNCTION();
+  ~RelockableScope() EXCLUSIVE_UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE ReaderRelockableScope {
+public:
+  ReaderRelockableScope(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+  void Release() UNLOCK_FUNCTION();
+  void Acquire() SHARED_LOCK_FUNCTION();
+  ~ReaderRelockableScope() UNLOCK_FUNCTION();
+};
+
+Mutex mu;
+Mutex mu1;
+Mutex mu2;
+int x GUARDED_BY(mu);
+int y GUARDED_BY(mu) GUARDED_BY(mu2);
+void print(int);
+
+// Analysis inside the function with annotated parameters
+
+void sharedRequired(ReleasableMutexLock& scope SHARED_LOCKS_REQUIRED(mu)) {
+  print(x); // OK.
+}
+
+void sharedAcquire(ReaderRelockableScope& scope SHARED_LOCK_FUNCTION(mu)) {
+  scope.Acquire();
+  print(x);
+}
+
+void sharedRelease(RelockableScope& scope SHARED_UNLOCK_FUNCTION(mu)) {
+  print(x);
+  scope.Release();
+  print(x); // expected-warning{{reading variable 'x' requires holding mutex 
'mu'}}
+}
+
+void requiredLock(ReleasableMutexLock& scope EXCLUSIVE_LOCKS_REQUIRED(mu)) {
+  x = 1; // OK.
+}
+
+void reacquireRequiredLock(RelockableScope& scope 
EXCLUSIVE_LOCKS_REQUIRED(mu)) {
+  scope.Release();
+  scope.Acquire();
+  x = 2; // OK.
+}
+
+void releaseSingleMutex(ReleasableMutexLock& scope 
EXCLUSIVE_UNLOCK_FUNCTION(mu)) {
+  x = 1; // OK.
+  scope.Release();
+  x = 2; // expected-warning{{writing variable 'x' requires holding mutex 'mu' 
exclusively}}
+}
+
+void releaseMultipleMutexes(ReleasableMutexLock& scope 
EXCLUSIVE_UNLOCK_FUNCTION(mu, mu2)) {
+  y = 1; // OK.
+  scope.Release();
+  y = 2; // expected-warning{{writing variable 'y' requires holding mutex 'mu' 
exclusively}}
+ // expected-warning@-1{{writing variable 'y' requires holding mutex 
'mu2' exclusively}}
+}
+
+void acquireLock(RelockableScope& scope EXCLUSIVE_LOCK_FUNCTION(mu)) {
+  x = 1; // expected-warning{{writing variable 'x' requires holding mutex 'mu' 
exclusively}}
+  scope.Acquire();
+  x = 2; // OK.
+}
+
+void acquireMultipleLocks(RelockableScope& scope EXCLUSIVE_LOCK_FUNCTION(mu, 
mu2)) {
+  y = 1; // expected-warning{{writing variable 'y' requires holding mutex 'mu' 
exclusively}}
+ // expected-warning@-1{{writing variable 'y' requires holding mutex 
'mu2' exclusively}}
+  scope.Acquire();
+  y = 2;  // OK.
+}
+
+void excludedLock(ReleasableMutexLock& scope LOCKS_EXCLUDED(mu)) {
+  x = 1; // expected-warning{{writing variable 'x' requires holding mutex 'mu' 
exclusively}}
+}
+
+void acquireAndReleaseExcludedLocks(RelockableScope& scope LOCKS_EXCLUDED(mu)) 
{
+  scope.Acquire();
+  scope.Release();
+}
+
+// expected-note@+1{{mutex acquired here}}
+void unreleasedMutex(ReleasableMutexLock& scope EXCLUSIVE_UNLOCK_FUNCTION(mu)) 
{
+  x = 1; // OK.
+} // expected-warning{{mutex 'mu' is still held at the end of function}}
+
+// expected-note@+1{{mutex acquired here}}
+void acquireAlreadyHeldMutex(RelockableScope& scope 
EXCLUSIVE_UNLOCK_FUNCTION(mu)) {
+  scope.Acquire(); // expected-warning{{acquiring mutex 'mu' that is already 
held}}
+  scope.Release();
+}
+
+void reacquireMutex(RelockableScope& scope EXCLUSIVE_UNLOCK_FUNCTION(mu)) {
+  scope.Release();
+  scope.Acquire(); // expected-note{{mutex acquired here}}
+  x = 2; // OK.
+} // expected-warning{{mutex 'mu' is still held at the end of function}}
+
+// expected-note@+1{{mutex acquired here}}
+void requireSingleMutex(ReleasableMutexLock& scope 
EXCLUSIVE_LOCKS_REQUIRED(mu)) {
+  x = 1; // OK.
+  scope.Release();
+  x = 2; // expected-warning{{writing variable 'x' requires holding mutex 'mu' 
exclusively}}
+} // expected-warning{{expecting mutex 'mu' to be held at the end of function}}
+
+// expected-note@+1 2{{mutex acquired here}}
+void requireMultipleMutexes(ReleasableMutexLock& scope 
EXCLUSIVE_LOCKS_REQUIRED(mu, mu2)) {
+  y = 1; // OK.
+  scope.Release();
+  y = 2; // expected-warning{{writing variable 'y' requires holding mutex 'mu' 
exclusively}}
+ // expected-warning@-1{{writing variable 'y' requires holding mutex 
'mu2' exclusively}}
+} // expected-warning{{expecting mutex 'mu' to be held at the end of function}}
+  // expected-warning@-1{{expecting mutex 'mu2' to be held at the end of 
function}}
+
+// expected-note@+1{{mutex acquired here}}
+void acquireAlreadyHeldLock(RelockableScope& scope 
EXCLUSIVE_LOCKS_REQUIRED(mu)) {
+  scope.Acquire(); // expected-warning{{acquiring mutex 'mu' that is already 
held}}
+}
+
+// expected-note@+1 {{mutex acquired here}}
+void releaseWithoutHoldingLock(ReleasableMutexLock& scope 
EXCLUSIVE_LOCK_FUNCTION(mu)) {
+  scope.Release(); // expected-warning{{releasing mutex 'mu' that was not 
held}}
+} // expected-warning{{expecting mutex 'mu' to be held at the end of function}}
+
+// ex

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -3171,6 +3171,372 @@ void lockUnlock() EXCLUSIVE_LOCKS_REQUIRED(mu) {
 
 } // end namespace ScopedUnlock
 
+namespace PassingScope {
+
+class SCOPED_LOCKABLE RelockableScope {
+public:
+  RelockableScope(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
+  void Release() UNLOCK_FUNCTION();
+  void Acquire() EXCLUSIVE_LOCK_FUNCTION();
+  ~RelockableScope() EXCLUSIVE_UNLOCK_FUNCTION();
+};
+
+class SCOPED_LOCKABLE ReaderRelockableScope {
+public:
+  ReaderRelockableScope(Mutex *mu) SHARED_LOCK_FUNCTION(mu);
+  void Release() UNLOCK_FUNCTION();
+  void Acquire() SHARED_LOCK_FUNCTION();
+  ~ReaderRelockableScope() UNLOCK_FUNCTION();
+};
+
+Mutex mu;
+Mutex mu1;
+Mutex mu2;
+int x GUARDED_BY(mu);
+int y GUARDED_BY(mu) GUARDED_BY(mu2);
+void print(int);
+
+// Analysis inside the function with annotated parameters
+
+void sharedRequired(ReleasableMutexLock& scope SHARED_LOCKS_REQUIRED(mu)) {
+  print(x); // OK.
+}
+
+void sharedAcquire(ReaderRelockableScope& scope SHARED_LOCK_FUNCTION(mu)) {
+  scope.Acquire();
+  print(x);
+}
+
+void sharedRelease(RelockableScope& scope SHARED_UNLOCK_FUNCTION(mu)) {
+  print(x);
+  scope.Release();
+  print(x); // expected-warning{{reading variable 'x' requires holding mutex 
'mu'}}
+}
+
+void requiredLock(ReleasableMutexLock& scope EXCLUSIVE_LOCKS_REQUIRED(mu)) {
+  x = 1; // OK.
+}
+
+void reacquireRequiredLock(RelockableScope& scope 
EXCLUSIVE_LOCKS_REQUIRED(mu)) {
+  scope.Release();
+  scope.Acquire();
+  x = 2; // OK.
+}
+
+void releaseSingleMutex(ReleasableMutexLock& scope 
EXCLUSIVE_UNLOCK_FUNCTION(mu)) {
+  x = 1; // OK.
+  scope.Release();
+  x = 2; // expected-warning{{writing variable 'x' requires holding mutex 'mu' 
exclusively}}
+}
+
+void releaseMultipleMutexes(ReleasableMutexLock& scope 
EXCLUSIVE_UNLOCK_FUNCTION(mu, mu2)) {
+  y = 1; // OK.
+  scope.Release();
+  y = 2; // expected-warning{{writing variable 'y' requires holding mutex 'mu' 
exclusively}}
+ // expected-warning@-1{{writing variable 'y' requires holding mutex 
'mu2' exclusively}}
+}
+
+void acquireLock(RelockableScope& scope EXCLUSIVE_LOCK_FUNCTION(mu)) {
+  x = 1; // expected-warning{{writing variable 'x' requires holding mutex 'mu' 
exclusively}}
+  scope.Acquire();
+  x = 2; // OK.
+}
+
+void acquireMultipleLocks(RelockableScope& scope EXCLUSIVE_LOCK_FUNCTION(mu, 
mu2)) {
+  y = 1; // expected-warning{{writing variable 'y' requires holding mutex 'mu' 
exclusively}}
+ // expected-warning@-1{{writing variable 'y' requires holding mutex 
'mu2' exclusively}}
+  scope.Acquire();
+  y = 2;  // OK.
+}
+
+void excludedLock(ReleasableMutexLock& scope LOCKS_EXCLUDED(mu)) {
+  x = 1; // expected-warning{{writing variable 'x' requires holding mutex 'mu' 
exclusively}}
+}
+
+void acquireAndReleaseExcludedLocks(RelockableScope& scope LOCKS_EXCLUDED(mu)) 
{
+  scope.Acquire();
+  scope.Release();
+}
+
+// expected-note@+1{{mutex acquired here}}
+void unreleasedMutex(ReleasableMutexLock& scope EXCLUSIVE_UNLOCK_FUNCTION(mu)) 
{
+  x = 1; // OK.
+} // expected-warning{{mutex 'mu' is still held at the end of function}}
+
+// expected-note@+1{{mutex acquired here}}
+void acquireAlreadyHeldMutex(RelockableScope& scope 
EXCLUSIVE_UNLOCK_FUNCTION(mu)) {
+  scope.Acquire(); // expected-warning{{acquiring mutex 'mu' that is already 
held}}
+  scope.Release();
+}
+
+void reacquireMutex(RelockableScope& scope EXCLUSIVE_UNLOCK_FUNCTION(mu)) {
+  scope.Release();
+  scope.Acquire(); // expected-note{{mutex acquired here}}
+  x = 2; // OK.
+} // expected-warning{{mutex 'mu' is still held at the end of function}}
+
+// expected-note@+1{{mutex acquired here}}
+void requireSingleMutex(ReleasableMutexLock& scope 
EXCLUSIVE_LOCKS_REQUIRED(mu)) {
+  x = 1; // OK.
+  scope.Release();
+  x = 2; // expected-warning{{writing variable 'x' requires holding mutex 'mu' 
exclusively}}
+} // expected-warning{{expecting mutex 'mu' to be held at the end of function}}
+
+// expected-note@+1 2{{mutex acquired here}}
+void requireMultipleMutexes(ReleasableMutexLock& scope 
EXCLUSIVE_LOCKS_REQUIRED(mu, mu2)) {
+  y = 1; // OK.
+  scope.Release();
+  y = 2; // expected-warning{{writing variable 'y' requires holding mutex 'mu' 
exclusively}}
+ // expected-warning@-1{{writing variable 'y' requires holding mutex 
'mu2' exclusively}}
+} // expected-warning{{expecting mutex 'mu' to be held at the end of function}}
+  // expected-warning@-1{{expecting mutex 'mu2' to be held at the end of 
function}}
+
+// expected-note@+1{{mutex acquired here}}
+void acquireAlreadyHeldLock(RelockableScope& scope 
EXCLUSIVE_LOCKS_REQUIRED(mu)) {
+  scope.Acquire(); // expected-warning{{acquiring mutex 'mu' that is already 
held}}
+}
+
+// expected-note@+1 {{mutex acquired here}}
+void releaseWithoutHoldingLock(ReleasableMutexLock& scope 
EXCLUSIVE_LOCK_FUNCTION(mu)) {
+  scope.Release(); // expected-warning{{releasing mutex 'mu' that was not 
held}}
+} // expected-warning{{expecting mutex 'mu' to be held at the end of function}}
+
+// ex

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -1915,6 +1936,101 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
 }
   }
 
+  std::optional Args;
+  if (Exp) {
+if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else
+  llvm_unreachable("Unknown call kind");
+  }
+  const FunctionDecl *CalledFunction = dyn_cast(D);
+  if (CalledFunction && Args.has_value()) {
+for (auto [Param, Arg] : zip(CalledFunction->parameters(), *Args)) {
+  CapExprSet DeclaredLocks;
+  for (const Attr *At : Param->attrs()) {
+switch (At->getKind()) {
+case attr::AcquireCapability: {
+  const auto *A = cast(At);
+  Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
+  : ExclusiveLocksToAdd,
+A, Exp, D, Self);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::ReleaseCapability: {
+  const auto *A = cast(At);
+  if (A->isGeneric())
+Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, Self);
+  else if (A->isShared())
+Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
+  else
+Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::RequiresCapability: {
+  const auto *A = cast(At);
+  for (auto *Arg : A->args())
+Analyzer->warnIfMutexNotHeld(FSet, D, Exp,
+ A->isShared() ? AK_Read : AK_Written,
+ Arg, POK_FunctionCall, Self, Loc);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::LocksExcluded: {
+  const auto *A = cast(At);
+  for (auto *Arg : A->args())
+Analyzer->warnIfMutexHeld(FSet, D, Exp, Arg, Self, Loc);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+default:
+  break;
+}
+  }
+  if (DeclaredLocks.size() == 0)
+continue;
+  CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(Arg, nullptr);
+  if (const auto *CBTE = 
dyn_cast(Arg->IgnoreCasts());
+  Cp.isInvalid() && CBTE) {
+if (auto Object = 
Analyzer->ConstructedObjects.find(CBTE->getSubExpr());
+Object != Analyzer->ConstructedObjects.end())
+  Cp = CapabilityExpr(Object->second, StringRef(), false);

aaronpuchert wrote:

This is slightly wrong, but we currently use "mutex" as fallback, see e.g. 
`ClassifyDiagnostic` in `ThreadSafetyCommon.cpp`.
```suggestion
  Cp = CapabilityExpr(Object->second, StringRef("mutex"), false);
```

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -1915,6 +1936,101 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
 }
   }
 
+  std::optional Args;
+  if (Exp) {
+if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else
+  llvm_unreachable("Unknown call kind");
+  }
+  const FunctionDecl *CalledFunction = dyn_cast(D);
+  if (CalledFunction && Args.has_value()) {
+for (auto [Param, Arg] : zip(CalledFunction->parameters(), *Args)) {
+  CapExprSet DeclaredLocks;
+  for (const Attr *At : Param->attrs()) {
+switch (At->getKind()) {
+case attr::AcquireCapability: {
+  const auto *A = cast(At);
+  Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
+  : ExclusiveLocksToAdd,
+A, Exp, D, Self);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::ReleaseCapability: {
+  const auto *A = cast(At);
+  if (A->isGeneric())
+Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, Self);
+  else if (A->isShared())
+Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
+  else
+Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::RequiresCapability: {
+  const auto *A = cast(At);
+  for (auto *Arg : A->args())
+Analyzer->warnIfMutexNotHeld(FSet, D, Exp,
+ A->isShared() ? AK_Read : AK_Written,
+ Arg, POK_FunctionCall, Self, Loc);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::LocksExcluded: {
+  const auto *A = cast(At);
+  for (auto *Arg : A->args())
+Analyzer->warnIfMutexHeld(FSet, D, Exp, Arg, Self, Loc);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+default:
+  break;
+}
+  }
+  if (DeclaredLocks.size() == 0)
+continue;
+  CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(Arg, nullptr);
+  if (const auto *CBTE = 
dyn_cast(Arg->IgnoreCasts());
+  Cp.isInvalid() && CBTE) {
+if (auto Object = 
Analyzer->ConstructedObjects.find(CBTE->getSubExpr());
+Object != Analyzer->ConstructedObjects.end())
+  Cp = CapabilityExpr(Object->second, StringRef(), false);
+  }
+  const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
+  if (!Fact) {
+Analyzer->Handler.handleMutexNotHeld(Cp.getKind(), D, POK_FunctionCall,
+ Cp.toString(), LK_Exclusive,
+ Exp->getExprLoc());
+continue;
+  }
+  const auto *Scope =
+  cast(Fact);
+  for (const auto &[a, b] :
+   zip_longest(DeclaredLocks, Scope->getUnderlyingMutexes())) {
+if (!a.has_value()) {
+  Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
+  Exp->getExprLoc(), D->getLocation(), Scope->toString(),
+  b.value().getKind(), b.value().toString());
+} else if (!b.has_value()) {
+  Analyzer->Handler.handleExpectMoreUnderlyingMutexes(
+  Exp->getExprLoc(), D->getLocation(), Scope->toString(),
+  a.value().getKind(), a.value().toString());
+} else if (!a.value().matches(b.value())) {

aaronpuchert wrote:

Should we use `equals` instead of `matches`? I don't think we can add wildcards 
to the set of known capabilities, they are only allowed on `guarded_by` or 
`requires` to indicate that any capability is sufficient.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -305,6 +337,16 @@ deadlock can occur if the function acquires the mutex a 
second time.
 mu.Unlock();
   }
 
+  void exclude(MutexLocker& scope LOCKS_EXCLUDED(mu)){

aaronpuchert wrote:

```suggestion
  void exclude(MutexLocker& scope LOCKS_EXCLUDED(mu)) {
```

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -1915,6 +1936,101 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
 }
   }
 
+  std::optional Args;
+  if (Exp) {
+if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else if (const auto *CE = dyn_cast(Exp))
+  Args = CE->arguments();
+else
+  llvm_unreachable("Unknown call kind");
+  }
+  const FunctionDecl *CalledFunction = dyn_cast(D);
+  if (CalledFunction && Args.has_value()) {
+for (auto [Param, Arg] : zip(CalledFunction->parameters(), *Args)) {
+  CapExprSet DeclaredLocks;
+  for (const Attr *At : Param->attrs()) {
+switch (At->getKind()) {
+case attr::AcquireCapability: {
+  const auto *A = cast(At);
+  Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
+  : ExclusiveLocksToAdd,
+A, Exp, D, Self);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::ReleaseCapability: {
+  const auto *A = cast(At);
+  if (A->isGeneric())
+Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, Self);
+  else if (A->isShared())
+Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
+  else
+Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::RequiresCapability: {
+  const auto *A = cast(At);
+  for (auto *Arg : A->args())
+Analyzer->warnIfMutexNotHeld(FSet, D, Exp,
+ A->isShared() ? AK_Read : AK_Written,
+ Arg, POK_FunctionCall, Self, Loc);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+case attr::LocksExcluded: {
+  const auto *A = cast(At);
+  for (auto *Arg : A->args())
+Analyzer->warnIfMutexHeld(FSet, D, Exp, Arg, Self, Loc);
+  Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
+  break;
+}
+
+default:
+  break;
+}
+  }
+  if (DeclaredLocks.size() == 0)

aaronpuchert wrote:

```suggestion
  if (!DeclaredLocks.empty())
```

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

@@ -211,17 +214,35 @@ must be held on entry to the function, *and must still be 
held on exit*.
 mu1.Unlock();
   }
 
+  void require(MutexLocker& scope REQUIRES(mu1)) {
+scope.Unlock();
+a=0; // Warning!  Requires mu1.

aaronpuchert wrote:

Nitpick: spaces around operators.
```suggestion
a = 0; // Warning!  Requires mu1.
```

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

https://github.com/aaronpuchert commented:

One more idea for a test: can a parameter attribute reference another 
parameter? Then we could try something like
```c++
struct ObjectWithMutex { Mutex mu; };
void releaseMember(ObjectWithMutex& object, ReleasableMutexLock& scope 
EXCLUSIVE_UNLOCK_FUNCTION(object.mu));
```
Just to ensure we're parsing the attribute in the right context. My 
understanding is that the attributes can currently not reference `this` due to 
the mentioned parser issue, so we can't test that.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Aaron Puchert via cfe-commits]

aaronpuchert wrote:

@delesley, your input on the conceptual side would also be appreciated as this 
is a substantial addition.

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

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[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 f4042077e2e3946ee35c1df8cab8237de6086480 
f669df245c2661ad502c8f4eca2bc446ebc06606 --extensions cpp,h -- 
clang/include/clang/Analysis/Analyses/ThreadSafety.h 
clang/lib/Analysis/ThreadSafety.cpp clang/lib/Sema/AnalysisBasedWarnings.cpp 
clang/lib/Sema/SemaDeclAttr.cpp 
clang/test/SemaCXX/warn-thread-safety-analysis.cpp 
clang/test/SemaCXX/warn-thread-safety-parsing.cpp
``





View the diff from clang-format here.


``diff
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 591891b07c..0fcf5bed12 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -256,7 +256,8 @@ public:
   virtual void handleExpectFewerUnderlyingMutexes(SourceLocation Loc,
   SourceLocation DLoc,
   Name ScopeName,
-  StringRef Kind, Name Actual) 
{}
+  StringRef Kind, Name Actual) 
{
+  }
 
   /// Warn that L1 cannot be acquired before L2.
   virtual void handleLockAcquiredBefore(StringRef Kind, Name L1Name,
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index f40a4f5f39..9736e1d7bd 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -126,8 +126,8 @@ private:
   SourceLocation AcquireLoc;
 
 public:
-  FactEntry(FactEntryKind FK, const CapabilityExpr &CE, LockKind LK, 
SourceLocation Loc,
-SourceKind Src)
+  FactEntry(FactEntryKind FK, const CapabilityExpr &CE, LockKind LK,
+SourceLocation Loc, SourceKind Src)
   : CapabilityExpr(CE), Kind(FK), LKind(LK), Source(Src), AcquireLoc(Loc) 
{}
   virtual ~FactEntry() = default;
 
@@ -2010,8 +2010,7 @@ void BuildLockset::handleCall(const Expr *Exp, const 
NamedDecl *D,
  Exp->getExprLoc());
 continue;
   }
-  const auto *Scope =
-  cast(Fact);
+  const auto *Scope = cast(Fact);
   for (const auto &[a, b] :
zip_longest(DeclaredLocks, Scope->getUnderlyingMutexes())) {
 if (!a.has_value()) {
@@ -2510,7 +2509,7 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
   }
   if (UnderlyingLocks.empty())
 continue;
-  CapabilityExpr Cp (SxBuilder.createVariable(Param), StringRef(), false);
+  CapabilityExpr Cp(SxBuilder.createVariable(Param), StringRef(), false);
   auto ScopedEntry = std::make_unique(
   Cp, Param->getLocation(), FactEntry::Declared);
   for (const CapabilityExpr &M : UnderlyingLocks)
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp 
b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index d20cf553b6..d131072aea 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1854,7 +1854,8 @@ class ThreadSafetyReporter : public 
clang::threadSafety::ThreadSafetyHandler {
   OptionalNotes makeManagedMismatchNoteForParam(SourceLocation DeclLoc) {
 return DeclLoc.isValid()
? getNotes(PartialDiagnosticAt(
- DeclLoc, 
S.PDiag(diag::note_managed_mismatch_here_for_param)))
+ DeclLoc,
+ S.PDiag(diag::note_managed_mismatch_here_for_param)))
: getNotes();
   }
 
@@ -1890,7 +1891,7 @@ class ThreadSafetyReporter : public 
clang::threadSafety::ThreadSafetyHandler {
 
   void handleExpectMoreUnderlyingMutexes(SourceLocation Loc,
  SourceLocation DLoc, Name scopeName,
- StringRef Kind, 
+ StringRef Kind,
  Name expected) override {
 PartialDiagnosticAt Warning(
 Loc, S.PDiag(diag::warn_expect_more_underlying_mutexes)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 2248354e17..e28f8b0e70 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -431,7 +431,8 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
   }
 }
 
-static bool checkFunParamsAreScopedLockable(Sema &S, const ParmVarDecl 
*ParamDecl,
+static bool checkFunParamsAreScopedLockable(Sema &S,
+const ParmVarDecl *ParamDecl,
 const ParsedAttr &AL) {
   QualType ParamType = ParamDecl->getType();
   if (const auto *RefType = ParamType->getAs())

``




https://github.com/llvm/llvm-project/pull/110523
___
cf

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Malek Ben Slimane (malek203)


Changes

This is helpful when multiple functions operate on the same capabilities, but 
we still want to use scoped lockable types for readability and exception safety.
- Introduce support for thread safety annotations on function parameters marked 
with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring correct usage.
- Enhance the analysis to recognize and handle parameters annotated for thread 
safety, extending the scope of analysis to track these across function 
boundries.
- Verify that the underlying mutexes of function arguments match the 
expectations set by the annotations. 
Limitation: This does not work when the attribute arguments are class members, 
because attributes on function parameters are parsed differently from 
attributes on functions.

---

Patch is 47.05 KiB, truncated to 20.00 KiB below, full version: 
https://github.com/llvm/llvm-project/pull/110523.diff


9 Files Affected:

- (modified) clang/docs/ThreadSafetyAnalysis.rst (+47-4) 
- (modified) clang/include/clang/Analysis/Analyses/ThreadSafety.h (+35) 
- (modified) clang/include/clang/Basic/Attr.td (+4-4) 
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+15) 
- (modified) clang/lib/Analysis/ThreadSafety.cpp (+164-9) 
- (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+39) 
- (modified) clang/lib/Sema/SemaDeclAttr.cpp (+40) 
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+366) 
- (modified) clang/test/SemaCXX/warn-thread-safety-parsing.cpp (+30-12) 


``diff
diff --git a/clang/docs/ThreadSafetyAnalysis.rst 
b/clang/docs/ThreadSafetyAnalysis.rst
index cc4089b97b4923..6c513a52277cee 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -187,10 +187,13 @@ REQUIRES(...), REQUIRES_SHARED(...)
 
 *Previously*: ``EXCLUSIVE_LOCKS_REQUIRED``, ``SHARED_LOCKS_REQUIRED``
 
-``REQUIRES`` is an attribute on functions or methods, which
+``REQUIRES`` is an attribute on functions, methods or function parameters of
+reference to :ref:`scoped_capability`-annotated type, which
 declares that the calling thread must have exclusive access to the given
 capabilities.  More than one capability may be specified.  The capabilities
 must be held on entry to the function, *and must still be held on exit*.
+Additionally, if the attribute is on a function parameter, it declares that
+the scoped capability manages the specified capabilities in the given order.
 
 ``REQUIRES_SHARED`` is similar, but requires only shared access.
 
@@ -211,6 +214,21 @@ must be held on entry to the function, *and must still be 
held on exit*.
 mu1.Unlock();
   }
 
+  void require(MutexLocker& scope REQUIRES(mu1)) {
+scope.Unlock();
+a=0; // Warning!  Requires mu1.
+scope.Lock();
+  }
+
+  void testParameter() {
+MutexLocker scope(&mu1);
+MutexLocker scope2(&mu2);
+require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead of 
'mu1'
+require(scope); // OK.
+scope.Unlock();
+require(scope); // Warning!  Requires mu1.
+  }
+
 
 ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), RELEASE_SHARED(...), 
RELEASE_GENERIC(...)
 
--
@@ -218,10 +236,13 @@ ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), 
RELEASE_SHARED(...), RELEASE_GE
 *Previously*: ``EXCLUSIVE_LOCK_FUNCTION``, ``SHARED_LOCK_FUNCTION``,
 ``UNLOCK_FUNCTION``
 
-``ACQUIRE`` and ``ACQUIRE_SHARED`` are attributes on functions or methods
-declaring that the function acquires a capability, but does not release it.
+``ACQUIRE`` and ``ACQUIRE_SHARED`` are attributes on functions, methods
+or function parameters of reference to :ref:`scoped_capability`-annotated type,
+which declare that the function acquires a capability, but does not release it.
 The given capability must not be held on entry, and will be held on exit
 (exclusively for ``ACQUIRE``, shared for ``ACQUIRE_SHARED``).
+Additionally, if the attribute is on a function parameter, it declares that
+the scoped capability manages the specified capabilities in the given order.
 
 ``RELEASE``, ``RELEASE_SHARED``, and ``RELEASE_GENERIC`` declare that the
 function releases the given capability.  The capability must be held on entry
@@ -249,6 +270,14 @@ shared for ``RELEASE_GENERIC``), and will no longer be 
held on exit.
 myObject.doSomething();  // Warning, mu is not locked.
   }
 
+  void release(MutexLocker& scope RELEASE(mu)) {
+  }  // Warning!  Need to unlock mu.
+
+  void testParameter() {
+MutexLocker scope(&mu);
+release(scope);
+  }
+
 If no argument is passed to ``ACQUIRE`` or ``RELEASE``, then the argument is
 assumed to be ``this``, and the analysis will not check the body of the
 function.  This pattern is intended for use by classes which hide locking
@@ -283,10 +312,13 @@ EXCLU

[clang] Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations (PR #110523)

[Malek Ben Slimane via cfe-commits]

https://github.com/malek203 created 
https://github.com/llvm/llvm-project/pull/110523

This is helpful when multiple functions operate on the same capabilities, but 
we still want to use scoped lockable types for readability and exception safety.
- Introduce support for thread safety annotations on function parameters marked 
with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring correct usage.
- Enhance the analysis to recognize and handle parameters annotated for thread 
safety, extending the scope of analysis to track these across function 
boundries.
- Verify that the underlying mutexes of function arguments match the 
expectations set by the annotations. 
Limitation: This does not work when the attribute arguments are class members, 
because attributes on function parameters are parsed differently from 
attributes on functions.

>From f669df245c2661ad502c8f4eca2bc446ebc06606 Mon Sep 17 00:00:00 2001
From: Malek Ben Slimane 
Date: Wed, 11 Sep 2024 22:07:45 +0200
Subject: [PATCH] Thread Safety Analysis: Support passing scoped locks between
 functions with appropriate annotations

This is helpful when multiple functions operate on the same
capabilities, but we still want to use scoped lockable types for
readability and exception safety.
- Introduce support for thread safety annotations on function parameters
  marked with the 'scoped_lockable' attribute.
- Add semantic checks for annotated function parameters, ensuring
  correct usage.
- Enhance the analysis to recognize and handle parameters annotated for
  thread safety, extending the scope of analysis to track these across
  function boundries.
- Verify that the underlying mutexes of function arguments match the
  expectations set by the annotations.
Limitation: This does not work when the attribute arguments are class
members, because attributes on function parameters are parsed
differently from attributes on functions.
---
 clang/docs/ThreadSafetyAnalysis.rst   |  51 ++-
 .../clang/Analysis/Analyses/ThreadSafety.h|  35 ++
 clang/include/clang/Basic/Attr.td |   8 +-
 .../clang/Basic/DiagnosticSemaKinds.td|  15 +
 clang/lib/Analysis/ThreadSafety.cpp   | 173 -
 clang/lib/Sema/AnalysisBasedWarnings.cpp  |  39 ++
 clang/lib/Sema/SemaDeclAttr.cpp   |  40 ++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 366 ++
 .../SemaCXX/warn-thread-safety-parsing.cpp|  42 +-
 9 files changed, 740 insertions(+), 29 deletions(-)

diff --git a/clang/docs/ThreadSafetyAnalysis.rst 
b/clang/docs/ThreadSafetyAnalysis.rst
index cc4089b97b4923..6c513a52277cee 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -187,10 +187,13 @@ REQUIRES(...), REQUIRES_SHARED(...)
 
 *Previously*: ``EXCLUSIVE_LOCKS_REQUIRED``, ``SHARED_LOCKS_REQUIRED``
 
-``REQUIRES`` is an attribute on functions or methods, which
+``REQUIRES`` is an attribute on functions, methods or function parameters of
+reference to :ref:`scoped_capability`-annotated type, which
 declares that the calling thread must have exclusive access to the given
 capabilities.  More than one capability may be specified.  The capabilities
 must be held on entry to the function, *and must still be held on exit*.
+Additionally, if the attribute is on a function parameter, it declares that
+the scoped capability manages the specified capabilities in the given order.
 
 ``REQUIRES_SHARED`` is similar, but requires only shared access.
 
@@ -211,6 +214,21 @@ must be held on entry to the function, *and must still be 
held on exit*.
 mu1.Unlock();
   }
 
+  void require(MutexLocker& scope REQUIRES(mu1)) {
+scope.Unlock();
+a=0; // Warning!  Requires mu1.
+scope.Lock();
+  }
+
+  void testParameter() {
+MutexLocker scope(&mu1);
+MutexLocker scope2(&mu2);
+require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead of 
'mu1'
+require(scope); // OK.
+scope.Unlock();
+require(scope); // Warning!  Requires mu1.
+  }
+
 
 ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), RELEASE_SHARED(...), 
RELEASE_GENERIC(...)
 
--
@@ -218,10 +236,13 @@ ACQUIRE(...), ACQUIRE_SHARED(...), RELEASE(...), 
RELEASE_SHARED(...), RELEASE_GE
 *Previously*: ``EXCLUSIVE_LOCK_FUNCTION``, ``SHARED_LOCK_FUNCTION``,
 ``UNLOCK_FUNCTION``
 
-``ACQUIRE`` and ``ACQUIRE_SHARED`` are attributes on functions or methods
-declaring that the function acquires a capability, but does not release it.
+``ACQUIRE`` and ``ACQUIRE_SHARED`` are attributes on functions, methods
+or function parameters of reference to :ref:`scoped_capability`-annotated type,
+which declare that the function acquires a capability, but does not release it.
 The given capability must not be held on entry, and will be held on exit
 (exclusively for ``ACQUIRE``, shared for ``ACQUIRE_SHARED``).
+Additionally