Possible bug in std::allocate_shared implementation

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.

Hello,

Was it wrong to report the issue below on the mailing list, should I
have requested a bug tracker account and reported it there ? As I'm not
100% confident that this is a libc++ bug I was hoping to get feedback
here before requesting a bug tracker (as self-registration is disabled
due to spam).

Typically it's better to report bugs on the bug tracker.

-eric

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

class Private;

class Factory {
public:
static std::shared_ptr allocate();
};

class Private {
private:
friend class Factory;
Private() { }
~Private() { }
};

std::shared_ptr Factory::allocate()
{
struct Allocator : std::allocator {
void construct(void *p)
{
::new(p) Private();
}
void destroy(Private *p)
{
p->~Private();
}
};

return std::allocate_shared(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 :slight_smile:

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

Hi Eric,

> 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 :slight_smile:

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 :frowning: Would you mind elaborating a bit ?

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

And I can fix my code :slight_smile: