[libcxx] Add clang thread safety annotations to mutex and lock_guard

[jamesr]

Summary

This adds clang thread safety annotations to std::mutex and
std::lock_guard so code using these types can use these types directly
instead of having to wrap the types to provide annotations. These checks
when enabled by -Wthread-safety provide simple but useful static
checking to detect potential race conditions.
See http://clang.llvm.org/docs/ThreadSafetyAnalysis.html for details.

[mclow.lists]

In general, this looks fine. There are a few nits.

1. There needs to be a way to disable these annotations.

The usual way in libc++ is to check to see if _LIBCPP_THREAD_ANNOTATION is already defined, and if so, do not define it. We usually state things in the negative, so the controlling macro should be _LIBCPP_HAS_NO_THREAD_ANNOTATION

2. We need to support non-ToT versions of clang, so the test needs to be more nuanced than #if defined(__clang__). See L432 of <__config> for an example there.

More as I see them

[mclow.lists]

We usually state things in the negative, so the controlling macro should be _LIBCPP_HAS_NO_THREAD_ANNOTATION

Or actually, _LIBCPP_HAS_NO_THREAD_ANNOTATIONS

[mclow.lists]

Where are the tests?

[jamesr]

Updates the macros to avoid defining anything if _LIBCPP_THREAD_ANNOTATION is already defined and to define _LIBCPP_THREAD_ANNOTATION to nothing if clang is not set, __has_attribute(acquire_capability) is not set, or _LIBCPP_HAS_NO_THREAD_ANNOTATIONS is set.

It turns out there's not a clean feature guard for the thread safety annotations (i.e. no __has_feature() guard). The annotations were added over time to clang. I'm not sure at which point the feature was considered stable enough for general use. Looking at clang's history the most recently added annotation out of the ones this patch uses is acquire_capability which was added in this commit: https://llvm.org/svn/llvm-project/cfe/trunk@201890, so I'm testing for that annotation to test for support for the feature as a whole. It's quite possible that there are revisions of clang after the annotation support was added that contain bugs that make the annotations not very useful but the fix for any users trying to use such versions of clang is easy - just avoid passing -Wthread-safety to these versions of clang.

[jamesr]

In D14731#299152, @mclow.lists wrote:

Where are the tests?

There aren't any yet. I investigated a few avenues for testing but none seem very useful.

The most obvious testing strategy would be to write some code that fails the -Wthread-safety checks such as

std::mutex m; m.lock(); m.lock();

and verify that this generates the expected warning. There's a harness for expected warnings that does this in the clang repo but no such harness exists in libcxx. Depending on the text of warnings from clang in libcxx tests seems like a layering violation, anyway. Another check would be to test that such code fails to compile with -Wthread-safety -Werror set. libcxx's test suite does support negative compilation tests but I can't find any way to specify that particular tests should be compiled with particular flags. I tried enabling -Wthread-safety for all tests, but other tests fail because they do things that the annotations do not understand (deliberately, the annotations are for static checking not dynamic code analysis). I could add suppressions to all of these tests but that seems backwards. This also hits the problem that such annotations would only have meaning for new-enough versions of clang and there's no way for the driver to cleanly tell if the cxx being used supports the annotations or not.

The annotations are (by design) not supposed to have an influence on the program other than generating warnings in non-default configs so there isn't any other testing hook I can think of.

[jamesr]

It looks like Aaron Ballman was involved with adding parsing for these attributes in clang - perhaps he has an idea of a better way to detect these than __has_attribute(acquire_capability).

[aaron.ballman]

In D14731#300074, @jamesr wrote:

It looks like Aaron Ballman was involved with adding parsing for these attributes in clang - perhaps he has an idea of a better way to detect these than __has_attribute(acquire_capability).

__has_attribute is a reasonable approach. You are correct that there is (unfortunately) no good feature testing macro for these. The good news is, the attributes are relatively stable these days, and have been for a while.

FWIW, the annotations all LGTM.

~Aaron

[jamesr]

Marshall - friendly ping. If you know a way to test functionality guarded by a compile-time flag in the libcxx test suite I'd be happy to wire in tests for this, but if that doesn't exist (and I think it does not currently) then I think the only way to verify this is by inspection. Let me know what you'd prefer when you get a chance. Thanks!

