[libcxx-commits] [PATCH] D114119: [libcxx] Fix potential lost wake-up in counting semaphore

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a subscriber: __simt__.
ldionne added a comment.
This revision now requires changes to proceed.

I would actually like @__simt__ to take a look at this one.

If we do agree this is a proper fix, then yeah I would cherry-pick to `release/13.x`.



================
Comment at: libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp:11
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// This is a regression test for PR#47013.
----------------
You will need to add:

```
// This test requires the dylib support introduced in D68480, which shipped in macOS 11.0.
// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}}
```

You will also need to add:

```
// TODO(ldionne): This test fails on Ubuntu Focal on our CI nodes (and only there), in 32 bit mode.
// UNSUPPORTED: linux && 32bits-on-64bits
```

Sorry, I still have not figured this one out.



================
Comment at: libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp:51-53
+    threads.emplace_back(acquire);
+
+  threads.emplace_back(release);
----------------
This is used for some freestanding configurations where there is no way to create a thread without providing a stack size. In those configurations, `support::make_test_thread` can be used to pass a stack size.

That isn't covered by CI yet so you couldn't notice.


================
Comment at: libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp:9
+//
+// REQUIRES: long_tests
+// UNSUPPORTED: libcpp-has-no-threads
----------------
Quuxplusone wrote:
> I assume that `REQUIRES: long_tests` is a tradeoff — faster tests in most configurations, but testing of this specific test only in one (or a few) configurations; right?
> This test takes only 1.3 seconds on my laptop. Maybe we should just knock the iteration count down from `100'000` to `10'000` (so it takes 0.13 seconds) and then we can definitely remove this `REQUIRES`. Defer to @ldionne here.
`REQUIRES: long_tests` was used for slow devices like simulators. It's not really maintained anymore.

By default, it is enabled. I'm OK with the current state, i.e. we always run this test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114119/new/

https://reviews.llvm.org/D114119