[libc++] Fix thread annotations on shared_mutex and shared_timed_mutex

[ldionne]

Summary

Based on the comment in https://reviews.llvm.org/D54290#4418958, these
attributes need to be on the top-level functions in order to work
properly. Also, add tests.

Fixes http://llvm.org/PR57035.

[philnik]

Could we add some sort of tests for this?

[aaronpuchert]

Thanks for picking this up!

In D154354#4468666, @philnik wrote:

Could we add some sort of tests for this?

There are several existing tests, see libcxx/test/libcxx/thread/thread.mutex/thread_safety_*. What should work:

std::shared_mutex m;
int data __attribute__((guarded_by(m)));
void read(int);

++foo; // warning, but probably no need to test this: libc++ has
       // nothing to do with it, it's due to the attribute above.
m.lock();
++foo; // ok.
m.unlock();

if (m.try_lock())
  ++foo; // ok.
m.unlock();

m.lock_shared();
read(foo); // ok.
++foo; // warning.
m.unlock_shared();

libcxx/include/shared_mutex

187

You could also replace the underscore by a space, or go with just "mutex". This string is used as a "capability kind" in diagnostics.

229

Otherwise, if you keep the type, it probably makes sense to write shared_timed_mutex here.

235–236

Should also have _LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_capability(true)): it's a try-acquire, the time parameter is irrelevant for thread safety.

235–236

Same here.

249–250

Same for these two, of course _LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_shared_capability(true)).

[aaronpuchert]

Side note: you can include in the commit message that this fixes #57035.

[philnik]

In D154354#4469777, @aaronpuchert wrote:

Thanks for picking this up!

In D154354#4468666, @philnik wrote:

Could we add some sort of tests for this?

There are several existing tests, see libcxx/test/libcxx/thread/thread.mutex/thread_safety_*. What should work:

std::shared_mutex m;
int data __attribute__((guarded_by(m)));
void read(int);

++foo; // warning, but probably no need to test this: libc++ has
       // nothing to do with it, it's due to the attribute above.
m.lock();
++foo; // ok.
m.unlock();

if (m.try_lock())
  ++foo; // ok.
m.unlock();

m.lock_shared();
read(foo); // ok.
++foo; // warning.
m.unlock_shared();

Then I definitely want some tests for this.

libcxx/include/shared_mutex

197–206

As a fly-by we might as well _Uglify the attributes.

[aaronpuchert]

In D154354#4469777, @aaronpuchert wrote:

if (m.try_lock())
  ++foo; // ok.
m.unlock();

That would generate a warning, better:

if (m.try_lock()) {
  ++foo; // ok.
  m.unlock();
}

And there is no point in testing for that warning, because we're doing this in Clang. We should essentially only test that we got the right attribute.

[ldionne]

Thanks for the comments!

[ldionne]

Address comments.

[ldionne]

Improve test synopsis.

[aaronpuchert]

Annotations and tests look good to me. Guess the tests require C++14/17, respectively, and need to guarded accordingly.

[ldionne]

Update UNSUPPORTED markup for added tests

[philnik]

LGTM with green CI.

[ldionne]

Add markup to the tests

libcxx/.clang-format

AttributeMacros: [
                   '_LIBCPP_NOALIAS',
                   '_LIBCPP_NODISCARD_EXT',
                   '_LIBCPP_NODISCARD',
                   '_LIBCPP_NORETURN',
                   '_LIBCPP_OVERRIDABLE_FUNC_VIS',
                   '_LIBCPP_STANDALONE_DEBUG',
                   '_LIBCPP_TEMPLATE_DATA_VIS',
                   '_LIBCPP_TEMPLATE_VIS',
+                  '_LIBCPP_THREAD_SAFETY_ANNOTATION',
                   '_LIBCPP_USING_IF_EXISTS',
                   '_LIBCPP_WEAK',
                  ]
 BinPackArguments: false
 BinPackParameters: false
 BreakBeforeConceptDeclarations: true
 BreakInheritanceList: BeforeColon
 EmptyLineAfterAccessModifier: Never

libcxx/include/shared_mutex

 #  endif
 
 #  ifdef _LIBCPP_HAS_NO_THREADS
 #    error "<shared_mutex> is not supported since libc++ has been configured without support for threads."
 #  endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-struct _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_AVAILABILITY_SHARED_MUTEX
