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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
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 {
----------------
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.


================
Comment at: libc/src/stdlib/CMakeLists.txt:280
+  COMPILE_OPTIONS
+    -O3 # To get the effect of constexpr destructors of global objects.
   DEPENDS
----------------
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`.


================
Comment at: libc/src/stdlib/atexit.cpp:41
 
-LLVM_LIBC_FUNCTION(int, atexit, (void (*function)())) {
+LLVM_LIBC_FUNCTION(int, atexit, (AtExitCallback * callback)) {
   handler_list_mtx.lock();
----------------
abrachet wrote:
> I think this is how we would normally format this. But if `clang-format` get's confused because of the parentheses then we should just go with what `clang-format` says.
Yes, that was done by clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121350