Skip to content
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

[libc++] Make sure the test for compare-and-wait bug doesn't hang forever #97907

Merged

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Jul 6, 2024

This patch adds an explicit timeout mechanism in the compare-and-wait test for std::atomic, ensuring that it doesn't run forever when the bug is present. This is not an issue when we run inside the CI because we specify a timeout manually, but it can be a problem when running locally, for example.

…ever

This patch adds an explicit timeout mechanism in the compare-and-wait
test for std::atomic, ensuring that it doesn't run forever when the
bug is present. This is not an issue when we run inside the CI because
we specify a timeout manually, but it can be a problem when running
locally, for example.
@ldionne ldionne requested a review from a team as a code owner July 6, 2024 18:30
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 6, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 6, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This patch adds an explicit timeout mechanism in the compare-and-wait test for std::atomic, ensuring that it doesn't run forever when the bug is present. This is not an issue when we run inside the CI because we specify a timeout manually, but it can be a problem when running locally, for example.


Full diff: https://github.com/llvm/llvm-project/pull/97907.diff

1 Files Affected:

  • (renamed) libcxx/test/libcxx/atomics/atomics.syn/wait.issue_85107.pass.cpp (+24-6)
diff --git a/libcxx/test/libcxx/atomics/atomics.syn/wait.pass.cpp b/libcxx/test/libcxx/atomics/atomics.syn/wait.issue_85107.pass.cpp
similarity index 57%
rename from libcxx/test/libcxx/atomics/atomics.syn/wait.pass.cpp
rename to libcxx/test/libcxx/atomics/atomics.syn/wait.issue_85107.pass.cpp
index 6ae1c0997aad72..03eaa0e55ac6a8 100644
--- a/libcxx/test/libcxx/atomics/atomics.syn/wait.pass.cpp
+++ b/libcxx/test/libcxx/atomics/atomics.syn/wait.issue_85107.pass.cpp
@@ -15,22 +15,40 @@
 
 // XFAIL: availability-synchronization_library-missing
 
+// This is a regression test for https://github.com/llvm/llvm-project/issues/85107, which describes
+// how we were using UL_COMPARE_AND_WAIT instead of UL_COMPARE_AND_WAIT64 in the implementation of
+// atomic::wait, leading to potential infinite hangs.
+
 #include <atomic>
 #include <cassert>
+#include <chrono>
+
+#include "make_test_thread.h"
 
-void test_85107() {
+int main(int, char**) {
   if constexpr (sizeof(std::__cxx_contention_t) == 8 && sizeof(long) > 4) {
+    std::atomic<bool> done = false;
+    auto const timeout     = std::chrono::system_clock::now() + std::chrono::seconds(600); // fail after 10 minutes
+
+    auto timeout_thread = support::make_test_thread([&] {
+      while (!done) {
+        assert(std::chrono::system_clock::now() < timeout); // make sure we don't hang forever
+      }
+    });
+
     // 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 + (1ll << 32);
     std::__cxx_atomic_contention_t ct(new_val);
-    std::__libcpp_atomic_wait(&ct, old_val); // this will hang forever if the bug is present
-  }
-}
 
-int main(int, char**) {
-  test_85107();
+    // This would hang forever if the bug is present, but the test will fail in a bounded amount of
+    // time due to the timeout above.
+    std::__libcpp_atomic_wait(&ct, old_val);
+
+    done = true;
+    timeout_thread.join();
+  }
 
   return 0;
 }

@ldionne ldionne requested a review from huixie90 July 8, 2024 20:19
@ldionne
Copy link
Member Author

ldionne commented Jul 12, 2024

Discussed with Hui during live review today, got verbal LGTM.

@ldionne ldionne merged commit 0d26216 into llvm:main Jul 12, 2024
57 checks passed
@ldionne ldionne deleted the review/compare-and-wait-dont-hang-forever branch July 12, 2024 19:00
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
…ever (llvm#97907)

This patch adds an explicit timeout mechanism in the compare-and-wait
test for std::atomic, ensuring that it doesn't run forever when the bug
is present. This is not an issue when we run inside the CI because we
specify a timeout manually, but it can be a problem when running
locally, for example.
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.

None yet

2 participants