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

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

Tests?

Seems a shame to duplicate mutex again... Why can't use use the STL's version again? It doesn't allocate.



================
Comment at: compiler-rt/lib/gwp_asan/definitions.h:15
+#endif // defined(LIKELY)
+#define LIKELY(X) __builtin_expect(!!(X), 1)
+
----------------
You're not using this.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:32
+private:
+  void yieldProcessor(uint8_t Count);
+  void yieldPlatform();
----------------
`uint8_t` seems easy to inadvertently truncate. If you want to restrict size, make it a template and refuse to compile above a certain size. It's always `10` anyways.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:37
+
+  bool Locked = false;
+};
----------------
You should `static_assert` that this is always lock free.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:42
+public:
+  explicit ScopedLock(Mutex *Mx) : Mu(Mx) { Mu->lock(); }
+  ~ScopedLock() { Mu->unlock(); }
----------------
You should take a reference here, not a pointer.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:47
+  Mutex *Mu;
+};
+
----------------
`ScopedLock(const ScopedLock&) = delete;`


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:65
+#if defined(__i386__) || defined(__x86_64__)
+  asm volatile("" ::: "memory");
+  for (uint8_t i = 0; i < Count; ++i)
----------------
Seems like this leading compiler barrier should be above the `#if`.
You also might as well use something like `__atomic_signal_fence` instead.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:72
+    asm volatile("yield");
+#endif
+  asm volatile("" ::: "memory");
----------------
You should hard-error on other architectures.


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:81
+    else
+      yieldPlatform();
+
----------------
This won't compile if `__unix__` isn't defined.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61923