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

[Kostya Kortchinsky via Phabricator via llvm-commits]

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


================
Comment at: lib/scudo/standalone/mutex.h:40
+  enum State { Unlocked = 0, Locked = 1, Sleeping = 2 };
+  uptr Storage[1];
+
----------------
hctim wrote:
> cryptoad wrote:
> > 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.
> For GWP-ASan, we decided that `ifdef`s around the member and the header include were okay, which is also what `jemalloc` does. I was initially very hesitent to use opaque platform-specific bytes here, as I didn't know the internals of zircon's `sync_mutex_t`. Looks like it basically typedefs down to an int through `zx_futex_t`, which means it doesn't violate the standard.
> 
> On this note, why in the `linux` implementation was the choice made to use `SYS_futex` instead of `pthread_mutex_t`?
Replaced the opaque `uptr` with platform specific types.
Discussed offline the reason for not using `pthread_mutex_t`, to make a long story short: performance on some platforms.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D64358