[EricWF]

I tried to do this over the summer. The author of the annotations insisted that it would be a bad idea to apply them to libc++ since it would break existing code. Should these be off by default?

Also at the time the annotations were unstable and somewhat buggy. Has this situtation cleared up?

Also could we add some ".fail.cpp" tests that use Clang verify? I'm happy to give an example if your unfamiliar with how libc++ handles testing.

[EricWF]

I generated an updated patch with a sample test case (and the LIT changes needed to run it).
https://gist.github.com/EricWF/12f77078e4efc6610572

[jamesr]

Thank you for posting the LIT changes - that's the part I was unable to figure out. I have a number of additional failure test cases that I will add.

I don't know what sort of code this could break, although I certainly don't claim to know more about these annotations than the author. Do you have a link to that discussion handy? The most obvious way this could break existing code would be if existing code was compiling with -Wthread-safety and had its own annotations that were inconsistent with these, but as it's a compile error to declare thread-safety relationships with types that aren't already annotated I can't see how any code that would be broken could compile today. Concretely, if some code is written like this today:

struct Foo {

int a __attribute__((guarded_by(m)));
std::mutex m;

};

void Bad(Foo* f) { f->a++; }

will fail to compile without this patch in libc++ with the error:

/tmp/test.cc:4:26: error: 'guarded_by' attribute requires arguments whose type is annotated with 'capability' attribute; type here is 'std::mutex'

  [-Werror,-Wthread-safety-attributes]
int a __attribute__((guarded_by(m))

[EricWF]

Unfortunately the discussion was offline so it's not available.

Anybody using std::mutex with std::unique_lock or their own lock guards will probably experience some breakage.

Heres an example of code that will now not compile with "-Werror=thread-safety"

std::mutex m;
m.lock();
{
  std::unique_lock<std::mutex> g(m, std::adopt_lock);
}  // -Wthread-safety thinks `m` is still locked

AFAIK we can't fully annotate std::unique_lock either because there is no way to model the deferred locking behavior.

[EricWF]

My suggestion would be to make these annotations OFF by default and allow users to turn them on with a macro.

[jamesr]

Ah, that's true. I didn't think of that case. With the design of these annotations the author of that function would have to disable checks in each piece of code that uses these patterns.

What about adding a different guard for these annotations in libc++ that consumers could choose to toggle independently of the -Wthread-safety compiler option? That way consumers could choose to enable the libc++ annotations if their codebase was compatible with them. The alternative is that consumers have to fully wrap the std:: types in their own types to add the annotations which works but makes code reuse very difficult.

[jamesr]

In D14731#373289, @EricWF wrote:

My suggestion would be to make these annotations OFF by default and allow users to turn them on with a macro.

Our comments crossed streams but suggested the same thing :). Any suggestions on a naming convention for the guard? Also, would I set this macro unconditionally in LIT or is there a way to have it run tests both with and without the macro to ensure that when it is off code like the snippet you posted continues to build unmodified?

[EricWF]

In D14731#373301, @jamesr wrote:

In D14731#373289, @EricWF wrote:

My suggestion would be to make these annotations OFF by default and allow users to turn them on with a macro.

Our comments crossed streams but suggested the same thing :). Any suggestions on a naming convention for the guard? Also, would I set this macro unconditionally in LIT or is there a way to have it run tests both with and without the macro to ensure that when it is off code like the snippet you posted continues to build unmodified?

My suggestion would be _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS but I'm bad at naming things :-P

I think we leave the macro off in LIT because each test can #define it at the top of the file if it want's it on.

[jamesr]

This patch guards the new annotations with _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS and adds a number of tests to check that the annotations produce errors when the annotations are enabled that code violates the thread safety rules, that correct code does not produce errors, and that code that does not enable the annotations does not produce errors even in code that the thread-safety feature in Clang does not understand.

The feature guard is state in the positive as opposed to the negative as the annotations may be incompatible with correct code in some situations.

[EricWF]

test/libcxx/thread/thread.mutex/thread_safety_access_guarded_without_lock.fail.cpp