-_LIBCPP_THREAD_SAFETY_ANNOTATION(capability("shared_mutex")) __shared_mutex_base {
+struct _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_AVAILABILITY_SHARED_MUTEX __shared_mutex_base {
   mutex __mut_;
   condition_variable __gate1_;
   condition_variable __gate2_;
   unsigned __state_;
 
   static const unsigned __write_entered_ = 1U << (sizeof(unsigned) * __CHAR_BIT__ - 1);
   static const unsigned __n_readers_     = ~__write_entered_;
 
   __shared_mutex_base();
   _LIBCPP_HIDE_FROM_ABI ~__shared_mutex_base() = default;
 
   __shared_mutex_base(const __shared_mutex_base&)            = delete;
   __shared_mutex_base& operator=(const __shared_mutex_base&) = delete;
 
   // Exclusive ownership
-  void lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(acquire_capability()); // blocking
-  bool try_lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_capability(true));
-  void unlock() _LIBCPP_THREAD_SAFETY_ANNOTATION(release_capability());
+  void lock(); // blocking
+  bool try_lock();
+  void unlock();
 
   // Shared ownership
-  void lock_shared() _LIBCPP_THREAD_SAFETY_ANNOTATION(acquire_shared_capability()); // blocking
-  bool try_lock_shared() _LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_shared_capability(true));
-  void unlock_shared() _LIBCPP_THREAD_SAFETY_ANNOTATION(release_shared_capability());
+  void lock_shared(); // blocking
+  bool try_lock_shared();
+  void unlock_shared();
 
   //     typedef implementation-defined native_handle_type; // See 30.2.3
   //     native_handle_type native_handle(); // See 30.2.3
 };
 
 #  if _LIBCPP_STD_VER >= 17
