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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/memory:2630
+        _LIBCPP_HIDE_FROM_ABI explicit _Storage(_Alloc&& __a) {
+            ::new ((void*)__get_alloc()) _Alloc(_VSTD::move(__a));
+        }
----------------
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 :-).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91201