[libc-commits] [PATCH] D121350: [libc] Add a resizable container with constexpr constructor and destructor.

Thu Mar 10 09:52:04 PST 2022

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

Thanks, this is much more correct than what I had and more performant too.

================
Comment at: libc/src/__support/CPP/blobstore.h:31
+// on its value will be optimized out in the code below.
+template <typename T, size_t BLOB_SIZE, bool REVERSE_ORDER = false>
+class BlobStore {
----------------
sivachandra wrote:
> abrachet wrote:
> > sivachandra wrote:
> > > abrachet wrote:
> > > > Could we just have a `reverse_iterator [rbegin|rend]()` instead of this `REVERSE_ORDER` template parameter? Maybe it makes more sense for `BlobStore<T>::iterator` to have `REVERSE_ORDER`?
> > > If we have to support bidirectional iteration, then we need a doubly-linked list. Not sure if we will ever need bidirectional iteration so I have chosen to keep it a simple linked list with the direction attached to the type.
> > > 
> > > > Maybe it makes more sense for `BlobStore<T>::iterator` to have `REVERSE_ORDER`?
> > > 
> > > Do you mean, it enables range-for-loop? Or, something else?
> > > 
> > No strong preference because we would need to add one pointer overhead with a new `prev` pointer to `Blob`, but it would be much more ergonomic if `BlobStore` could iterate both ways.
> > 
> > `REVERSE_ORDER` could be changed to only be used in the iterator. So maybe it could be like
> > ```
> > template <bool REVERSE_ORDER> class iterator_impl { ... };
> > using iterator = iterator_impl<false>;
> > using reverse_iterator = iterator_impl<true>;
> > iterator rbegin() {
> >   return reverse_iterator(current, fill_count);
> > }
> > ...
> > ```
> > 
> I agree that having a forward and reverse iterator would be aesthetically/"ergonomically" ideal. Also, while I do not see today as to why we would need bidirectional iteration, I won't be surprised we actually make it a doubly linked list and allow bidirectional iteration in future. But, until then, I personally prefer to keep it restrictive.
Sounds good.

================
Comment at: libc/src/stdlib/CMakeLists.txt:280
+  COMPILE_OPTIONS
+    -O3 # To get the effect of constexpr destructors of global objects.
   DEPENDS
----------------
sivachandra wrote:
> abrachet wrote:
> > sivachandra wrote:
> > > abrachet wrote:
> > > > From what I understand, as long as `T` is POD and `BlobStore<T>::~BlobStore() is defaulted, then there shouldn't be a destructor emitted because then BlobStore will be a POD too. Do you see `__cxa_atexit` being called in this TU?
> > > > 
> > > > We could in the future when we add an analogue to `is_trivially_destructible` in our type_traits, add `static_assert(cpp::IsTriviallyDestructibleV<ExitCallbackList>)`
> > > You are correct. I think I misread or checked it with the previous mutex handling still present that I did see a call to `__cxa_atexit`. I don't see it now.
> > > 
> > > That said, in a debug build, I surprisingly see a call to the constructor (via `__cxx_global_var_init`) if I don't use -O3 [may be -O2 is sufficient, I did not check]. In a release build, everything works as expected without -O3 - I don't see calls to the constructor or `__cxa_atexit`. So, I have kept the -O3 for now.
> > > That said, in a debug build, I surprisingly see a call to the constructor (via `__cxx_global_var_init`) if I don't use -O3
> > We just bumped up to C++17 and we already need C++20's `constinit`! :)
> > 
> > Specifying `O3` here makes this TU basically impossible to debug and generally limits options. Maybe we want to optimize hard for size with `Oz` but atexit will always be `O3`.
> > 
> > Are global constructors something we need to avoid? The destructor was going to affect correctness, but unless I'm missing something the same isn't true for constructors. Is it worth the downside above if this will be optimized away during release builds anyway?
> > 
> The high level design goal is that, the runtime should iterate over init array (which is how the constructors will get called) and fini array to facilitate certain language features. If the runtime itself starts requiring them, we will soon end up with chicken and egg problems.
> 
> We can make an exception here because it is affecting debug builds only. But, a difference in behavior between debug and release builds will be hard to debug when we have to.
> 
> There are two options here - use C++ 20 (for `constinit`) for this TU or use a higher optimization level for this TU. I personally do not have problems with either of them. By the time this gets used in real production systems, C++ 20 will be acceptable. I preferred `-O3` because I am not sure if our bots will like the C++ 20 feature. I have changed now to use C++ 20 instead of `-O3`. If the bots don't like it, I can switch back to `-O3`.
You're right, it's actually very likely that a C++ object will have it's constructor and __cxa_atexit registration before this constructor is called. I don't know why I didn't see that... There is no way for us to control the order .init_array elements will be ordered across TU's so yes we need to make sure `exit_callbacks` is constructed statically.

I was actually joking about `constinit`, but it might be our best option here. `O3` doesn't guarantee that this object will be constructed statically. I was thinking this would be easy enough to optimize that any compiler would do it and it would be as good as a guarantee, but actually GCC doesn't at any optimization level but clang get's it even at `O1`. https://godbolt.org/z/4Pv3TdG77

Using a C++20 compiler isn't that big of an ask because there's one in the same repo as this, and we could recommend the runtimes build for this case. Let's discuss further offline

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121350