-class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_AVAILABILITY_SHARED_MUTEX shared_mutex {
+class _LIBCPP_EXPORTED_FROM_ABI
+    _LIBCPP_AVAILABILITY_SHARED_MUTEX _LIBCPP_THREAD_SAFETY_ANNOTATION(__capability__("shared_mutex")) shared_mutex {
   __shared_mutex_base __base_;
 
 public:
   _LIBCPP_HIDE_FROM_ABI shared_mutex() : __base_() {}
   _LIBCPP_HIDE_FROM_ABI ~shared_mutex() = default;
 
   shared_mutex(const shared_mutex&)            = delete;
   shared_mutex& operator=(const shared_mutex&) = delete;
 
   // Exclusive ownership
-  _LIBCPP_HIDE_FROM_ABI void lock() { return __base_.lock(); }
-  _LIBCPP_HIDE_FROM_ABI bool try_lock() { return __base_.try_lock(); }
-  _LIBCPP_HIDE_FROM_ABI void unlock() { return __base_.unlock(); }
+  _LIBCPP_HIDE_FROM_ABI void lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(__acquire_capability__()) {
+    return __base_.lock();
+  }
+  _LIBCPP_HIDE_FROM_ABI bool try_lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(__try_acquire_capability__(true)) {
+    return __base_.try_lock();
+  }
+  _LIBCPP_HIDE_FROM_ABI void unlock() _LIBCPP_THREAD_SAFETY_ANNOTATION(__release_capability__()) {
+    return __base_.unlock();
+  }
 
   // Shared ownership
-  _LIBCPP_HIDE_FROM_ABI void lock_shared() { return __base_.lock_shared(); }
-  _LIBCPP_HIDE_FROM_ABI bool try_lock_shared() { return __base_.try_lock_shared(); }
-  _LIBCPP_HIDE_FROM_ABI void unlock_shared() { return __base_.unlock_shared(); }
+  _LIBCPP_HIDE_FROM_ABI void lock_shared() _LIBCPP_THREAD_SAFETY_ANNOTATION(__acquire_shared_capability__()) {
+    return __base_.lock_shared();
+  }
+  _LIBCPP_HIDE_FROM_ABI bool try_lock_shared()
+      _LIBCPP_THREAD_SAFETY_ANNOTATION(__try_acquire_shared_capability__(true)) {
+    return __base_.try_lock_shared();
+  }
+  _LIBCPP_HIDE_FROM_ABI void unlock_shared() _LIBCPP_THREAD_SAFETY_ANNOTATION(__release_shared_capability__()) {
+    return __base_.unlock_shared();
+  }
 
   //     typedef __shared_mutex_base::native_handle_type native_handle_type;
   //     _LIBCPP_HIDE_FROM_ABI native_handle_type native_handle() { return __base::unlock_shared(); }
 };
 #  endif
 
-class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_AVAILABILITY_SHARED_MUTEX shared_timed_mutex {
+class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_AVAILABILITY_SHARED_MUTEX _LIBCPP_THREAD_SAFETY_ANNOTATION(
+    __capability__("shared_timed_mutex")) shared_timed_mutex {
   __shared_mutex_base __base_;
 
 public:
   shared_timed_mutex();
   _LIBCPP_HIDE_FROM_ABI ~shared_timed_mutex() = default;
 
   shared_timed_mutex(const shared_timed_mutex&)            = delete;
   shared_timed_mutex& operator=(const shared_timed_mutex&) = delete;
 
   // Exclusive ownership
-  void lock();
-  bool try_lock();
+  void lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(__acquire_capability__());
+  bool try_lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(__try_acquire_capability__(true));
   template <class _Rep, class _Period>
-  _LIBCPP_HIDE_FROM_ABI bool try_lock_for(const chrono::duration<_Rep, _Period>& __rel_time) {
+  _LIBCPP_HIDE_FROM_ABI bool try_lock_for(const chrono::duration<_Rep, _Period>& __rel_time)
+      _LIBCPP_THREAD_SAFETY_ANNOTATION(__try_acquire_capability__(true)) {
     return try_lock_until(chrono::steady_clock::now() + __rel_time);
   }
   template <class _Clock, class _Duration>
   _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS bool
-  try_lock_until(const chrono::time_point<_Clock, _Duration>& __abs_time);
-  void unlock();
+  try_lock_until(const chrono::time_point<_Clock, _Duration>& __abs_time)
+      _LIBCPP_THREAD_SAFETY_ANNOTATION(__try_acquire_capability__(true));
+  void unlock() _LIBCPP_THREAD_SAFETY_ANNOTATION(__release_capability__());
 
   // Shared ownership
-  void lock_shared();
-  bool try_lock_shared();
+  void lock_shared() _LIBCPP_THREAD_SAFETY_ANNOTATION(__acquire_shared_capability__());
+  bool try_lock_shared() _LIBCPP_THREAD_SAFETY_ANNOTATION(__try_acquire_shared_capability__(true));
   template <class _Rep, class _Period>
-  _LIBCPP_HIDE_FROM_ABI bool try_lock_shared_for(const chrono::duration<_Rep, _Period>& __rel_time) {
+  _LIBCPP_HIDE_FROM_ABI bool try_lock_shared_for(const chrono::duration<_Rep, _Period>& __rel_time)
+      _LIBCPP_THREAD_SAFETY_ANNOTATION(__try_acquire_shared_capability__(true)) {
     return try_lock_shared_until(chrono::steady_clock::now() + __rel_time);
   }
   template <class _Clock, class _Duration>
   _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS bool
-  try_lock_shared_until(const chrono::time_point<_Clock, _Duration>& __abs_time);
-  void unlock_shared();
+  try_lock_shared_until(const chrono::time_point<_Clock, _Duration>& __abs_time)
+      _LIBCPP_THREAD_SAFETY_ANNOTATION(__try_acquire_shared_capability__(true));
+  void unlock_shared() _LIBCPP_THREAD_SAFETY_ANNOTATION(__release_shared_capability__());
 };
 
 template <class _Clock, class _Duration>
 bool shared_timed_mutex::try_lock_until(const chrono::time_point<_Clock, _Duration>& __abs_time) {
   unique_lock<mutex> __lk(__base_.__mut_);
   if (__base_.__state_ & __base_.__write_entered_) {
     while (true) {
       cv_status __status = __base_.__gate1_.wait_until(__lk, __abs_time);

libcxx/test/libcxx/thread/thread.shared_mutex/thread_safety.verify.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14
+// UNSUPPORTED: no-threads
+// UNSUPPORTED: availability-shared_mutex-missing
+// REQUIRES: thread-safety
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS
+
+// On Windows Clang bugs out when both __declspec and __attribute__ are present,
+// the processing goes awry preventing the definition of the types.
+// XFAIL: msvc
+
+// <shared_mutex>
+//
+// class shared_mutex;
+//
+// void lock();
+// bool try_lock();
+// void unlock();
+//
+// void lock_shared();
+// bool try_lock_shared();
+// void unlock_shared();
+
+#include <shared_mutex>
+
+std::shared_mutex m;
+int data __attribute__((guarded_by(m))) = 0;
+void read(int);
+
+void f() {
+  // Exclusive locking
+  {
+    m.lock();
+    ++data; // ok
+    m.unlock();
+  }
+  {
+    if (m.try_lock()) {
+      ++data; // ok
+      m.unlock();
+    }
+  }
+
+  // Shared locking
+  {
+    m.lock_shared();
+    read(data); // ok
+    ++data; // expected-error {{writing variable 'data' requires holding shared_mutex 'm' exclusively}}
+    m.unlock_shared();
+  }
+  {
+    if (m.try_lock_shared()) {
+      read(data); // ok
+      ++data; // expected-error {{writing variable 'data' requires holding shared_mutex 'm' exclusively}}
+      m.unlock_shared();
+    }
+  }
+}

libcxx/test/libcxx/thread/thread.shared_timed_mutex/thread_safety.verify.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11
+// UNSUPPORTED: no-threads
+// UNSUPPORTED: availability-shared_mutex-missing
+// REQUIRES: thread-safety
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS
+
+// On Windows Clang bugs out when both __declspec and __attribute__ are present,
+// the processing goes awry preventing the definition of the types.
+// XFAIL: msvc
+
+// <shared_mutex>
+//
+// class shared_timed_mutex;
+//
+// void lock();
+// bool try_lock();
+// bool try_lock_for(const std::chrono::duration<Rep, Period>&);
+// bool try_lock_until(const std::chrono::time_point<Clock, Duration>&);
+// void unlock();
+//
+// void lock_shared();
+// bool try_lock_shared();
+// bool try_lock_shared_for(const std::chrono::duration<Rep, Period>&);
+// bool try_lock_shared_until(const std::chrono::time_point<Clock, Duration>&);
+// void unlock_shared();
+
+#include <chrono>
+#include <shared_mutex>
+
+std::shared_timed_mutex m;
+int data __attribute__((guarded_by(m))) = 0;
+void read(int);
+
+void f(std::chrono::time_point<std::chrono::steady_clock> tp, std::chrono::milliseconds d) {
+  // Exclusive locking
+  {
+    m.lock();
+    ++data; // ok
+    m.unlock();
+  }
+  {
+    if (m.try_lock()) {
+      ++data; // ok
+      m.unlock();
+    }
+  }
+  {
+    if (m.try_lock_for(d)) {
+      ++data; // ok
+      m.unlock();
+    }
+  }
+  {
+    if (m.try_lock_until(tp)) {
+      ++data; // ok
+      m.unlock();
+    }
+  }
+
+  // Shared locking
+  {
+    m.lock_shared();
+    read(data); // ok
+    ++data; // expected-error {{writing variable 'data' requires holding shared_timed_mutex 'm' exclusively}}
+    m.unlock_shared();
+  }
+  {
+    if (m.try_lock_shared()) {
+      read(data); // ok
+      ++data; // expected-error {{writing variable 'data' requires holding shared_timed_mutex 'm' exclusively}}
+      m.unlock_shared();
+    }
+  }
+  {
+    if (m.try_lock_shared_for(d)) {
+      read(data); // ok
+      ++data; // expected-error {{writing variable 'data' requires holding shared_timed_mutex 'm' exclusively}}
+      m.unlock_shared();
+    }
+  }
+  {
+    if (m.try_lock_shared_until(tp)) {
+      read(data); // ok
+      ++data; // expected-error {{writing variable 'data' requires holding shared_timed_mutex 'm' exclusively}}
+      m.unlock_shared();
+    }
+  }
+}