1 ↗

(On Diff #50481)

This test doesn't really test any libc++ behavior. It would fail in the same way before and after applying your patch. I don't think we need it.

test/libcxx/thread/thread.mutex/thread_safety_annotations_not_enabled.cpp

1 ↗

(On Diff #50481)

'.pass.cpp'

test/libcxx/thread/thread.mutex/thread_safety_call_requires_capability_without_having.fail.cpp

1 ↗

(On Diff #50481)

This test doesn't really test any libc++ behavior. It would fail in the same way before and after applying your patch. I don't think we need it.

test/libcxx/thread/thread.mutex/thread_safety_lock_guard.cpp

1 ↗

(On Diff #50481)

'.pass.cpp' and main().

test/libcxx/thread/thread.mutex/thread_safety_lock_unlock.cpp

1 ↗

(On Diff #50481)

'.pass.cpp' and add another 'main()' definition.

test/libcxx/thread/thread.mutex/thread_safety_requires_capability.cpp

1 ↗

(On Diff #50481)

Passing tests need to have the suffix ".pass.cpp" in order for the test suite to pick them up.
Also this function will still need a definition for main in order to pass with GCC and other configurations.

[jamesr]

Thank you for the comments, new patch coming.. (is there a way to post comments and upload a new patch at the same time?)

[EricWF]

So I fixed up LIT so that it also adds "-Werror=thread-safety" for both ".pass.cpp" and ".fail.cpp" tests. Could you apply it to your patch?
https://gist.github.com/EricWF/8a0bfb6ff02f8c9f9940

include/__mutex_base

37

Does capability have a alternative keyword that's a reserved name? ie __capability__? If so we should use that instead.

[jamesr]

In D14731#376128, @EricWF wrote:

So I fixed up LIT so that it also adds "-Werror=thread-safety" for both ".pass.cpp" and ".fail.cpp" tests. Could you apply it to your patch?
https://gist.github.com/EricWF/8a0bfb6ff02f8c9f9940

Cool! Applied and confirmed that it's set for both.

include/__mutex_base

37

If I'm reading https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/Attr.td#L1657 correctly:

def Capability : InheritableAttr {

let Spellings = [GNU<"capability">, CXX11<"clang", "capability">,
                 GNU<"shared_capability">,
                 CXX11<"clang", "shared_capability">];

then the answer is unfortunately "no"

[EricWF]

So this LGTM except for one last change (I'm sorry). LIT now knows when "-Werror=thread-safety" is being passed. Could you change the tests guarded by "#if !defined(clang) || !__has_attribute(aquire_capability)` to say "// REQUIRES: thread-safety" instead? I prefer using LIT to manage when tests run because it can report un-run tests where simply using macros cant.

After that LGTM.

include/__mutex_base

37

I appreciate the super thorough answer!

[jamesr]

In D14731#376138, @EricWF wrote:

So this LGTM except for one last change (I'm sorry). LIT now knows when "-Werror=thread-safety" is being passed. Could you change the tests guarded by "#if !defined(clang) || !__has_attribute(aquire_capability)` to say "// REQUIRES: thread-safety" instead? I prefer using LIT to manage when tests run because it can report un-run tests where simply using macros cant.

After that LGTM.

If I do that, does that mean I can omit the extra main()s, etc, and the LIT harness will avoid attempting the test at all when the feature bit is set?

[EricWF]

Yes exactly. // REQUIRES: <feature> tells LIT to skip the test (and report it as UNSUPPORTED) whenever the feature is not present.

[jamesr]

Use // REQUIRES: thread-safety instead of macros to feature guard tests

[EricWF]

Oh fudge. One last change. The tests need license headers. Just copy it from an existing test. LGTM after that.

Thanks again.

[jamesr]

Add LLVM license headers to new test files

[EricWF]

Committed in r263611.

include/__config

 #if __cplusplus < 201103L
 #define _LIBCPP_CXX03_LANG
 #else
 #if defined(_LIBCPP_HAS_NO_VARIADIC_TEMPLATES) || defined(_LIBCPP_HAS_NO_RVALUE_REFERENCES)
 #error Libc++ requires a feature complete C++11 compiler in C++11 or greater.
 #endif
 #endif
 
+#if (defined(_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS) && defined(__clang__) \
+      && __has_attribute(acquire_capability))
+#define _LIBCPP_HAS_THREAD_SAFETY_ANNOTATIONS
+#endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP_CONFIG

include/__mutex_base

 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #ifndef _LIBCPP_HAS_NO_THREADS
 
-class _LIBCPP_TYPE_VIS mutex
+#ifndef _LIBCPP_THREAD_SAFETY_ANNOTATION
+#  ifdef _LIBCPP_HAS_THREAD_SAFETY_ANNOTATIONS
+#    define _LIBCPP_THREAD_SAFETY_ANNOTATION(x) __attribute__((x))
+#  else
+#    define _LIBCPP_THREAD_SAFETY_ANNOTATION(x)
+#  endif
+#endif  // _LIBCPP_THREAD_SAFETY_ANNOTATION
+
+class _LIBCPP_TYPE_VIS _LIBCPP_THREAD_SAFETY_ANNOTATION(capability("mutex")) mutex
 {
     pthread_mutex_t __m_;
 
 public:
     _LIBCPP_INLINE_VISIBILITY
 #ifndef _LIBCPP_HAS_NO_CONSTEXPR
      constexpr mutex() _NOEXCEPT : __m_(PTHREAD_MUTEX_INITIALIZER) {}
 #else
      mutex() _NOEXCEPT {__m_ = (pthread_mutex_t)PTHREAD_MUTEX_INITIALIZER;}
 #endif
      ~mutex();
 
 private:
     mutex(const mutex&);// = delete;
     mutex& operator=(const mutex&);// = delete;
 
 public:
-    void lock();
-    bool try_lock() _NOEXCEPT;
-    void unlock() _NOEXCEPT;
+    void lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(acquire_capability());
+    bool try_lock() _NOEXCEPT _LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_capability(true));
+    void unlock() _NOEXCEPT _LIBCPP_THREAD_SAFETY_ANNOTATION(release_capability());
 
     typedef pthread_mutex_t* native_handle_type;
     _LIBCPP_INLINE_VISIBILITY native_handle_type native_handle() {return &__m_;}
 };
 
 struct _LIBCPP_TYPE_VIS defer_lock_t {};
 struct _LIBCPP_TYPE_VIS try_to_lock_t {};
 struct _LIBCPP_TYPE_VIS adopt_lock_t {};
 
 #if defined(_LIBCPP_HAS_NO_CONSTEXPR) || defined(_LIBCPP_BUILDING_MUTEX)
 
 extern const defer_lock_t  defer_lock;
 extern const try_to_lock_t try_to_lock;
 extern const adopt_lock_t  adopt_lock;
 
 #else
 
 constexpr defer_lock_t  defer_lock  = defer_lock_t();
 constexpr try_to_lock_t try_to_lock = try_to_lock_t();
 constexpr adopt_lock_t  adopt_lock  = adopt_lock_t();
 
 #endif
 
 template <class _Mutex>
-class _LIBCPP_TYPE_VIS_ONLY lock_guard
+class _LIBCPP_TYPE_VIS_ONLY _LIBCPP_THREAD_SAFETY_ANNOTATION(scoped_lockable) lock_guard
 {
 public:
     typedef _Mutex mutex_type;
 
 private:
     mutex_type& __m_;
 public:
 
     _LIBCPP_INLINE_VISIBILITY
-    explicit lock_guard(mutex_type& __m)
+    explicit lock_guard(mutex_type& __m) _LIBCPP_THREAD_SAFETY_ANNOTATION(acquire_capability(__m))
         : __m_(__m) {__m_.lock();}
     _LIBCPP_INLINE_VISIBILITY
-    lock_guard(mutex_type& __m, adopt_lock_t)
+    lock_guard(mutex_type& __m, adopt_lock_t) _LIBCPP_THREAD_SAFETY_ANNOTATION(requires_capability(__m))
         : __m_(__m) {}
     _LIBCPP_INLINE_VISIBILITY
-    ~lock_guard() {__m_.unlock();}
+    ~lock_guard() _LIBCPP_THREAD_SAFETY_ANNOTATION(release_capability()) {__m_.unlock();}
 
 private:
     lock_guard(lock_guard const&);// = delete;
     lock_guard& operator=(lock_guard const&);// = delete;
 };
 
 template <class _Mutex>
 class _LIBCPP_TYPE_VIS_ONLY unique_lock

test/libcxx/test/config.py

def configure(self):
         self.configure_target_info()
         self.configure_cxx()
         self.configure_triple()
         self.configure_src_root()
         self.configure_obj_root()
         self.configure_cxx_library_root()
         self.configure_use_system_cxx_lib()
         self.configure_use_clang_verify()
+        self.configure_use_thread_safety()
         self.configure_execute_external()
         self.configure_ccache()
         self.configure_compile_flags()
         self.configure_link_flags()
         self.configure_env()
         self.configure_color_diagnostics()
         self.configure_debug_mode()
         self.configure_warnings()
def configure_use_clang_verify(self):
         if self.use_clang_verify is None:
             # NOTE: We do not test for the -verify flag directly because
             #   -verify will always exit with non-zero on an empty file.
             self.use_clang_verify = self.cxx.hasCompileFlag(
                 ['-Xclang', '-verify-ignore-unexpected'])
             self.lit_config.note(
                 "inferred use_clang_verify as: %r" % self.use_clang_verify)
 
+    def configure_use_thread_safety(self):
+        '''If set, run clang with -verify on failing tests.'''
+        has_thread_safety = self.cxx.hasCompileFlag('-Werror=thread-safety')
+        if has_thread_safety:
+            self.cxx.compile_flags += ['-Werror=thread-safety']
+            self.config.available_features.add('thread-safety')
+            self.lit_config.note("enabling thread-safety annotations")
+
     def configure_execute_external(self):
         # Choose between lit's internal shell pipeline runner and a real shell.
         # If LIT_USE_INTERNAL_SHELL is in the environment, we use that as the
         # default value. Otherwise we ask the target_info.
         use_lit_shell_default = os.environ.get('LIT_USE_INTERNAL_SHELL')
         if use_lit_shell_default is not None:
             use_lit_shell_default = use_lit_shell_default != '0'
         else:

test/libcxx/thread/thread.mutex/thread_safety_annotations_not_enabled.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// <mutex>
+
+// This test does not define _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS so it
+// should compile without any warnings or errors even though this pattern is not
+// understood by the thread safety annotations.
+
+#include <mutex>
+
+int main() {
+  std::mutex m;
+  m.lock();
+  {
+    std::unique_lock<std::mutex> g(m, std::adopt_lock);
+  }
+}

test/libcxx/thread/thread.mutex/thread_safety_lock_guard.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: thread-safety
+
+// <mutex>
+
+#define _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS
+
+#include <mutex>
+
+std::mutex m;
+int foo __attribute__((guarded_by(m)));
+
+int main() {
+  std::lock_guard<std::mutex> lock(m);
+  foo++;
+}

test/libcxx/thread/thread.mutex/thread_safety_lock_unlock.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: thread-safety
+
+// <mutex>
+
+#define _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS
+
+#include <mutex>
+
+std::mutex m;
+int foo __attribute__((guarded_by(m)));
+
+int main() {
+  m.lock();
+  foo++;
+  m.unlock();
+}

test/libcxx/thread/thread.mutex/thread_safety_missing_unlock.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: thread-safety
+
+// <mutex>
+
+#define _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS
+
+#include <mutex>
+
+std::mutex m;
+
+int main() {
+  m.lock();
+} // expected-error {{mutex 'm' is still held at the end of function}}

test/libcxx/thread/thread.mutex/thread_safety_requires_capability.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: thread-safety
+
+// <mutex>
+
+#define _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS
+
+#include <mutex>
+
+std::mutex m;
+int foo __attribute__((guarded_by(m)));
+
+void increment() __attribute__((requires_capability(m))) {
+  foo++;
+}
+
+int main() {
+  m.lock();
+  increment();
+  m.unlock();
+}