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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
