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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked an inline comment as done.
cryptoad added inline 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.
----------------
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.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D69265