[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
Thu Jan 7 14:52:59 PST 2021


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


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