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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver 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));
+        }
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91201