Disable -Wthread-safety-analysis for some functions in __thread_support

[dim]

Summary

Many thread-related libc++ test cases fail on FreeBSD, due to the following -Werror warnings:

In file included from /share/dim/src/llvm/trunk/projects/libcxx/test/std/thread/thread.threads/thread.thread.this/sleep_until.pass.cpp:17:
In file included from /share/dim/src/llvm/trunk/projects/libcxx/include/thread:97:
In file included from /share/dim/src/llvm/trunk/projects/libcxx/include/__mutex_base:17:
/share/dim/src/llvm/trunk/projects/libcxx/include/__threading_support:222:1: error: mutex '__m' is still held at the end of function [-Werror,-Wthread-safety-analysis]
}
^
/share/dim/src/llvm/trunk/projects/libcxx/include/__threading_support:221:10: note: mutex acquired here
  return pthread_mutex_lock(__m);
         ^
/share/dim/src/llvm/trunk/projects/libcxx/include/__threading_support:231:10: error: releasing mutex '__m' that was not held [-Werror,-Wthread-safety-analysis]
  return pthread_mutex_unlock(__m);
         ^
/share/dim/src/llvm/trunk/projects/libcxx/include/__threading_support:242:1: error: mutex '__m' is still held at the end of function [-Werror,-Wthread-safety-analysis]
}
^
/share/dim/src/llvm/trunk/projects/libcxx/include/__threading_support:241:10: note: mutex acquired here
  return pthread_mutex_lock(__m);
         ^
/share/dim/src/llvm/trunk/projects/libcxx/include/__threading_support:251:10: error: releasing mutex '__m' that was not held [-Werror,-Wthread-safety-analysis]
  return pthread_mutex_unlock(__m);
         ^
/share/dim/src/llvm/trunk/projects/libcxx/include/__threading_support:272:10: error: calling function 'pthread_cond_wait' requires holding mutex '__m' exclusively [-Werror,-Wthread-safety-analysis]
  return pthread_cond_wait(__cv, __m);
         ^
/share/dim/src/llvm/trunk/projects/libcxx/include/__threading_support:278:10: error: calling function 'pthread_cond_timedwait' requires holding mutex '__m' exclusively [-Werror,-Wthread-safety-analysis]
  return pthread_cond_timedwait(__cv, __m, __ts);
         ^
6 errors generated.

Obviously, these warnings are false, since the functions are exactly doing what they are supposed to do.

Therefore, add pragmas to ignored -Wthread-safety-analysis warnings. These could also be set for the whole block of thread-related functions, but in this version I have added them per function.

I also considered doing this with macros, as Joerg suggested, but macros that expand to multi-line pragma statements are rather unwieldy, and don't make it much nicer, in my opinion. Suggestions welcome, of course.

[aaron.ballman]

Alternatively, these functions could be given the proper thread safety annotations, couldn't they?

[dim]

In D28520#641534, @aaron.ballman wrote:

Alternatively, these functions could be given the proper thread safety annotations, couldn't they?

Aha, I was not aware of the existence of these attributes. I'll take a look.

[aaron.ballman]

In D28520#641536, @dim wrote:

In D28520#641534, @aaron.ballman wrote:

Alternatively, these functions could be given the proper thread safety annotations, couldn't they?

Aha, I was not aware of the existence of these attributes. I'll take a look.

This may be of some help: http://clang.llvm.org/docs/ThreadSafetyAnalysis.html

[EricWF]

Also look in __mutex where libc++ defines its own macros for the annotations.

[dim]

In D28520#641563, @EricWF wrote:

Also look in __mutex where libc++ defines its own macros for the annotations.

I assume you mean __mutex_base. Do we want to reuse the macros from that file? If so we'd have to include it in __thread_support. Maybe it is better to move the macros to __config instead, then.

[EricWF]

In D28520#641569, @dim wrote:

In D28520#641563, @EricWF wrote:

