[PATCH] D69265: [scudo][standalone] Consolidate lists

[Mitch Phillips via Phabricator via llvm-commits]

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

LGTM with final comments.



================
Comment at: lib/scudo/standalone/list.h:17
+// Intrusive POD singly and doubly linked list.
 // An object with all zero fields should represent a valid empty list. clear()
 // should be called on all non-zero-initialized objects before using.
----------------
cryptoad wrote:
> hctim wrote:
> > cryptoad wrote:
> > > hctim wrote:
> > > > Why does the requirements to call `clear()` exist for non-zero-inited objects? For globally zero-init'ed instances of this object, we don't encounter a perf overhead of having `Size`, `First`, and `Last` be initialised to zero. For non-global instances, we have to call `clear()`, which seems easy to forget, and moving this initialisation into the c'tor doesn't cause any additional overhead (and I'd assume the call is elided, making it faster).
> > > > 
> > > > It also feels easy to forget to call `clear()`, and if any global objects are moved into static scopage, it may be incredibly hard to patch any instances where `clear()` isn't called.
> > > Part of the issue, even with `constexpr explicit` constructors is that `__thread` can't seem to deal with them (as opposed to `thread_local`) and complains about it needing "dynamic initialization". It circles back to the choices made with `init` & `initLinkerInitialized`.
> > > While your point makes sense, I don't think I can do it, unless I am missing something which is also very possible.
> > So I think I remember the same problem from `gwp_asan`, but it was solved for us by marking the c'tor as `constexpr`.
> > 
> > https://godbolt.org/z/j1q8uR
> > ^ works with constexpr c'tor with `__thread` and `thread_local`, fails for `__thread` if not `constexpr` c'tor. Sounds like this might be the same problem?
> I gave a shot earlier to `constexpr` as well without success.
> Using a `constexpr explicit` constructor for the lists, it bubbles up to the TSD:
> ```
> .../compiler-rt/lib/scudo/standalone/tsd_exclusive.h:93:28: error: non-local variable ‘scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig> >::ThreadTSD’ declared ‘__thread’ needs dynamic initialization
>  THREADLOCAL TSD<Allocator> TSDRegistryExT<Allocator>::ThreadTSD;
>                             ^~~~~~~~~~~~~~~~~~~~~~~~~
> .../compiler-rt/lib/scudo/standalone/tsd_exclusive.h:93:28: note: C++11 ‘thread_local’ allows dynamic initialization and destruction
> ```
> Part of the issue is probably that all of the TSD sub components as well as the TSD itself should all have `constexpr` constructors, but that's a rabbit hole I am not keen to look into at this moment.
I see. Might be worth adding a strong TODO to go and sprinkle `constexpr` around where necessary to make this more bullet proof, but I don't want to block you on this.


================
Comment at: lib/scudo/standalone/list.h:101
     if (empty()) {
       X->Next = nullptr;
+      Last = X;
----------------
if `empty()`, then `First == nullptr` as invariant,  can hoist  `X->Next = First` and this line out as well, right?


================
Comment at: lib/scudo/standalone/list.h:168
+    T *Prev = Y->Prev;
+    CHECK_EQ(Prev->Next, Y);
+    Prev->Next = X;
----------------
DCHECK?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D69265