<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 29, 2019 at 5:28 AM Laurent Pinchart via libcxx-dev <<a href="mailto:libcxx-dev@lists.llvm.org" target="_blank">libcxx-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
I ran into what I believe can be a bug in the libc++ implementation of<br>
std::allocate_shared<>. The following code fails to compile due to two<br>
issues.<br>
<br>
------------------------------------------------------------------------<br>
#include <memory><br>
<br>
class Private;<br>
<br>
class Factory {<br>
public:<br>
static std::shared_ptr<Private> allocate();<br>
};<br>
<br>
class Private {<br>
private:<br>
friend class Factory;<br>
Private() { }<br>
~Private() { }<br>
};<br>
<br>
std::shared_ptr<Private> Factory::allocate()<br>
{<br>
struct Allocator : std::allocator<Private> {<br>
void construct(void *p)<br>
{<br>
::new(p) Private();<br>
}<br>
void destroy(Private *p)<br>
{<br>
p->~Private();<br>
}<br>
};<br>
<br>
return std::allocate_shared<Private>(Allocator());<br>
}<br>
------------------------------------------------------------------------<br>
<br>
First of all, commit 7700912976a5 ("Land D28253 which fixes PR28929<br>
(which we mistakenly marked as fixed before)") introduced a static<br>
assert to verify that the object type is constructible:<br>
<br>
static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared" );<br>
<br>
This fails, as Private is not constructible in the context of the<br>
std::allocate_shared<> implementations, due to its private constructor<br>
and destructor. I believe that the check is incorrect, as I don't see<br>
anything in the C++ standard that mandates the object type to be<br>
constructible by std::allocate_shared<> itself.<br>
<br>
I tried removing the static asserts, and that's where the real fun<br>
began. If my understanding is correct, in order to store the pointed<br>
data, libc++ uses an internal __shared_ptr_emplace<_Tp, _Alloc> class<br>
that contains (through a few levels of inheritance) an instance of _Tp.<br>
That class has its destructor implicitly deleted due to the private<br>
nature of ~Private(). The compiler then complains about<br>
<br>
/usr/include/c++/v1/memory:3604:7: error: deleted function '~__shared_ptr_emplace' cannot override a non-deleted function<br>
class __shared_ptr_emplace<br>
[...]<br>
/usr/include/c++/v1/memory:3513:13: note: overridden virtual function is here<br>
virtual ~__shared_weak_count();<br>
<br>
My limited knowledge of libc++ internals and of C++ compilers in general<br>
prevents me from proposing a fix. I would appreciate if someone could<br>
first confirm that the test case is valid, and then hopefully help :-)<br></blockquote><div><br></div><div>There's bugs everywhere with this one. The standard, this reproducer, and of course libc++.</div><div><br></div><div>First, libc++ should be doing roughly what you're suggesting. It's going to be tricky to fix this behavior</div><div>without breaking the ABI. The embedded control block stores `Private` as a normal data member so</div><div>it's constructed when the control block is. The fix will have to replace that data member with a equivalent</div><div>storage buffer without changing the triviality of the embedded control block and without introducing</div><div>ODR violations between the old and new implementations.<br></div><div class="gmail_quote"><br></div>The standard also has a bunch of bugs. It doesn't seem to understand how allocator construction works,</div><div class="gmail_quote">and incorrectly tells the implementation to pass `pv` (assumed to be "pointer to void"), which asks the</div><div class="gmail_quote">allocator to do `::new ((void*)pv) void(args...)`. It also specifies that the allocator used for</div><div class="gmail_quote">construction is rebound which is unnecessary. But we can do what the standard means and not what it says.</div><div class="gmail_quote"><br></div><div class="gmail_quote">The bug in the provided test case both that it provides a construct method taking `void*` but constructs `Private*`,</div><div class="gmail_quote">but also that the `Allocator` type doesn't rebind correctly. At best it picks up `std::allocator<T>::rebind` which</div><div class="gmail_quote">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</div><div class="gmail_quote">this.</div><div class="gmail_quote"><br></div><div class="gmail_quote">In short everything is terrible.</div><div class="gmail_quote">But we can at least fix libc++.<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">/Eric</div><div class="gmail_quote"><br><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
_______________________________________________<br>
libcxx-dev mailing list<br>
<a href="mailto:libcxx-dev@lists.llvm.org" target="_blank">libcxx-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-dev</a><br>
</blockquote></div></div>