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