[BUG] thread annotation problem for mutex when using libc++ and enabled libc++'s own thread annotation

[zou000]

Describe the bug We are using clang 8 with libc++ with libc++'s thread annotation enabled by -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS globally. However, it conflicts with benchmark's mutex wrapper's thread annotation.

System Which OS, compiler, and compiler version are you using:

• OS: Linux
• Compiler and version: clang 8 with libc++

To reproduce Steps to reproduce the behavior:

1. sync to commit 8e0b1913d4ea803dfeb2e55567208fcab6b1b6c7

export CC=clang 
bazel build --copt '-stdlib=libc++' --copt -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS :benchmark

4. See error

src/mutex.h:79:40: warning: mutex 'mut_' is still held at the end of function [-Wthread-safety-analysis]
  void lock() ACQUIRE() { mut_.lock(); }
                                       ^
src/mutex.h:79:32: note: mutex acquired here
  void lock() ACQUIRE() { mut_.lock(); }
                               ^
src/mutex.h:80:34: warning: releasing mutex 'mut_' that was not held [-Wthread-safety-analysis]
  void unlock() RELEASE() { mut_.unlock(); }
                                 ^

Expected behavior Should compile without warning

[dmah42]

The issue is in the definition of the thread attribute here where we're missing exclusive_lock_function.

adding this attribute and using it (along with unlock_function for unlock) would solve the issue.

now if we were using absl we'd have thread_annotations.h and mutex which has these defined...

[zou000]

now if we were using absl we'd have thread_annotations.h and mutex which has these defined...

Right, another benefit of using absl::Mutex would be that the explicit notify_all calls like this can be replace by lock_.Await(...)

[LebedevRI]

The issue is in the definition of the thread attribute here where we're missing exclusive_lock_function.

adding this attribute and using it (along with unlock_function for unlock) would solve the issue.

now if we were using absl we'd have thread_annotations.h and mutex which has these defined...

I don't see how that follows. From https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

// Replaced by ACQUIRE
#define EXCLUSIVE_LOCK_FUNCTION(...) \
  THREAD_ANNOTATION_ATTRIBUTE__(exclusive_lock_function(__VA_ARGS__))

// Replaced by RELEASE and RELEASE_SHARED
#define UNLOCK_FUNCTION(...) \
  THREAD_ANNOTATION_ATTRIBUTE__(unlock_function(__VA_ARGS__))

replaced==deprecated, so if abseil if still using those, it's their bug, not a feature.

Also, benchmark::Mutex is already annotated with ACQUIRE()/RELEASE(), so this is something else..

[pleroy]

Isn't it simply that the macros are missing their mutex argument, e.g., ACQUIRE(mut_)? As written I can't imagine how the analyzer could figure out what happens to mut_.

[LebedevRI]

I'm trying that locally now, and no that doesn't seem to help

[dmah42]

from that same doc: "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 details behind an abstract interface."

[cottsay]

I've encountered this as well.

I believe that unrelated changes have caused the HAVE_THREAD_SAFETY_ATTRIBUTES check to fail during configuration, so -DHAVE_THREAD_SAFETY_ATTRIBUTES isn't set and the annotations aren't actually added. If I manually add -DHAVE_THREAD_SAFETY_ATTRIBUTES to my CMAKE_CXX_FLAGS, the warnings go away.

Here's the link error in the HAVE_THREAD_SAFETY_ATTRIBUTES check:

/usr/bin/ld: CMakeFiles/cmTC_2984c.dir/thread_safety_attributes.cpp.o: in function `__cxx_global_var_init':
thread_safety_attributes.cpp:(.text.startup+0x5): undefined reference to `benchmark::internal::InitializeStreams()'

---

UPDATE: This isn't a recommendation per se, but this change worked around the link error and with the check succeeding, fixed the problem for me

--- a/cmake/thread_safety_attributes.cpp        2020-07-15 17:35:35.557813957 -0700
+++ b/cmake/thread_safety_attributes.cpp        2020-07-15 17:36:19.040227189 -0700
@@ -1,4 +1,12 @@
 #define HAVE_THREAD_SAFETY_ATTRIBUTES
 #include "../src/mutex.h"

+namespace benchmark
+{
+namespace internal
+{
+int InitializeStreams() { return 0; }
+}
+}
+
 int main() {}

[dmah42]

the InitializeStreams issue is a separate issue, i think.