This revision was automatically updated to reflect the committed changes.
Closed by commit rL293197: Disable thread safety analysis for some functions in
__thread_support (authored by dim).
Changed prior to commit:
https://reviews.llvm.org/D28520?vs=85794=85939#toc
Repository:
rL LLVM
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM! Thank you for being patient while we figured this out. :-)
https://reviews.llvm.org/D28520
___
cfe-commits mailing list
delesley added a comment.
This looks good to me.
https://reviews.llvm.org/D28520
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dim updated this revision to Diff 85794.
dim added a comment.
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.
https://reviews.llvm.org/D28520
Files:
dim added a comment.
In https://reviews.llvm.org/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
delesley added a comment.
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
EricWF added a comment.
@dim I would really rather just suppress these warnings if we want them merged
into 4.0.
https://reviews.llvm.org/D28520
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dim updated this revision to Diff 85440.
dim added a comment.
In https://reviews.llvm.org/D28520#653360, @aaron.ballman wrote:
> In https://reviews.llvm.org/D28520#652607, @dim wrote:
>
> >
>
[...]
>> I'm really open to any variant, as long as something that works can get in
>> before the
aaron.ballman added a comment.
In https://reviews.llvm.org/D28520#652607, @dim wrote:
> In https://reviews.llvm.org/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
dim updated this revision to Diff 85283.
dim added a comment.
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.
https://reviews.llvm.org/D28520
Files:
dim added a comment.
In https://reviews.llvm.org/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
delesley added a comment.
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
dim added inline comments.
Comment at: include/__threading_support:43
+#if defined(__clang__) && __has_attribute(acquire_capability)
+#define _LIBCPP_THREAD_SAFETY_ATTRIBUTE(x) __attribute__((x))
I think the least intrusive way would be to add a
dim added a comment.
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.
https://reviews.llvm.org/D28520
dim added a comment.
In https://reviews.llvm.org/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.
https://reviews.llvm.org/D28520
EricWF added a comment.
This breaks on all platforms were pthread_mutex_t isn't annotated.
https://reviews.llvm.org/D28520
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dim updated this revision to Diff 84465.
dim added a comment.
Rebase after recent changes.
https://reviews.llvm.org/D28520
Files:
include/__threading_support
Index: include/__threading_support
===
---
dim added a comment.
Actually, according to
https://svnweb.freebsd.org/base?view=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?
dim updated this revision to Diff 84451.
dim added a comment.
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
dim added a subscriber: ed.
dim added a comment.
In https://reviews.llvm.org/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
EricWF added a comment.
In https://reviews.llvm.org/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
aaron.ballman added a comment.
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?
dim updated this revision to Diff 83851.
dim added a comment.
Let's try this simpler version instead.
https://reviews.llvm.org/D28520
Files:
include/__threading_support
Index: include/__threading_support
===
---
dim added a comment.
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
dim updated this revision to Diff 83843.
dim added a comment.
- 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
EricWF added a comment.
In https://reviews.llvm.org/D28520#641569, @dim wrote:
> In https://reviews.llvm.org/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
dim added a comment.
In https://reviews.llvm.org/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
EricWF added a comment.
Also look in `__mutex` where libc++ defines its own macros for the annotations.
https://reviews.llvm.org/D28520
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
aaron.ballman added a comment.
In https://reviews.llvm.org/D28520#641536, @dim wrote:
> In https://reviews.llvm.org/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
aaron.ballman added a reviewer: delesley.
aaron.ballman added a comment.
Alternatively, these functions could be given the proper thread safety
annotations, couldn't they?
https://reviews.llvm.org/D28520
___
cfe-commits mailing list
dim created this revision.
dim added reviewers: EricWF, mclow.lists.
dim added subscribers: cfe-commits, emaste, joerg.
Many thread-related libc++ test cases fail on FreeBSD, due to the following
-Werror warnings:
In file included from
31 matches
Mail list logo