Also look in __mutex where libc++ defines its own macros for the annotations.

I assume you mean __mutex_base. Do we want to reuse the macros from that file? If so we'd have to include it in __thread_support. Maybe it is better to move the macros to __config instead, then.

Yes sorry __mutex_base. Moving them to __config sounds reasonable.

[dim]

• Move _LIBCPP_THREAD_SAFETY_ANNOTATION macro definition to __config.
• Add _LIBCPP_THREAD_SAFETY_ANNOTATION(no_thread_safety_analysis) macros to __threading_support function declarations which require them.

Note that I was not able to figure out how to make the other thread safety annotation attributes, such as try_acquire_capability(), match to the function signatures, as these use int instead of bool, for example.

[dim]

Hmm, actually this does not work. The definition of _LIBCPP_THREAD_SAFETY_ANNOTATION I moved from __mutex_base to __config is only enabled if _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS is manually defined.

There must have been some reason to do it like this in __mutex_base, but for __thread_support we unconditionally need such a macro. I will try defining them slightly differently in __thread_support only instead.

[dim]

Let's try this simpler version instead.

[aaron.ballman]

I feel like I must be missing something; why is this disabling rather than specifying the thread safety behavior? e.g., __libcpp_mutex_lock() specifying that it acquires the capability and __libcpp_mutex_unlock() specifying that it releases it?

[EricWF]

In D28520#646160, @aaron.ballman wrote:

I feel like I must be missing something; why is this disabling rather than specifying the thread safety behavior? e.g., __libcpp_mutex_lock() specifying that it acquires the capability and __libcpp_mutex_unlock() specifying that it releases it?

I agree. Also how is pthread_mutex_t getting annotated as a mutex type? Is it now done automatically?

[dim]

In D28520#646160, @aaron.ballman wrote:

I feel like I must be missing something; why is this disabling rather than specifying the thread safety behavior? e.g., __libcpp_mutex_lock() specifying that it acquires the capability and __libcpp_mutex_unlock() specifying that it releases it?

I wasn't able to figure out how that should work. The Thread Safety Analysis documentation specifies a number of macros that can be used, but does not directly document the attributes themselves. It looks like some of the attributes have a slightly different signature than the actual pthread functions, and the __libcpp wrapper for them, e.g. the example for TRY_ACQUIRE seems to assume a bool return value:

// Try to acquire the mutex.  Returns true on success, and false on failure.
bool TryLock() TRY_ACQUIRE(true);

where TRY_ACQUIRE(true) gets replaced by __attribute__ ((try_acquire_capability(true))). However, the signature for the __libcpp variant, and the "real" pthread function, is:

int __libcpp_mutex_trylock(__libcpp_mutex_t *__m)

so the return value is an int, where 0 means success, and any other value an error. I am unsure how this can be mapped to the attribute, and the documentation does not specify it.

In D28520#646169, @EricWF wrote:

Also how is pthread_mutex_t getting annotated as a mutex type? Is it now done automatically?

Since a few years, FreeBSD's <pthread.h> has locking annotations, for example https://svnweb.freebsd.org/base/head/include/pthread.h?view=markup#l228 which has:

int         pthread_mutex_init(pthread_mutex_t *__mutex,
                        const pthread_mutexattr_t *)
                    __requires_unlocked(*__mutex);
int         pthread_mutex_lock(pthread_mutex_t *__mutex)
                    __locks_exclusive(*__mutex);
int         pthread_mutex_trylock(pthread_mutex_t *__mutex)
                    __trylocks_exclusive(0, *__mutex);
int         pthread_mutex_timedlock(pthread_mutex_t *__mutex,
                        const struct timespec *)
                    __trylocks_exclusive(0, *__mutex);

These annotations expand to __attribute__((locks_excluded)), __attribute__((exclusive_lock_function)), and so on, if __has_extension(c_thread_safety_attributes) is true. Apparently @ed added these.

[dim]

