Page MenuHomePhabricator

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

Authored by dim on Jan 10 2017, 10:31 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

dim updated this revision to Diff 83825.Jan 10 2017, 10:31 AM
dim retitled this revision from to Disable -Wthread-safety-analysis for some functions in __thread_support.
dim updated this object.
dim added reviewers: EricWF, mclow.lists.
dim added subscribers: cfe-commits, emaste, joerg.
aaron.ballman added a subscriber: aaron.ballman.

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

dim added a comment.Jan 10 2017, 10:54 AM

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.

In D28520#641536, @dim 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 edited edge metadata.Jan 10 2017, 11:16 AM

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

dim added a comment.Jan 10 2017, 11:19 AM

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.

In D28520#641569, @dim 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 updated this revision to Diff 83843.Jan 10 2017, 12:00 PM
dim edited edge metadata.
  • 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 added a comment.Jan 10 2017, 12:17 PM

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 updated this revision to Diff 83851.Jan 10 2017, 12:30 PM

Let's try this simpler version instead.

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 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 added a subscriber: ed.Jan 14 2017, 6:13 AM

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.

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 updated this revision to Diff 84451.Jan 14 2017, 7:31 AM

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 updated this revision to Diff 84452.Jan 14 2017, 8:11 AM

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

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 added a comment.Jan 14 2017, 11:42 AM

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 updated this revision to Diff 84462.Jan 14 2017, 12:50 PM

Remove extraneous parenthesis.

dim updated this revision to Diff 84465.Jan 14 2017, 2:42 PM

Rebase after recent changes.

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

dim added a comment.Jan 14 2017, 3:06 PM

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 added a comment.Jan 14 2017, 3:26 PM

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 added inline comments.Jan 17 2017, 1:30 PM
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 edited edge metadata.Jan 17 2017, 2:41 PM

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 added a comment.Jan 22 2017, 5:02 AM

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 updated this revision to Diff 85283.Jan 22 2017, 10:47 AM

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.

In D28520#652607, @dim 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 updated this revision to Diff 85440.Jan 23 2017, 12:13 PM
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.

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

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 added a comment.Jan 25 2017, 1:11 PM

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 updated this revision to Diff 85794.Jan 25 2017, 1:21 PM

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.

dim marked an inline comment as done.Jan 25 2017, 1:22 PM

This looks good to me.

aaron.ballman accepted this revision.Jan 26 2017, 7:00 AM

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

This revision is now accepted and ready to land.Jan 26 2017, 7:00 AM
This revision was automatically updated to reflect the committed changes.