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();
I think the issue is `Allocator` is not able to rebind through either a template or a rebind struct. See the allocator requirements: http://eel.is/c++draft/allocator.requirements#tab:utilities.allocator.requirements Because you cannot have a template class inside another class it means your `Private` class will not work as currently defined. I agree that the static_assert needs to either change or be removed. Maybe we can add one for rebindability.
(In reply to Zoe Carver from comment #1) > I think the issue is `Allocator` is not able to rebind through either a > template or a rebind struct. See the allocator requirements: > http://eel.is/c++draft/allocator.requirements#tab:utilities.allocator. > requirements > > Because you cannot have a template class inside another class it means your > `Private` class will not work as currently defined. Please pardon my lack of knowledge in this area, why is the std::allocator::rebind not sufficient in this case ?
Yes, you are right. That should satisfy the requirements. I misdiagnosed the issue because of the version of libc++ I am using (I made some modifications to std::allocator). Anyway, I think I found the issue (for real this time). std::allocate_shared passes the args (or no args in this case) to the constructor of T (in this case that is `Private`). That calls the private constructor which causes your issue. How do you get around this? There is also a shared_ptr constructor that accepts a pointer, a deleter, and an allocator. default_delete will run into the same issue because the destructor is private, but you can get around this as well. See the updated code below: std::shared_ptr<Private> Factory::allocate() { struct Allocator : std::allocator<Private> { void construct(void *p) { ::new(p) Private(); } void destroy(Private *p) { p->~Private(); } }; struct Deleter : std::default_delete<Private> { void operator()(Private* p) { delete p; } }; return std::shared_ptr<Private>(nullptr, Deleter(), Allocator()); } I provided shared_ptr a nullptr, but you can change that to a pointer you create using whatever method you want.
(In reply to Zoe Carver from comment #3) > Yes, you are right. That should satisfy the requirements. I misdiagnosed the > issue because of the version of libc++ I am using (I made some modifications > to std::allocator). Anyway, I think I found the issue (for real this time). > std::allocate_shared passes the args (or no args in this case) to the > constructor of T (in this case that is `Private`). That calls the private > constructor which causes your issue. Thinking more about this though, and trying to understand rebind, the rebind member is defined in http://eel.is/c++draft/allocator.requirements#tab:utilities.allocator.requirements as "typename X::template rebind<U>::other" returning a Y, where for all U (including T), Y::template rebind<T>::other is X. As my struct Allocator inherits from std::allocator<Private>, tn the forward direction std::allocator<Private>::rebind<U>::other will return an std::allocator<U>, which is fine. However, in the reverse direction, std::allocator<U>::rebind<Private>::other will return an std::allocator<Private>, which can't allocate an instance of Private due to the private constructor and destructor. My allocator thus violates the allocator requirements, I would need to provide a rebind member where rebind<U>::other::rebind<Private>::other would return an Allocator, not an std::allocator<Private>. I tried this (moving everything to the global scope as I can't define template structures in a function) and miserably failed :-) struct Allocator; template<typename T> struct OtherAllocator : std::allocator<T> { template<class U> struct rebind; }; template<typename T> template<class U> struct OtherAllocator<T>::rebind { typedef OtherAllocator<U> other; }; template<typename T> template<> struct OtherAllocator<T>::rebind<Private> { typedef Allocator other; }; struct Allocator : std::allocator<Private> { template<class U> struct rebind { typedef std::allocator<U> other; }; void construct(Private *p) { ::new(p) Private(); } void destroy(Private *p) { p->~Private(); } }; The compiler, rightfully I believe, complained that I "cannot specialize (with 'template<>') a member of an unspecialized template". There is nothing in the C++ standard, as far as I can tell, that requires std::allocate_shared<T> to not construct the object by rebinding the allocator to a class that inherits from T (or embeds a T member), as libc++ does, and nothing the requires it to do so, so this may be a grey area. Provided that I could provide an allocator implementation that satisfies the rebind constraint, libc++ should accept it and allow me to construct the Private instance with allocate_shared. However, I don't know if providing such an allocator is feasible. Is this an unsolvable problem ? > How do you get around this? There is also a shared_ptr constructor that > accepts a pointer, a deleter, and an allocator. default_delete will run into > the same issue because the destructor is private, but you can get around > this as well. See the updated code below: > > std::shared_ptr<Private> Factory::allocate() > { > struct Allocator : std::allocator<Private> { > void construct(void *p) > { > ::new(p) Private(); > } > void destroy(Private *p) > { > p->~Private(); > } > }; > > struct Deleter : std::default_delete<Private> { > void operator()(Private* p) > { > delete p; > } > }; > > > return std::shared_ptr<Private>(nullptr, Deleter(), Allocator()); > } > > I provided shared_ptr a nullptr, but you can change that to a pointer you > create using whatever method you want. But that won't solve my issue, as I need to use allocate_shared in order to later use shared_from_this. Otherwise I would have simply gone for std::shared_ptr<Private> Factory::allocate() { return std::shared_ptr<Private>(new Private()); }
(In reply to Laurent Pinchart from comment #4) > (In reply to Zoe Carver from comment #3) > > How do you get around this? There is also a shared_ptr constructor that > > accepts a pointer, a deleter, and an allocator. default_delete will run into > > the same issue because the destructor is private, but you can get around > > this as well. See the updated code below: > > > > std::shared_ptr<Private> Factory::allocate() > > { > > struct Allocator : std::allocator<Private> { > > void construct(void *p) > > { > > ::new(p) Private(); > > } > > void destroy(Private *p) > > { > > p->~Private(); > > } > > }; > > > > struct Deleter : std::default_delete<Private> { > > void operator()(Private* p) > > { > > delete p; > > } > > }; > > > > > > return std::shared_ptr<Private>(nullptr, Deleter(), Allocator()); > > } > > > > I provided shared_ptr a nullptr, but you can change that to a pointer you > > create using whatever method you want. > > But that won't solve my issue, as I need to use allocate_shared in order to > later use shared_from_this. Otherwise I would have simply gone for > > std::shared_ptr<Private> Factory::allocate() > { > return std::shared_ptr<Private>(new Private()); > } This being said, in my specific case, I think I can make the private object inherit from std::shared_from_this and use you above solution, minus the Allocator that isn't needed. std::shared_ptr<Private> Factory::allocate() { struct Deleter : std::default_delete<Private> { void operator()(Private* p) { delete p; } }; return std::shared_ptr<Private>(new Private(), Deleter()); } This doesn't fix the issue we're discussing, but works around it in my case.
> But that won't solve my issue, as I need to use allocate_shared in order to later use shared_from_this. According to the standard, those constructors will enable `shared_from_this`: http://eel.is/c++draft/util.smartptr.shared#const-1 > This doesn't fix the issue we're discussing, but works around it in my case. Just to be clear, the issue is that `construct` is not being used to initialize the object, yes?
(In reply to Zoe Carver from comment #6) > > But that won't solve my issue, as I need to use allocate_shared in order to later use shared_from_this. > > According to the standard, those constructors will enable > `shared_from_this`: http://eel.is/c++draft/util.smartptr.shared#const-1 > > > This doesn't fix the issue we're discussing, but works around it in my case. > > Just to be clear, the issue is that `construct` is not being used to > initialize the object, yes? The issue is tha clang++ fails to compile the test program, because the libc++ implementation of std::allocate_shared doesn't support objects with private constructors or destructors. The use of a custom allocator wasn't so much to allocate memory in any special way, but to provide std::allocate_shared with an object that can call the private constructor.
Patch is up: https://reviews.llvm.org/D62760
I at the moment stumble over the same issue, but with a protected constructor. The usage of the allocator seems to work just fine here, but the static assert already makes that impossible. If I just remove the static_assert, the code compiles and works for me as it should (and does for libstdc++). Would it be possible as a first step to remove that static_assert again?
I must confess that was a bit too fast ;=) Using the right file one runs into the issue that the compressed_pair construction runs into memory:2155:9: error: field of type 'test' has protected constructor : __value_(_VSTD::forward<_Args>(_VSTD::get<_Indexes>(__args))...) {} inside the instantiation of memory:2258:9: note: in instantiation of function template specialization 'std::__1::__compressed_pair_elem
This should have been fixed by https://reviews.llvm.org/D91201. Please reopen if you still see the original issue (or another issue!). commit 955dd7b7f3f6df79f573508ffb567f3923e892f7 Author: Louis Dionne <ldionne.2@gmail.com> Date: Fri Dec 11 12:22:16 2020 -0500 [libc++] LWG2070: Use Allocator construction for objects created with allocate_shared This patch updates `allocate_shared` to call `allocator_traits::construct` when creating the object held inside the shared_pointer, and `allocator_traits::destroy` when destroying it. This resolves the part of P0674R1 that was originally filed as LWG2070. This change is landed separately from the rest of P0674R1 because it is incredibly tricky from an ABI perspective. This is the reason why this change is so tricky is that we previously used EBO in a compressed pair to store both the allocator and the object type stored in the `shared_ptr`. However, starting in C++20, P0674 requires us to use Allocator construction for initializing the object type. That requirement rules out the use of the EBO for the object type, since using the EBO implies that the base will be initialized when the control block is initialized (and hence we can't do it through Allocator construction). Hence, supporting P0674 requires changing how we store the object type inside the control block, which we do while being ABI compatible by using some trickery with a properly aligned char buffer. Fixes https://llvm.org/PR41900 Supersedes https://llvm.org/D62760 Differential Revision: https://reviews.llvm.org/D91201