Something like this might work, maybe. (I haven't yet run the full test suite, as that takes quite a while on my machines.)

I did not re-use the _LIBCPP_THREAD_SAFETY_ANNOTATION macro from __mutex_base, since that is included *after* this file, and it is only enabled when the user defines _LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS. The _LIBCPP_THREAD_SAFETY_ATTRIBUTE macro is defined iff clang supports any of the thread safety stuff.

Note that I had to add a capability attribute to the __libcpp_mutex types, otherwise this would not work.

Also, I had to place the thread safety attributes *after* the function declarations, since otherwise the compiler complains that it does not know the parameter name __m.

Furthermore, I am still not sure whether the true value for the try_acquire_capability attribute it correct. If I understand the documentation correctly, true means that the function only succeeds when trying the lock actually acquires it, and it fails when the lock was already acquired.

[dim]

Also added capability attributes to the non-pthread versions of __libcpp_mutex types.

[EricWF]

I think the real bug here is FreeBSD annotating pthread_mutex_t. That's going to break a lot of code that already uses -Wthread-safety. This kind of breakage is the reason libc++ doesn't enable the annotations on std::mutex by default.

[dim]

Actually, according to https://svnweb.freebsd.org/base?view=revision&revision=270943 (where the annotations were added), this helped to uncover existing bugs. I don't see why it would interfere with anything; if you ask for -Wthread-safety warnings, you should get them, right? What use are the warnings otherwise?

[dim]

Remove extraneous parenthesis.

[dim]

Rebase after recent changes.

[EricWF]

This breaks on all platforms were pthread_mutex_t isn't annotated.

[dim]

In D28520#646564, @EricWF wrote:

This breaks on all platforms were pthread_mutex_t isn't annotated.

Hm, sorry about that, I didn't realize. I'm building it on Ubuntu now to see what breaks, and how to fix it.

[dim]

Note that my earlier approach of just disabling -Wthread-safety for those few functions might be easier. This should not cause any trouble for any platforms which don't use annotated pthread functions.

[dim]

include/__threading_support

43 ↗

(On Diff #84465)

I think the least intrusive way would be to add a defined(__FreeBSD__) here, as that is the only OS with thread annotations for pthread functions, as far as I know.

The alternative is to turn off the capability annotations for __libcpp_mutex_t again, and use the no_thread_safety_analysis annotation for the functions.

Any particular preference?

[delesley]

Sorry about the slow response. My main concern here is that the thread safety analysis was designed for use with a library that wraps the system mutex in a separate Mutex class. We did that specifically to avoid breaking anything; code has to opt-in to the static checking by defining and using a Mutex class, and the API of that class is restricted to calls that can be easily checked via annotations. Including attributes directly in the standard library has the potential to cause lots of breakage and false positives.

Is there some way to control the #ifdefs so that the annotations are turned off by default for everyone except possibly freebsd, but there's still a way to turn them on for users who want to see the warnings? I'm not a libcxx expert.

[dim]

In D28520#648880, @delesley wrote:

Sorry about the slow response. My main concern here is that the thread safety analysis was designed for use with a library that wraps the system mutex in a separate Mutex class. We did that specifically to avoid breaking anything; code has to opt-in to the static checking by defining and using a Mutex class, and the API of that class is restricted to calls that can be easily checked via annotations. Including attributes directly in the standard library has the potential to cause lots of breakage and false positives.

Yes, I agree with that. However, on FreeBSD the pthread functions themselves are already annotated, so the libc++ wrapper functions cause -Werror warnings during the tests. Therefore one of my suggestions was to explicitly turn off warnings using no_thread_safety_analysis attributes. Is there any disadvantage in doing that?

Is there some way to control the #ifdefs so that the annotations are turned off by default for everyone except possibly freebsd, but there's still a way to turn them on for users who want to see the warnings? I'm not a libcxx expert.

Yes, that was one of my other suggestions, using #if defined(__FreeBSD__) to limit these attributes to FreeBSD only. We can do that either with the no_thread_safety_analysis attributes, or with the 'real' annotations, though the latter are not really useful in this case.

I'm really open to any variant, as long as something that works can get in before the 4.0.0 release. :)

[dim]

Let's try it with this much simpler version instead, which disables the thread safety analysis _only_ for FreeBSD, and nowhere else. And no messing with capabilities, either.

[aaron.ballman]

In D28520#652607, @dim wrote:

In D28520#648880, @delesley wrote:

Sorry about the slow response. My main concern here is that the thread safety analysis was designed for use with a library that wraps the system mutex in a separate Mutex class. We did that specifically to avoid breaking anything; code has to opt-in to the static checking by defining and using a Mutex class, and the API of that class is restricted to calls that can be easily checked via annotations. Including attributes directly in the standard library has the potential to cause lots of breakage and false positives.

Yes, I agree with that. However, on FreeBSD the pthread functions themselves are already annotated, so the libc++ wrapper functions cause -Werror warnings during the tests. Therefore one of my suggestions was to explicitly turn off warnings using no_thread_safety_analysis attributes. Is there any disadvantage in doing that?

Is there some way to control the #ifdefs so that the annotations are turned off by default for everyone except possibly freebsd, but there's still a way to turn them on for users who want to see the warnings? I'm not a libcxx expert.

Yes, that was one of my other suggestions, using #if defined(__FreeBSD__) to limit these attributes to FreeBSD only. We can do that either with the no_thread_safety_analysis attributes, or with the 'real' annotations, though the latter are not really useful in this case.

I'm really open to any variant, as long as something that works can get in before the 4.0.0 release. :)

I feel like there's still some confusion here (likely on my part). DeLesley was asking if there was a way to turn this off for everyone *except* FreeBSD (that is user-controllable), but your code turns it off specifically *for* FreeBSD with no option to enable. The part that has me confused is that FreeBSD is annotating their functionality specifically to enable thread safety checking; it seems like turning that checking off rather than supporting it is the wrong way to go. I think that may be why DeLesley was saying to disable the functionality for non-FreeBSD systems while still honoring it on FreeBSD, but if I'm confused, hopefully he'll clarify.

[dim]

In D28520#653360, @aaron.ballman wrote:

In D28520#652607, @dim wrote:

[...]

I'm really open to any variant, as long as something that works can get in before the 4.0.0 release. :)

I feel like there's still some confusion here (likely on my part). DeLesley was asking if there was a way to turn this off for everyone *except* FreeBSD (that is user-controllable), but your code turns it off specifically *for* FreeBSD with no option to enable. The part that has me confused is that FreeBSD is annotating their functionality specifically to enable thread safety checking; it seems like turning that checking off rather than supporting it is the wrong way to go. I think that may be why DeLesley was saying to disable the functionality for non-FreeBSD systems while still honoring it on FreeBSD, but if I'm confused, hopefully he'll clarify.

Ok, so let's go back to the previous variant, but with conditionals to be able to turn checking on, if so desired, using _LIBCPP_ENABLE_THREAD_SAFETY_ATTRIBUTE. Checking is on by default for FreeBSD only, but can still be disabled explicitly by defining _LIBCPP_DISABLE_THREAD_SAFETY_ATTRIBUTE.

[EricWF]

@dim I would really rather just suppress these warnings if we want them merged into 4.0.

[delesley]

The big question for me is whether these functions are exposed as part of the public libcxx API, or whether they are only used internally. (Again, I'm not a libcxx expert.) If they are part of the public API, then users may want to switch them on and off in their own code. In that case, I'm happy with the current variant.

If they are only used internally, then no_thread_safety_analysis is probably the correct option. (Unless, of course, the libcxx developers want to start using the analysis themselves, but I suspect they don't.)

[dim]

In D28520#656202, @delesley wrote:

The big question for me is whether these functions are exposed as part of the public libcxx API, or whether they are only used internally.

As far as I can see, they are only used internally, in src/algorithm.cpp and src/mutex.cpp. Of course they aren't "hidden" in any way in the include/__thread_support header, but obviously functions starting with double underscores are not meant as a public API.

If they are only used internally, then no_thread_safety_analysis is probably the correct option. (Unless, of course, the libcxx developers want to start using the analysis themselves, but I suspect they don't.)

Yes, and since @EricWF also prefers this, I going to switch this review back to the previous form again.

[dim]

Back to the previous version, using no_thread_safety_analysis for FreeBSD only. Optionally, we could delete the defined(__FreeBSD__) part, and disable the analysis for all platforms.

[delesley]

This looks good to me.

[aaron.ballman]

LGTM! Thank you for being patient while we figured this out. :-)

libcxx/trunk/include/__threading_support

 
 #if defined(_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL) || \
     defined(_LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL)
 #define _LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_FUNC_VIS
 #else
 #define _LIBCPP_THREAD_ABI_VISIBILITY inline _LIBCPP_INLINE_VISIBILITY
 #endif
 
+#if defined(__FreeBSD__) && defined(__clang__) && __has_attribute(no_thread_safety_analysis)
+#define _LIBCPP_NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
+#else
+#define _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
+#endif
+
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if defined(_LIBCPP_HAS_THREAD_API_PTHREAD)
 // Mutex
 typedef pthread_mutex_t __libcpp_mutex_t;
 #define _LIBCPP_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER
 
 typedef pthread_mutex_t __libcpp_recursive_mutex_t;

 
 #define _LIBCPP_TLS_DESTRUCTOR_CC WINAPI
 #endif
 
 // Mutex
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_recursive_mutex_init(__libcpp_recursive_mutex_t *__m);
 
-_LIBCPP_THREAD_ABI_VISIBILITY
+_LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
 int __libcpp_recursive_mutex_lock(__libcpp_recursive_mutex_t *__m);
 
-_LIBCPP_THREAD_ABI_VISIBILITY
+_LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
 bool __libcpp_recursive_mutex_trylock(__libcpp_recursive_mutex_t *__m);
 
-_LIBCPP_THREAD_ABI_VISIBILITY
+_LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
 int __libcpp_recursive_mutex_unlock(__libcpp_recursive_mutex_t *__m);
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_recursive_mutex_destroy(__libcpp_recursive_mutex_t *__m);
 
-_LIBCPP_THREAD_ABI_VISIBILITY
+_LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
 int __libcpp_mutex_lock(__libcpp_mutex_t *__m);
 
-_LIBCPP_THREAD_ABI_VISIBILITY
+_LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
 bool __libcpp_mutex_trylock(__libcpp_mutex_t *__m);
 
-_LIBCPP_THREAD_ABI_VISIBILITY
+_LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
 int __libcpp_mutex_unlock(__libcpp_mutex_t *__m);
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_mutex_destroy(__libcpp_mutex_t *__m);
 
 // Condition variable
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_condvar_signal(__libcpp_condvar_t* __cv);
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_condvar_broadcast(__libcpp_condvar_t* __cv);
 
-_LIBCPP_THREAD_ABI_VISIBILITY
+_LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
 int __libcpp_condvar_wait(__libcpp_condvar_t* __cv, __libcpp_mutex_t* __m);
 
-_LIBCPP_THREAD_ABI_VISIBILITY
+_LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_NO_THREAD_SAFETY_ANALYSIS
 int __libcpp_condvar_timedwait(__libcpp_condvar_t *__cv, __libcpp_mutex_t *__m,
                                timespec *__ts);
 
 _LIBCPP_THREAD_ABI_VISIBILITY
 int __libcpp_condvar_destroy(__libcpp_condvar_t* __cv);
 
 // Execute once
 _LIBCPP_THREAD_ABI_VISIBILITY