Skip to content

Conversation

PiJoules
Copy link
Contributor

@PiJoules PiJoules commented Sep 4, 2025

This reverts commit 0a2eb85.

Reverting because this leads to a false positive with -Wthread-safety. See #156760.

…4078)"

This reverts commit 0a2eb85.

Reverting because this leads to a false positive with -Wthread-safety.
See llvm#156760.
@PiJoules PiJoules requested a review from a team as a code owner September 4, 2025 20:23
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 4, 2025
@PiJoules PiJoules requested a review from philnik777 September 4, 2025 20:24
@llvmbot
Copy link
Member

llvmbot commented Sep 4, 2025

@llvm/pr-subscribers-libcxx

Author: None (PiJoules)

Changes

This 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:

  • (modified) libcxx/include/__config (+1-1)
  • (modified) libcxx/include/mutex (+5-14)
  • (removed) libcxx/test/extensions/clang/thread/thread.mutex/lock.verify.cpp (-47)
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

@PiJoules PiJoules requested a review from ldionne September 4, 2025 20:24
@frederick-vs-ja

This comment was marked as resolved.

Copy link
Contributor

@philnik777 philnik777 left a 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.

@ldionne
Copy link
Member

ldionne commented Sep 5, 2025

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.

@ldionne ldionne closed this Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants