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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Please don't mark this as being a NFC in the commit message. This is a refactoring, and we don't expect it to change functionality, but we generally tag things like "remove unused include" or "fix comment" as NFCs. If there happens to be a bug in there, it would be bad to skip it from the commit list when searching for potential regressions because this was marked as a NFC.

Apart from that, thanks for your patience going through the review and for improving the documentation like this. LGTM with a few comments.



================
Comment at: libcxxabi/src/cxa_guard_impl.h:215
+//      COMPLETE:           Initialization was finished by somebody else. Return true.
+//      PENDING:            Somebeody has started the initialization already, set the WAITING bit,
+//                          then wait for the init byte to get updated with a new value.
----------------



================
Comment at: libcxxabi/src/cxa_guard_impl.h:217
+//                          then wait for the init byte to get updated with a new value.
+//      (PENDING|WAITING):  Somebeody has started the initialization already, and we're not the
+//                          first one waiting. Wait for the init byte to get updated.
----------------



================
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)) {}
----------------
DanielMcIntosh-IBM wrote:
> 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.
Got it, thanks for the explanation. Please consider reformatting as I suggested, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115368