[PATCH] D61923: [GWP-ASan] Mutex implementation [2].

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:22
+  constexpr Mutex() = default;
+  ~Mutex() = default;
+  // Lock the mutex.
----------------
We should probably delete copy constructor and operator=


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:40
+  ~ScopedLock() { Mu.unlock(); }
+  ScopedLock(const ScopedLock &) = delete;
+
----------------
We should also delete operator=.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp:19
+  // Remove warning for non-debug builds.
+  (void)(Status);
+}
----------------
Unnecessary parens (also below)


================
Comment at: compiler-rt/lib/gwp_asan/tests/driver.cpp:14
+  return RUN_ALL_TESTS();
+}
----------------
Can we just link with gtest_main instead of having this?


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:20
+
+TEST(GwpAsanMutexTest, ConstructionTest) {
+  Mutex Mu;
----------------
Nit: ConstructionTest doesn't seem to describe what this test does.

Something like LockUnlockTest seems more descriptive.

(Same for other "Construction" tests)


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:37
+  Mutex Mu;
+  { ScopedLock L(Mu); }
+  // Locking will fail here if the scoped lock failed to unlock.
----------------
We should trylock inside the scope, and maybe also do an unlock+lock.


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:43
+
+static constexpr unsigned kNumLocksInARow = 1000;
+void lockTask(std::atomic<bool> *StartingGun, Mutex *Mu) {
----------------
This doesn't need to be a global.


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:44
+static constexpr unsigned kNumLocksInARow = 1000;
+void lockTask(std::atomic<bool> *StartingGun, Mutex *Mu) {
+  while (!StartingGun->load())
----------------
static


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:45
+void lockTask(std::atomic<bool> *StartingGun, Mutex *Mu) {
+  while (!StartingGun->load())
+    ;
----------------
Can we use the simpler syntax `while (!StartingGun)` instead?


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:46
+  while (!StartingGun->load())
+    ;
+  for (unsigned i = 0; i < kNumLocksInARow; ++i) {
----------------
Format seems weird.  Does clang-format let us do `while (...) {}` on one line?


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:49
+    ScopedLock L(*Mu);
+  }
+}
----------------
Nit: curly braces unnecessary


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:59
+
+  StartingGun.store(true);
+  T1.join();
----------------
Can we use simpler `StartingGun = true`?


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:68
+
+void thrashTask(std::atomic<bool> *StartingGun, Mutex *Mu, unsigned *Counter,
+                unsigned NumIterations) {
----------------
static


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:82
+    NumThreads = std::thread::hardware_concurrency();
+  }
+
----------------
I don't think we should care about the number of cores at all.  Time-sliced threads are just fine for this test.


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:101
+
+TEST(GwpAsanMutexTest, ThrashTests) {
+  runThrashTest(2, 10000);
----------------
How are any of these "thrash" tests?

I'd prefer something more descriptive, like SynchronizedCounterTest.


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:102
+TEST(GwpAsanMutexTest, ThrashTests) {
+  runThrashTest(2, 10000);
+  runThrashTest(4, 10000);
----------------
This test case seems to test a superset of ThreadedConstructionTest, so do we really need that one?


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:104
+  runThrashTest(4, 10000);
+  runThrashTest(4, 100000);
+}
----------------
4 threads isn't very much.  Can we test with more threads, say ~1000?


================
Comment at: compiler-rt/test/gwp_asan/CMakeLists.txt:1
+set(GWP_ASAN_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+set(GWP_ASAN_LIT_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
----------------
Can the lit test stuff go in another patch when we actually have tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61923