[libcxx-dev] Possible bug in std::allocate_shared implementation

[Laurent Pinchart via libcxx-dev]

Hi Eric,

On Mon, May 20, 2019 at 07:50:51PM -0400, Eric Fiselier wrote:
> On Mon, Apr 29, 2019 at 5:28 AM Laurent Pinchart via libcxx-dev wrote:
> > Hello,
> > 
> > I ran into what I believe can be a bug in the libc++ implementation of
> > std::allocate_shared<>. The following code fails to compile due to two
> > issues.
> > 
> > ------------------------------------------------------------------------
> > #include <memory>
> > 
> > class Private;
> > 
> > class Factory {
> > public:
> >         static std::shared_ptr<Private> allocate();
> > };
> > 
> > class Private {
> > private:
> >         friend class Factory;
> >         Private() { }
> >         ~Private() { }
> > };
> > 
> > std::shared_ptr<Private> Factory::allocate()
> > {
> >         struct Allocator : std::allocator<Private> {
> >                 void construct(void *p)
> >                 {
> >                         ::new(p) Private();
> >                 }
> >                 void destroy(Private *p)
> >                 {
> >                         p->~Private();
> >                 }
> >         };
> > 
> >         return std::allocate_shared<Private>(Allocator());
> > }
> > ------------------------------------------------------------------------
> > 
> > First of all, commit 7700912976a5 ("Land D28253 which fixes PR28929
> > (which we mistakenly marked as fixed before)") introduced a static
> > assert to verify that the object type is constructible:
> > 
> > static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct
> > object in allocate_shared" );
> > 
> > This fails, as Private is not constructible in the context of the
> > std::allocate_shared<> implementations, due to its private constructor
> > and destructor. I believe that the check is incorrect, as I don't see
> > anything in the C++ standard that mandates the object type to be
> > constructible by std::allocate_shared<> itself.
> > 
> > I tried removing the static asserts, and that's where the real fun
> > began. If my understanding is correct, in order to store the pointed
> > data, libc++ uses an internal __shared_ptr_emplace<_Tp, _Alloc> class
> > that contains (through a few levels of inheritance) an instance of _Tp.
> > That class has its destructor implicitly deleted due to the private
> > nature of ~Private(). The compiler then complains about
> > 
> > /usr/include/c++/v1/memory:3604:7: error: deleted function
> > '~__shared_ptr_emplace' cannot override a non-deleted function
> > class __shared_ptr_emplace
> > [...]
> > /usr/include/c++/v1/memory:3513:13: note: overridden virtual function is
> > here
> >     virtual ~__shared_weak_count();
> > 
> > My limited knowledge of libc++ internals and of C++ compilers in general
> > prevents me from proposing a fix. I would appreciate if someone could
> > first confirm that the test case is valid, and then hopefully help :-)
> 
> There's bugs everywhere with this one. The standard, this reproducer, and of
> course libc++.
> 
> First, libc++ should be doing roughly what you're suggesting. It's
> going to be tricky to fix this behavior without breaking the ABI. The
> embedded control block stores `Private` as a normal data member so
> it's constructed when the control block is. The fix will have to
> replace that data member with a equivalent storage buffer without
> changing the triviality of the embedded control block and without
> introducing ODR violations between the old and new implementations.

I also concluded it would likely be tricky to fix this, and the fix
likely doesn't fit my C++ and libc++ knowledge, which is why this bug
report unfortunately didn't come with a patch. Thank you for looking
into it, I really appreciate.

By the way, I've filed a bug report for this, available at
https://bugs.llvm.org/show_bug.cgi?id=41900

> The standard also has a bunch of bugs. It doesn't seem to understand
> how allocator construction works, and incorrectly tells the
> implementation to pass `pv` (assumed to be "pointer to void"), which
> asks the allocator to do `::new ((void*)pv) void(args...)`. It also
> specifies that the allocator used for construction is rebound which is
> unnecessary. But we can do what the standard means and not what it
> says.
> 
> The bug in the provided test case both that it provides a construct method
> taking `void*` but constructs `Private*`,

Oops, my bad, I'll fix that.

> but also that the `Allocator` type doesn't rebind correctly. At best
> it picks up `std::allocator<T>::rebind` which doesn't round-trip when
> you try to bind it back to `Private`. And the bug in libstdc++ is that
> it doesn't seem to notice this.

I'm failing to understand this :-( Would you mind elaborating a bit ?

> 
> In short everything is terrible.
> But we can at least fix libc++.

And I can fix my code :-)

-- 
Regards,

Laurent Pinchart