[PATCH] D64358: [scudo][standalone] Merge Spin & Blocking mutex into a Hybrid one

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 3 inline comments as done.
cryptoad added inline comments.


================
Comment at: lib/scudo/standalone/common.h:122
+  // Bionic uses a hardcoded value.
+  if (SCUDO_ANDROID)
+    return 4096U;
----------------
hctim wrote:
> Could we potentially add an android-specific test to make sure this assumption doesn't change?
> 
> Also, could this potentially be a compile-time resolution? getPageSizeCached() is called for every allocation to the secondary allocator and seems like an unneccessary branch that can be eliminated.
Good point, I will add a test.
This will be optimized by the compiler, as `SCUDO_ANDROID` is a compile time constant.
>From previous sanitizer_common reviews, this construct is preferred to an `#if` as it is less cumbersome, and more maintainable.


================
Comment at: lib/scudo/standalone/mutex.h:21
 public:
-  void init() { atomic_store_relaxed(&State, 0); }
-
-  void lock() {
+  void init() { memset(this, 0, sizeof(*this)); }
+  bool tryLock();
----------------
hctim wrote:
> Instead of memset-init here, can we simply have a `constexpr` constructor to guarantee zero-initialisation? Would potentially prevent forgotten `init()` in future.
The benefit here is to not do unnecessary zeroing of memory when we are dealing with TLS or static variable that are already zero'd out.
That allows us to only have to use `init` in tests and for stack variables.
I'd rather keep it that way since it's conformant with other similar code constructs.


================
Comment at: lib/scudo/standalone/mutex.h:27
+#ifdef __clang__
+#pragma nounroll
+#endif
----------------
hctim wrote:
> Why?
We do not need speed here, a tighter function is preferable.
>From g3 & Android disassembly, the compiler unrolls that leading to a giant chunk that isn't ideal.
I tried to figure out a way that would work for all compilers, but loop optimization (or disabling of) is fairly specific afaict.
I settled for this.


================
Comment at: lib/scudo/standalone/mutex.h:40
+  enum State { Unlocked = 0, Locked = 1, Sleeping = 2 };
+  uptr Storage[1];
+
----------------
hctim wrote:
> Why is this a `uptr[1]`? It looks like the underlying `atomic_compare_exchange_strong` supports any atomic type, including char.
> 
> From my reading of the spec, the underlying type for an unscoped enum is implementation defined. If you want a concrete type, you could always use `enum State : char { ... }`. If you're worried about alignment, you could always use the uptr as the backing type or `alignas`. I feel like `State Storage;` would be much more easy to understand here. You could always also use `enum class` to get a guaranteed integer-backed enum.
This is variable that stores the underlying platform-specific data.
For Linux, we want a `u32` (atomic), but for Fuchsia it's a `sync_mutex_t`, which should be opaque to us (Fuchsia doesn't use the State enum).
A single `uptr` is enough to store either (with `CHECK`s in their respective implementation to verify size and alignment), but using an array would make it easier to add other platforms.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D64358