[libcxx-commits] [PATCH] D154354: [libc++] Fix thread annotations on shared_mutex and shared_timed_mutex

[Aaron Puchert via Phabricator via libcxx-commits]

aaronpuchert added a comment.

Thanks for picking this up!

In D154354#4468666 <https://reviews.llvm.org/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_* <https://github.com/llvm/llvm-project/tree/main/libcxx/test/libcxx/thread/thread.mutex>. 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();



================
Comment at: libcxx/include/shared_mutex:188
+class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_AVAILABILITY_SHARED_MUTEX
+_LIBCPP_THREAD_SAFETY_ANNOTATION(capability("shared_mutex")) shared_mutex {
+  __shared_mutex_base __base_;
----------------
You could also replace the underscore by a space, or go with just "mutex". This string is used as a "capability kind" in diagnostics.


================
Comment at: libcxx/include/shared_mutex:227
+class _LIBCPP_EXPORTED_FROM_ABI _LIBCPP_AVAILABILITY_SHARED_MUTEX
+_LIBCPP_THREAD_SAFETY_ANNOTATION(capability("shared_mutex")) shared_timed_mutex {
     __shared_mutex_base __base_;
----------------
Otherwise, if you keep the type, it probably makes sense to write `shared_timed_mutex` here.


================
Comment at: libcxx/include/shared_mutex:243
         bool
         try_lock_for(const chrono::duration<_Rep, _Period>& __rel_time)
         {
----------------
Should also have `_LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_capability(true))`: it's a try-acquire, the time parameter is irrelevant for thread safety.


================
Comment at: libcxx/include/shared_mutex:250
         bool
         try_lock_until(const chrono::time_point<_Clock, _Duration>& __abs_time);
+    void unlock() _LIBCPP_THREAD_SAFETY_ANNOTATION(release_capability());
----------------
Same here.


================
Comment at: libcxx/include/shared_mutex:256-266
     template <class _Rep, class _Period>
         _LIBCPP_INLINE_VISIBILITY
         bool
         try_lock_shared_for(const chrono::duration<_Rep, _Period>& __rel_time)
         {
             return try_lock_shared_until(chrono::steady_clock::now() + __rel_time);
         }
----------------
Same for these two, of course `_LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_shared_capability(true))`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154354/new/

https://reviews.llvm.org/D154354