EricWF closed this revision.
EricWF added a comment.
Committed in r263611.
http://reviews.llvm.org/D14731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
jamesr updated this revision to Diff 50793.
jamesr added a comment.
Add LLVM license headers to new test files
http://reviews.llvm.org/D14731
Files:
include/__config
include/__mutex_base
test/libcxx/test/config.py
test/libcxx/thread/thread.mutex/thread_safety_annotations_not_enabled.pas
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
Oh fudge. One last change. The tests need license headers. Just copy it from an
existing test. LGTM after that.
Thanks again.
http://reviews.llvm.org/D14731
__
jamesr updated this revision to Diff 50791.
jamesr added a comment.
Use // REQUIRES: thread-safety instead of macros to feature guard tests
http://reviews.llvm.org/D14731
Files:
include/__config
include/__mutex_base
test/libcxx/test/config.py
test/libcxx/thread/thread.mutex/thread_safet
EricWF added a comment.
Yes exactly. `// REQUIRES: ` tells LIT to skip the test (and report it
as UNSUPPORTED) whenever the feature is not present.
http://reviews.llvm.org/D14731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
jamesr added a comment.
In http://reviews.llvm.org/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_capab
EricWF added inline comments.
Comment at: include/__mutex_base:37
@@ -30,1 +36,3 @@
+
+class _LIBCPP_TYPE_VIS _LIBCPP_THREAD_SAFETY_ANNOTATION(capability("mutex"))
mutex
{
I appreciate the super thorough answer!
http://reviews.llvm.org/D14731
__
EricWF added a comment.
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 u
jamesr added a comment.
In http://reviews.llvm.org/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
jamesr updated this revision to Diff 50790.
http://reviews.llvm.org/D14731
Files:
include/__config
include/__mutex_base
test/libcxx/test/config.py
test/libcxx/thread/thread.mutex/thread_safety_annotations_not_enabled.pass.cpp
test/libcxx/thread/thread.mutex/thread_safety_lock_guard.pass
EricWF added a comment.
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
Comment at: include/__mutex_base:37
@@ -30,1 +36,3 @@
+
+clas
jamesr updated this revision to Diff 50788.
http://reviews.llvm.org/D14731
Files:
include/__config
include/__mutex_base
test/libcxx/test/format.py
test/libcxx/thread/thread.mutex/thread_safety_annotations_not_enabled.pass.cpp
test/libcxx/thread/thread.mutex/thread_safety_lock_guard.pass
jamesr marked 5 inline comments as done.
jamesr added a comment.
Thank you for the comments, new patch coming.. (is there a way to post comments
and upload a new patch at the same time?)
http://reviews.llvm.org/D14731
___
cfe-commits mailing list
c
EricWF added inline comments.
Comment at:
test/libcxx/thread/thread.mutex/thread_safety_access_guarded_without_lock.fail.cpp:1
@@ +1,2 @@
+#if !defined(__clang__) || !__has_attribute(acquire_capability)
+// This test is only meaningful on versions of clang that understand thread
jamesr updated this revision to Diff 50481.
jamesr added a comment.
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 r
EricWF added a comment.
In http://reviews.llvm.org/D14731#373301, @jamesr wrote:
> In http://reviews.llvm.org/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 b
jamesr added a comment.
In http://reviews.llvm.org/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 convent
jamesr added a comment.
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
consume
EricWF added a comment.
My suggestion would be to make these annotations OFF by default and allow users
to turn them on with a macro.
http://reviews.llvm.org/D14731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-b
EricWF added a comment.
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::mut
jamesr added a comment.
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 t
EricWF added a comment.
I generated an updated patch with a sample test case (and the LIT changes
needed to run it).
https://gist.github.com/EricWF/12f77078e4efc6610572
http://reviews.llvm.org/D14731
___
cfe-commits mailing list
cfe-commits@lists.l
EricWF added a subscriber: EricWF.
EricWF added a comment.
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 uns
jamesr added a comment.
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
aaron.ballman added a comment.
In http://reviews.llvm.org/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_att
jamesr added a subscriber: aaron.ballman.
jamesr added a comment.
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).
http://reviews.llvm.org/D14731
jamesr added a comment.
In http://reviews.llvm.org/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
jamesr updated this revision to Diff 41581.
jamesr added a comment.
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
_LIBCP
mclow.lists added a comment.
Where are the tests?
http://reviews.llvm.org/D14731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mclow.lists added a comment.
> 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`
http://reviews.llvm.org/D14731
___
cfe-commits mailin
mclow.lists added a comment.
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
neg
jamesr created this revision.
jamesr added a subscriber: cfe-commits.
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
32 matches
Mail list logo
