[libcxx-commits] [PATCH] D91201: [libc++] LWG2070: Use Allocator construction for objects created with allocate_shared

Zoe Carver via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri Jan 8 09:18:36 PST 2021


zoecarver accepted this revision.
zoecarver added a comment.

This looks great. Thanks for spending so much time iterating on this patch!



================
Comment at: libcxx/include/memory:2626
+    using _CompressedPair = __compressed_pair<_Alloc, _Tp>;
+    struct _ALIGNAS_TYPE(_CompressedPair) _Storage {
+        char __blob_[sizeof(_CompressedPair)];
----------------
ldionne wrote:
> zoecarver wrote:
> > Alternatively, you could make `__blob_` a `std::aligned_storage`. 
> I used to love `aligned_storage`, and then I read http://open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1413r2.pdf. I'm not sure I agree entirely with the paper, but I've been moving away from the type since it seemed like there were efforts to deprecate it. I'm not sure where those efforts ended up, though.
The reason I liked using it in libc++ is that it works in C++03 mode (and alignas / alignof didn't use to). But now that we don't need to use it, I'm on board with moving away. 

Thanks for the paper link :)


================
Comment at: libcxx/include/memory:2630
+        _LIBCPP_HIDE_FROM_ABI explicit _Storage(_Alloc&& __a) {
+            ::new ((void*)__get_alloc()) _Alloc(_VSTD::move(__a));
+        }
----------------
ldionne wrote:
> zoecarver wrote:
> > zoecarver wrote:
> > > ldionne wrote:
> > > > zoecarver wrote:
> > > > > (This is a new comment, I'm not replying to an old comment, even though that's what it looks like.)
> > > > > 
> > > > > Isn't this UB? `__blob_` is uninitialized when you call `.first()` on it in `__get_alloc()`.
> > > > Yes, it is indeed UB (hopefully benign UB). I ran it through ubsan before updating the patch and it didn't complain, so I thought to myself "it's good enough". But you're entirely right, it *is* plain and simple UB.
> > > > 
> > > > The issue is that I haven't been able to find a way to implement it without that. Simply, what we want is this:
> > > > 
> > > > ```
> > > > _Alloc* __get_alloc() _NOEXCEPT {
> > > >     std::ptrdiff_t __offset = _CompressedPair::__first_offset(); // how to compute this?
> > > >     _Alloc* __alloc = static_cast<_Alloc*>(&__blob_ + __offset);
> > > >     return __alloc;
> > > > }
> > > > ```
> > > > 
> > > > I wasn't able to find an implementation that didn't actually require an object to be alive:
> > > > 
> > > > ```
> > > > _LIBCPP_INLINE_VISIBILITY
> > > > static std::ptrdiff_t __first_offset() _NOEXCEPT {
> > > >     __compressed_pair __p; // also requires default-constructing, which doesn't work
> > > >     _T1 const* __first = _VSTD::addressof(__p.first());
> > > >     return static_cast<char const*>(__first) - static_cast<char const*>(&__p);
> > > > }
> > > > ```
> > > > 
> > > > I tried something with `offsetof` inside `__compressed_pair_elem`, but I couldn't compute the offset of the second element of the compressed pair. I don't know whether it's impossible or whether I'm just not imaginative enough. If we can't think of something, I can go back to the `no_unique_address` implementation (it's just a bit more complicated, but it's correct AFAICT).
> > > I actually really liked the `no_unique_address` implementation. And I think at this point we only support (or should only support) compiler that have implemented `no_unique_address` all the way back to C++03. 
> > > 
> > > That being said, there is a way to do what you're thinking. We discussed this earlier, but I don't think I did a good job explaining. Basically, here's the idea: `__compressed_pair` *always* inherits from two `__compressed_pair_elem`s. Always. We can simply static_cast a `__compressed_pair` pointer to both of it's base classes. That will give you two pointers which you can use to do placement new on both members. 
> > > 
> > > (I think that might still technically be UB, because you never actually construct the object, but it seems like better UB then calling a member on an uninitialized object, which seems fairly dangerous IMHO.)
> > Here's a pseudo-code example:
> > 
> > ```
> > _Alloc* __get_alloc() _NOEXCEPT {
> >     _CompressedPair *__as_pair = reinterpret_cast<_CompressedPair*>(__blob_);
> >     _CompressedPair::_Base1* __first = static_cast<_CompressedPair::_Base1*>(__as_pair);
> >     _Alloc *__alloc = reinterpret_cast<_Alloc*>(__first);
> >     return __alloc;
> > }
> > ```
> Hmm, that's clever, and actually very simple. Do you agree to go forward with that implementation, then?
> 
> Working with you is very humbling :-).
Yes, I'm happy to go forward with that. 

Thanks, you're too kind :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91201



More information about the libcxx-commits mailing list