[libcxx-commits] [PATCH] D115368: [NFC][libcxxabi] Re-organized inheritance structure to remove CRTP in cxa_guard

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM marked an inline comment as done.
DanielMcIntosh-IBM added inline comments.


================
Comment at: libcxxabi/src/cxa_guard_impl.h:282
+      : init_byte_address(_init_byte_address), thread_id_address(_thread_id_address),
+        has_thread_id_support(_thread_id_address != nullptr && GetThreadID != nullptr) {}
 
----------------
ldionne wrote:
> With this refactor, it now feels really weird to have this `GetThreadID != nullptr` here. Would it be possible to move the determination of whether the thread id is supported elsewhere (probably in the `GuardObject` constructor) so that `InitByteGlobalMutex` only has to check `_thread_id_address != nullptr`?
> 
> Same applies to Futex below.
It's possible, but without any further changes, `InitByteGlobalMutex` and `InitByteFutex` would still need the `GetThreadID` template parameter because of their `current_thread_id` member. So both the `GuardObject` and `InitByte...` classes would each need it as a template parameter, meaning we would have something like this:
```
GuardObject<InitByteGlobalMutex<LibcppMutex, LibcppCondVar, GlobalStatic<LibcppMutex>::instance,
                                               GlobalStatic<LibcppCondVar>::instance, PlatformThreadID>, PlatformThreadID>;
```
And for NoThreads we'd still have to pass `PlatformThreadID` as a template argument to GuardObject:
```
GuardObject<InitByteNoThreads, PlatformThreadID>;
```
That doesn't seem to me like much of an improvement to me, but you probably have a better idea of what is easiest to understand for someone who hasn't been looking at this for months.

I could try to avoid this by pulling out more than just `has_thread_id_support`, and maybe even going as far as to create a `ThreadIdWord` class or something. Unfortunately, while both GlobalMutex and Futex use `LazyValue<uint32_t, GetThreadID>` for fetching the current thread ID, after that they start to differ ever so slightly. Futex uses `AtomicInt<uint32_t> thread_id` while GlobalMutex reads and writes to it directly. There's also the issue of who should construct & who should own the ThreadIdWord instance - GuardObject is in charge of diving up the raw guard object, but `InitByte...` is the only one who interacts with it and (for GlobalMutex) all interaction with it needs to happen while we hold the lock.

All of which is to say, yes, it is possible, but I don't think it would make things better.


================
Comment at: libcxxabi/src/cxa_guard_impl.h:406-409
+      : init_byte(_init_byte_address),
+        has_thread_id_support(_thread_id_address != nullptr && GetThreadIDArg != nullptr),
+        thread_id(_thread_id_address), base_address(reinterpret_cast<int*>(
+                                           /*_init_byte_address & ~0x3*/ _init_byte_address - 1)) {}
----------------
ldionne wrote:
> Also, can you explain the `/*_init_byte_address & ~0x3*/` comment?
The goal here is not to compute the address of the raw guard object, but rather to compute the argument we need to pass to `Wait()` - a pointer to the word that includes `_init_byte_address` - i.e. `_init_byte_address & ~0x3`. However bitwise arithmetic is prohibited on pointers, so `_init_byte_address & ~0x3` is an illegal expression.

We could either cast to an arithmetic type such as `uintptr_t` and back again, or go the more direct route and use what we know about the layout of the raw guard object (and perform the exact inverse of what we do in `GuardObject` to compute `_init_byte_address`). I believe I went the direct route because using casts would have //technically// been UB, even if it is all but guaranteed to be safe (though I'm not too sure anymore since it has been a long time since I made the decision). Unfortunately, during the internal review downstream, the direct approach led to some confusion, so I added a short comment to hopefully clarify the intentions a bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115368