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

[Eric Fiselier via libcxx-dev]

On Mon, Apr 29, 2019 at 5:28 AM Laurent Pinchart via libcxx-dev <
libcxx-dev at lists.llvm.org> 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.

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*`,
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.

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

/Eric





>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcxx-dev mailing list
> libcxx-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-dev/attachments/20190520/87550c2b/attachment.html>