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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM % test nitpicks. However, I believe we should force @ldionne to look at this one. :)  `<semaphore>` has already gotten two patches merged back to 13.x, and this might be another one.



================
Comment at: libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp:9
+//
+// REQUIRES: long_tests
+// UNSUPPORTED: libcpp-has-no-threads
----------------
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.


================
Comment at: libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp:33-36
+  for (auto i = 0; i < 100'000; ++i) {
+    for (auto j = 0; j < 4; ++j)
+      s.release(1);
+    b.arrive_and_wait();
----------------
Throughout: please `int [ij] = 0` instead of `auto [ij] = 0`.
Also, I find this nested-loop version much harder to grok at first sight (I mean, maybe just because I wrote the other version myself ;)). IMO it would help to unroll the `j` loop.
I guess the real question is, does the change from "2 acquiring threads, 2 releases in 1 releasing thread" to "8 acquiring threads, 8 releases in 2 releasing threads" make the buggy situation more or less likely to occur? From the description in https://bugs.llvm.org/show_bug.cgi?id=47013#c1 , I //think// that increasing the number of waiting acquirers is good (because each adjacent pair of acquirers has a chance to race with each other, so we have 7x the chances to reproduce in each loop iteration), but I don't see how increasing the number of releasers helps. If you think it'd be harmless to go back to just one releaser (keeping all 8 acquirers, and ideally still unrolling the loop), that would simplify this code IMHO.


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

https://reviews.llvm.org/D114119