[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/tests/driver.cpp:14
+  return RUN_ALL_TESTS();
+}
----------------
hctim wrote:
> morehouse wrote:
> > Can we just link with gtest_main instead of having this?
> Unfortunately not easily. `generate_compiler_rt_tests` builds each test cpp file individually, and provides no way to add `gtest_main` only during link time. 
Then how does this driver.cpp get included in each unit test?

Maybe we should put the `main` in each test as is usually done.


================
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.
----------------
morehouse wrote:
> We should trylock inside the scope, and maybe also do an unlock+lock.
No, I didn't mean that way.

Something like this:
```
{
  ScopedLock L(Mu);
  EXPECT_FALSE(Mu.tryLock());  // Test that the ctor did in fact lock
  Mu.unlock()
  EXPECT_TRUE(Mu.tryLock());  // Test that manual unlock overrides ScopedLock
}
EXPECT_TRUE(Mu.tryLock());  // Test that dtor did in fact unlock
```


================
Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:46
+  while (!StartingGun->load())
+    ;
+  for (unsigned i = 0; i < kNumLocksInARow; ++i) {
----------------
hctim wrote:
> morehouse wrote:
> > Format seems weird.  Does clang-format let us do `while (...) {}` on one line?
> CF changes it to 
> ```
> while (!StartingGun) {
> }
> ```
Not a big deal, but might be more readable if we do:

```
while (!StartingGun) {
  // Wait for starting gun.
}
```


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61923