-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Revert "[libc++] Add thread safety annotations for std::lock (#154078)" #156964
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "[libc++] Add thread safety annotations for std::lock (#154078)" #156964
Conversation
…4078)" This reverts commit 0a2eb85. Reverting because this leads to a false positive with -Wthread-safety. See llvm#156760.
@llvm/pr-subscribers-libcxx Author: None (PiJoules) ChangesThis reverts commit 0a2eb85. Reverting because this leads to a false positive with -Wthread-safety. See #156760. Full diff: https://github.com/llvm/llvm-project/pull/156964.diff 3 Files Affected:
diff --git a/libcxx/include/__config b/libcxx/include/__config
index c197851f1c8fe..77a71b6cf1cae 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -934,7 +934,7 @@ typedef __char32_t char32_t;
# endif
# endif
-# if __has_attribute(__no_thread_safety_analysis__)
+# 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
diff --git a/libcxx/include/mutex b/libcxx/include/mutex
index 58474e0ca2b7a..78d8c8a9bcc6e 100644
--- a/libcxx/include/mutex
+++ b/libcxx/include/mutex
@@ -320,7 +320,7 @@ public:
};
template <class _L0, class _L1>
-_LIBCPP_NO_THREAD_SAFETY_ANALYSIS _LIBCPP_HIDE_FROM_ABI int try_lock(_L0& __l0, _L1& __l1) {
+_LIBCPP_HIDE_FROM_ABI int try_lock(_L0& __l0, _L1& __l1) {
unique_lock<_L0> __u0(__l0, try_to_lock_t());
if (__u0.owns_lock()) {
if (__l1.try_lock()) {
@@ -335,7 +335,7 @@ _LIBCPP_NO_THREAD_SAFETY_ANALYSIS _LIBCPP_HIDE_FROM_ABI int try_lock(_L0& __l0,
# ifndef _LIBCPP_CXX03_LANG
template <class _L0, class _L1, class _L2, class... _L3>
-_LIBCPP_NO_THREAD_SAFETY_ANALYSIS _LIBCPP_HIDE_FROM_ABI int try_lock(_L0& __l0, _L1& __l1, _L2& __l2, _L3&... __l3) {
+_LIBCPP_HIDE_FROM_ABI int try_lock(_L0& __l0, _L1& __l1, _L2& __l2, _L3&... __l3) {
int __r = 0;
unique_lock<_L0> __u0(__l0, try_to_lock);
if (__u0.owns_lock()) {
@@ -350,11 +350,8 @@ _LIBCPP_NO_THREAD_SAFETY_ANALYSIS _LIBCPP_HIDE_FROM_ABI int try_lock(_L0& __l0,
# endif // _LIBCPP_CXX03_LANG
-// We're using unique_lock to implement the functions, which thread annotations don't support. So we have to disable
-// the analysis inside the function.
template <class _L0, class _L1>
-_LIBCPP_NO_THREAD_SAFETY_ANALYSIS _LIBCPP_HIDE_FROM_ABI void lock(_L0& __l0, _L1& __l1)
- _LIBCPP_ACQUIRE_CAPABILITY(__l0, __l1) {
+_LIBCPP_HIDE_FROM_ABI void lock(_L0& __l0, _L1& __l1) {
while (true) {
{
unique_lock<_L0> __u0(__l0);
@@ -378,7 +375,7 @@ _LIBCPP_NO_THREAD_SAFETY_ANALYSIS _LIBCPP_HIDE_FROM_ABI void lock(_L0& __l0, _L1
# ifndef _LIBCPP_CXX03_LANG
template <class _L0, class _L1, class _L2, class... _L3>
-_LIBCPP_NO_THREAD_SAFETY_ANALYSIS void __lock_first(int __i, _L0& __l0, _L1& __l1, _L2& __l2, _L3&... __l3) {
+void __lock_first(int __i, _L0& __l0, _L1& __l1, _L2& __l2, _L3&... __l3) {
while (true) {
switch (__i) {
case 0: {
@@ -413,14 +410,8 @@ _LIBCPP_NO_THREAD_SAFETY_ANALYSIS void __lock_first(int __i, _L0& __l0, _L1& __l
}
}
-// We're using unique_lock to implement the functions, which thread annotations don't support. So we have to disable
-// the analysis inside the function.
template <class _L0, class _L1, class _L2, class... _L3>
-_LIBCPP_NO_THREAD_SAFETY_ANALYSIS inline _LIBCPP_HIDE_FROM_ABI void lock(_L0& __l0, _L1& __l1, _L2& __l2, _L3&... __l3)
-# if defined(_LIBCPP_CLANG_VER) && _LIBCPP_CLANG_VER >= 2101
- _LIBCPP_ACQUIRE_CAPABILITY(__l0, __l1, __l2, __l3...)
-# endif
-{
+inline _LIBCPP_HIDE_FROM_ABI void lock(_L0& __l0, _L1& __l1, _L2& __l2, _L3&... __l3) {
std::__lock_first(0, __l0, __l1, __l2, __l3...);
}
diff --git a/libcxx/test/extensions/clang/thread/thread.mutex/lock.verify.cpp b/libcxx/test/extensions/clang/thread/thread.mutex/lock.verify.cpp
deleted file mode 100644
index 51ffa6962ac83..0000000000000
--- a/libcxx/test/extensions/clang/thread/thread.mutex/lock.verify.cpp
+++ /dev/null
@@ -1,47 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// UNSUPPORTED: no-threads
-
-// <mutex>
-
-// GCC doesn't have thread safety attributes
-// UNSUPPORTED: gcc
-
-// ADDITIONAL_COMPILE_FLAGS: -Wthread-safety -Wno-comment
-
-// XFAIL: FROZEN-CXX03-HEADERS-FIXME
-
-#include <mutex>
-
-#include "test_macros.h"
-
-std::mutex m0;
-std::mutex m1;
-std::mutex m2;
-std::mutex m3;
-
-void f1() {
- std::lock(m0, m1);
-} // expected-warning {{mutex 'm0' is still held at the end of function}} \
- expected-warning {{mutex 'm1' is still held at the end of function}}
-
-#if TEST_STD_VER >= 11 && defined(TEST_CLANG_VER) && TEST_CLANG_VER >= 2101
-void f2() {
- std::lock(m0, m1, m2);
-} // expected-warning {{mutex 'm0' is still held at the end of function}} \
- expected-warning {{mutex 'm1' is still held at the end of function}} \
- expected-warning {{mutex 'm2' is still held at the end of function}}
-
-void f3() {
- std::lock(m0, m1, m2, m3);
-} // expected-warning {{mutex 'm0' is still held at the end of function}} \
- expected-warning {{mutex 'm1' is still held at the end of function}} \
- expected-warning {{mutex 'm2' is still held at the end of function}} \
- expected-warning {{mutex 'm3' is still held at the end of function}}
-#endif
|
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the reasoning here. -Wthread-safety
inherently has false-positives, so false-positives don't seem like a reason to remove the annotations again. If anything, we should expand them. If you can't deal with false-postives, don't enable a warning that has them.
I think I agree with @philnik777 here. This warning also doesn't provide any value if there are no annotations in the library. Instead, I'd much rather figure out ways to enhance these annotations to make them model more complex use cases and get rid of false positives (incrementally). Tentatively closing. |
This reverts commit 0a2eb85.
Reverting because this leads to a false positive with -Wthread-safety. See #156760.