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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
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();
----------------
cryptoad wrote:
> 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.
Constant initialization should generally be done at compile-time for statics, and non-IE TLS will have to zero-init either in the c'tor or `init()` anyway, right?

Currently, it's quite hard to keep track of whether the mutex is initialised or not. For example, `SizeClassInfo::Mutex` is only safe as it's used in `Allocator::Primary` which is (as far as I can tell) statically allocated. There shouldn't be a perf overhead for this, and it would make it a strong guarantee that the mutex is always initialised.

Up to you though - I personally find it very hard to keep track of what needs to be initialised. Also, if there is ever a change that makes the class unusable if zero-initialised, the author may mistakenly assume `init()` is called every time.


================
Comment at: lib/scudo/standalone/mutex.h:27
+#ifdef __clang__
+#pragma nounroll
+#endif
----------------
cryptoad wrote:
> 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.
Sounds good. Could you add this as a comment for future readers?


================
Comment at: lib/scudo/standalone/mutex.h:40
+  enum State { Unlocked = 0, Locked = 1, Sleeping = 2 };
+  uptr Storage[1];
+
----------------
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`?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D64358