[libc++] Fix std::atomic::wait ulock wait UL_COMPARE_AND_WAIT64
#92783
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Hui (huixie90) ChangesFull diff: https://github.com/llvm/llvm-project/pull/92783.diff 2 Files Affected:
diff --git a/libcxx/src/atomic.cpp b/libcxx/src/atomic.cpp
index 44100d4414e0d..031f247f88c2f 100644
--- a/libcxx/src/atomic.cpp
+++ b/libcxx/src/atomic.cpp
@@ -69,17 +69,17 @@ extern "C" int __ulock_wait(
uint32_t operation, void* addr, uint64_t value, uint32_t timeout); /* timeout is specified in microseconds */
extern "C" int __ulock_wake(uint32_t operation, void* addr, uint64_t wake_value);
-# define UL_COMPARE_AND_WAIT 1
+# define UL_COMPARE_AND_WAIT64 5
# define ULF_WAKE_ALL 0x00000100
static void
__libcpp_platform_wait_on_address(__cxx_atomic_contention_t const volatile* __ptr, __cxx_contention_t __val) {
- __ulock_wait(UL_COMPARE_AND_WAIT, const_cast<__cxx_atomic_contention_t*>(__ptr), __val, 0);
+ __ulock_wait(UL_COMPARE_AND_WAIT64, const_cast<__cxx_atomic_contention_t*>(__ptr), __val, 0);
}
static void __libcpp_platform_wake_by_address(__cxx_atomic_contention_t const volatile* __ptr, bool __notify_one) {
__ulock_wake(
- UL_COMPARE_AND_WAIT | (__notify_one ? 0 : ULF_WAKE_ALL), const_cast<__cxx_atomic_contention_t*>(__ptr), 0);
+ UL_COMPARE_AND_WAIT64 | (__notify_one ? 0 : ULF_WAKE_ALL), const_cast<__cxx_atomic_contention_t*>(__ptr), 0);
}
#elif defined(__FreeBSD__) && __SIZEOF_LONG__ == 8
diff --git a/libcxx/test/libcxx/atomics/atomics.syn/wait.pass.cpp b/libcxx/test/libcxx/atomics/atomics.syn/wait.pass.cpp
new file mode 100644
index 0000000000000..f59db4109bfd5
--- /dev/null
+++ b/libcxx/test/libcxx/atomics/atomics.syn/wait.pass.cpp
@@ -0,0 +1,30 @@
+//===----------------------------------------------------------------------===//
+//
+// 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: c++03, c++11, c++14, c++17
+// XFAIL: availability-synchronization_library-missing
+// XFAIL: !has-64-bit-atomics
+
+#include <atomic>
+#include <cassert>
+
+#include "test_macros.h"
+
+void test_85107() {
+ // https://github.com/llvm/llvm-project/issues/85107
+ // [libc++] atomic_wait uses UL_COMPARE_AND_WAIT when it should use UL_COMPARE_AND_WAIT64 on Darwin
+ constexpr std::__cxx_contention_t old_val = 0;
+ constexpr std::__cxx_contention_t new_val = old_val + (1l << 32);
+ std::__cxx_atomic_contention_t ct(new_val);
+ std::__libcpp_atomic_wait(&ct, old_val);
+}
+
+int main(int, char**) {
+ test_85107();
+
+ return 0;
+}
|
| // UNSUPPORTED: no-threads | ||
| // XFAIL: availability-synchronization_library-missing | ||
| // XFAIL: !has-64-bit-atomics | ||
|
|
There was a problem hiding this comment.
| // This bug was first fixed in LLVM 19 | |
| // XFAIL: using-built-library-before-llvm-19 |
There was a problem hiding this comment.
I'm assuming that implies "Darwin"?
There was a problem hiding this comment.
https://buildkite.com/llvm-project/libcxx-ci/builds/35617#018fc382-8cf3-45dc-a8c8-c3c9d9ccd245
Unfortunately we have "unexpected passes" because of this line. It seems that the CI "Apple system" does have this feature
using-built-library-before-llvm-19
and from what I can see, it is linking against the just built libc++ which has the fix in it
-L /Users/libcxx-buildkite-agent/libcxx.buildkite-agent/builds/f1-1-macminivault-com/llvm-project/libcxx-ci/build/apple-system/arm64/lib -lc++
| // UNSUPPORTED: no-threads | ||
| // XFAIL: availability-synchronization_library-missing | ||
| // XFAIL: !has-64-bit-atomics | ||
|
|
There was a problem hiding this comment.
I'm assuming that implies "Darwin"?
| // https://github.com/llvm/llvm-project/issues/85107 | ||
| // [libc++] atomic_wait uses UL_COMPARE_AND_WAIT when it should use UL_COMPARE_AND_WAIT64 on Darwin | ||
| constexpr std::__cxx_contention_t old_val = 0; | ||
| constexpr std::__cxx_contention_t new_val = old_val + (1l << 32); |
There was a problem hiding this comment.
Unfortunately the if constexpr doesn't really help us here, since it's not a template, all of the semantic checking is still done it seems. Which is what's causing the windows builds to fail (it appears).
Changing the 1l to 1ll might help stop the complaining, and since we're basically asserting that sizeof(long) > 4, it seems like a change that shouldn't change the meaning?
|
LGTM once the bots are passing. |
| // UNSUPPORTED: no-threads | ||
| // XFAIL: availability-synchronization_library-missing | ||
| // This bug was first fixed in LLVM 19 | ||
| // XFAIL: using-built-library-before-llvm-19 |
There was a problem hiding this comment.
About the CI failures:
The apple-system CI job should be renamed to something like apple-flavor or something like that. Then,
- It defines
stdlib=apple-libc++because it's the apple-configured flavor - It does NOT define
stdlib=system, because we are NOT running against the system-provided library
The back-deployment CI jobs running on Apple would then define stdlib=system and target=<arch>-apple-macosx<version>.
We also need something that implicitly defines stdlib=apple-libc++ whenever we have stdlib=system and a target of macosx (or we could set the feature manually too).
I'll work on that patch, this is just self notes.
| // UNSUPPORTED: no-threads | ||
| // XFAIL: availability-synchronization_library-missing | ||
| // This bug was first fixed in LLVM 19 | ||
| // XFAIL: using-built-library-before-llvm-19 |
There was a problem hiding this comment.
Maybe just turn this into UNSUPPORTED since the apple-backdeployment-hardening looks like it would require very tedious checking of the software versions and maybe even hardware.
in
atomic::wait, when we call the platform wait ulock_wait , we are using UL_COMPARE_AND_WAIT. But we should use UL_COMPARE_AND_WAIT64 instead as the address we are waiting for is a 64 bit integer.fixes #85107
It is rather hard to test directly because in
atomic::wait, before calling into the platform wait, our c++ code has some poll logic which checks the value not changing. Thus in this patch, the test is using the internal function.