This is an archive of the discontinued LLVM Phabricator instance.

Differential D62760

allocate_shared should call allocator_traits::construct
AbandonedPublic

Authored by zoecarver on May 31 2019, 4:36 PM.

Details

Reviewers
mclow.lists
EricWF
ldionne
Group Reviewers
Restricted Project
Summary

This patch updates allocate_shared to call allocator_traits::construct. Fixes issue 41900. Based on D62233.

Note: __shared_ptr_aligned_block is very similar to __shared_ptr_array_block from D62641 (but with support for custom pointers).

Currently, I have not transferred the changes so the C++03 overloads. Once this has been reviewed a bit, then I will update those.

Diff Detail

Event Timeline

zoecarver created this revision.May 31 2019, 4:36 PM
Herald added subscribers: libcxx-commits, dexonsmith. · View Herald TranscriptMay 31 2019, 4:36 PM
zoecarver updated this revision to Diff 202507.May 31 2019, 4:37 PM
  • update fail tests
Herald added a subscriber: christof. · View Herald TranscriptMay 31 2019, 4:37 PM
zoecarver mentioned this in D62641: Support arrays in make_shared and allocate_shared (P0674R1).Jun 6 2019, 6:41 PM
zoecarver updated this revision to Diff 263861.EditedMay 13 2020, 2:40 PM
zoecarver edited the summary of this revision. (Show Details)
  • Fix C++03 tests
  • Rebase off master
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 2:40 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: broadwaylamb. · View Herald Transcript
zoecarver added a comment.May 13 2020, 2:43 PM

@ldionne ping. This patch has most of the boilerplate needed for D62641/P0674R1.

Harbormaster failed remote builds in B56672: Diff 263861!May 13 2020, 4:24 PM
ldionne requested changes to this revision.May 14 2020, 5:10 AM
ldionne added inline comments.
libcxx/include/memory
3506

Wait, so you have an array of sizeof(ControlBlock) elements of aligned storage with sizeof(ControlBlock)? That's sizeof(ControlBlock) * sizeof(ControlBlock) bytes of storage. Why do you need that?

3511

Similar comment but with sizeof(_Tp) * sizeof(_Tp). I also don't understand why this needs to be aligned to alignment_of<ControlBlock>. Can you explain?

3611

Isn't this an ABI break? It's not clear to me why we need to store the size -- we should always be calling allocator.deallocate with one instance of sizeof(__shared_ptr_pointer), no?

4547

These variable names (like _D1) are not really helpful. I know there weren't better before, but it would be easier to read with a better name.

4549

Shouldn't this be <_Tp*, _D1, _TAlloc> instead of <_Tp*, _D1, _Alloc>? If not, why?

4566

__create_with_control_block needs to be marked as noexcept if you want to do this and make it clear that it's safe. Otherwise we could leak __hold2 in case an exception was thrown from within __create_with_control_block.

libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp
41

This expected-error is brittle, it won't work if we e.g. change the ABI namespace.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp
2

Could you add a test (in another test file) with the reproducer for https://bugs.llvm.org/show_bug.cgi?id=41900, with both private and protected constructors? IIUC, the intent of this patch was primarily to fix that.

17

Please explain why this needs to be tested in the comment. It could be as simple as "[...] as requested in C++20", which is the case I think.

110

Please fix this todo. It's fine to assume that C++03 has variadics, cause we only support C++03 with Clang, which supports variadics.

This revision now requires changes to proceed.May 14 2020, 5:10 AM
zoecarver marked 8 inline comments as done.May 14 2020, 9:58 AM

Thanks for the review!

libcxx/include/memory
3506

🤦‍♂️ Oops. Good catch. I originally had alignas(_CntrlBlk) char __cntrl_buff[sizeof(_CntrlBlk)] which didn't work in C++03 mode so, I used aligned_storage and seem to have forgotten to remove the array part.

3611

I was worried this might be an ABI break.

We need size, though. Because we need to know how big __shared_ptr_aligned_block is (and later we'll need to know how big __shared_ptr_aligned_block + elements is).

sizeof(__shared_ptr_pointer) would just be the control block. If we knew that __shared_ptr_pointer would only be used by allocate_shared we could do something like sizeof(__shared_ptr_aligned_block< __shared_ptr_pointer , remove_pointer_t<_Tp>>). To do that we'd probably have to duplicate __shared_ptr_pointer which would be an ABI break too, I think.

4547

You're right. I'll make these more clear.

4549

It doesn't really matter because it gets rebound to char before being called. I'll put a comment here.

4566

OK, I'll make that a separate patch, though. __create_with_control_block was already used here.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp
2

Yes. I'll add a test for that. Good call.

17

Huh, you're right. I didn't realize this was since C++20. I'll add the comment.

110

Yes, I'll fix this. I want to make it a separate patch because it will be a fairly large change.

zoecarver updated this revision to Diff 264036.May 14 2020, 11:07 AM
zoecarver marked 6 inline comments as done.
  • Fix based on review
zoecarver marked an inline comment as done.May 14 2020, 11:09 AM
zoecarver added inline comments.
libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp
131

Because of construct is currently implemented, this only works when passed exactly one argument. I'll fix that when I remove the rebind variadics.

zoecarver updated this revision to Diff 264037.May 14 2020, 11:10 AM
  • Format
Harbormaster failed remote builds in B56757: Diff 264036!May 14 2020, 12:31 PM
Harbormaster failed remote builds in B56758: Diff 264037!
ldionne added inline comments.May 15 2020, 6:11 AM
libcxx/include/memory
3658

Can you explain why, in this patch, using __a.deallocate(_PTraits::pointer_to(*this), 1); is not sufficient?

4542–4544

This should either be there, or not be there. We don't leave commented-out stuff in the code :-)

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp
17

Is this a DR that was back-ported to older standards, or should this "feature" only work in C++20? If so, I would expect something like UNSUPPORTED: c++03, c++11, c++14, c++17.

110

I guess my point is that you can always assume variadics, even in C++03 mode. We can remove any attempt to emulate variadics in C++03 prior to applying this patch.

ldionne requested changes to this revision.May 15 2020, 7:08 AM

Requesting changes so it shows up properly in my review queue.

This revision now requires changes to proceed.May 15 2020, 7:08 AM
zoecarver marked 4 inline comments as done.May 16 2020, 1:18 PM
zoecarver added inline comments.
libcxx/include/memory
3658

Because _Al is rebound to char type and used in _ATraits which is used to get the pointer type for _PTraits so, _PTraits:: pointer_to will be expecting a char*.

4542–4544

Oops. I thought I removed this.

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp
17

According to cppreference, it should use placement new before C++20 (FWIW that seems a bit odd to me). I'll update this to use placement new before C++20.

110

OK good to know. I'll submit a patch for that today.

zoecarver updated this revision to Diff 264452.May 16 2020, 1:46 PM
zoecarver marked an inline comment as done.
  • Don't call construct before C++20
  • Remove static_assert
Harbormaster failed remote builds in B56971: Diff 264452!May 16 2020, 2:49 PM
ldionne requested changes to this revision.May 19 2020, 6:07 AM
ldionne added inline comments.
libcxx/include/memory
3658

But why do you need to rebind it to char?

This revision now requires changes to proceed.May 19 2020, 6:07 AM
zoecarver marked an inline comment as done.May 19 2020, 5:42 PM
zoecarver added inline comments.
libcxx/include/memory
3658

What other type would you bind it to? Sometimes we're going to need to deallocate sizeof(__shared_ptr_pointer) but sometimes it will need to be sizeof(__shared_ptr_aligned_block< __shared_ptr_pointer , remove_pointer_t<_Tp>>) and later it will need to be that plus the size and number of elements we allocated. We don't really know.

A temporary alternative could be adding another template argument which the allocator could rebind to but, we'd need to add back size in D62641 (to support unbounded array types).

zoecarver added a comment.Nov 3 2020, 2:23 PM

Friendly ping @ldionne.

libcxx/include/memory
3611

Is there a way to verify that this is in fact an ABI break? If this is an ABI break, I have an idea about how to implement this by storing the size at the beginning of __shared_ptr_aligned_block but, I think that will add a lot of complexity, so this is (IMHO) a much better implementation if we can get away with it.

ldionne added a child revision: D62641: Support arrays in make_shared and allocate_shared (P0674R1).Nov 10 2020, 6:56 AM
ldionne mentioned this in D91201: [libc++] LWG2070: Use Allocator construction for objects created with allocate_shared.Nov 10 2020, 12:25 PM
ldionne added a comment.Nov 10 2020, 12:29 PM

Zoe and I discussed this offline, but saying here for watchers. I had to dive deep into this change because it is pretty complicated and I did not fully understand it. After playing around, I now understand why some things were done that way (e.g. why aligned_storage is being used) -- that is to avoid using __compressed_pair, which breaks the use case of private destructors.

However, I've come up with what I believe is a simpler approach to making this change in D91201. Zoe and I will be discussing that tomorrow, and we'll see what approach we end up taking (or perhaps a hybrid).

zoecarver abandoned this revision.Dec 17 2020, 4:39 PM
zoecarver removed a child revision: D62641: Support arrays in make_shared and allocate_shared (P0674R1).Feb 21 2021, 12:32 PM

Revision Contents

PathSize
libcxx/
include/
80 lines
test/
libcxx/
utilities/
memory/
util.smartptr/
util.smartptr.shared/
2 lines
std/
utilities/
memory/
util.smartptr/
util.smartptr.shared/
util.smartptr.shared.create/
25 lines
189 lines

Diff 264452

libcxx/include/memory

Show First 20 Lines • Show All 3,185 Lines • ▼ Show 20 Linespublic:
_LIBCPP_INLINE_VISIBILITY __allocator_destructor(_Alloc& __a, size_type __s) _LIBCPP_INLINE_VISIBILITY __allocator_destructor(_Alloc& __a, size_type __s)
_NOEXCEPT _NOEXCEPT
: __alloc_(__a), __s_(__s) {} : __alloc_(__a), __s_(__s) {}
_LIBCPP_INLINE_VISIBILITY _LIBCPP_INLINE_VISIBILITY
void operator()(pointer __p) _NOEXCEPT void operator()(pointer __p) _NOEXCEPT
{__alloc_traits::deallocate(__alloc_, __p, __s_);} {__alloc_traits::deallocate(__alloc_, __p, __s_);}
}; };
template <class _Alloc>
class __allocator_destroyer
{
typedef allocator_traits<_Alloc> __alloc_traits;
_Alloc __alloc_;
public:
_LIBCPP_INLINE_VISIBILITY __allocator_destroyer(_Alloc const& __a)
_NOEXCEPT
: __alloc_(__a) {}
template<class _Tp>
_LIBCPP_INLINE_VISIBILITY
void operator()(_Tp* __p) _NOEXCEPT
{__alloc_traits::destroy(__alloc_, __p);}
};
template <class _InputIterator, class _ForwardIterator> template <class _InputIterator, class _ForwardIterator>
_ForwardIterator _ForwardIterator
uninitialized_copy(_InputIterator __f, _InputIterator __l, _ForwardIterator __r) uninitialized_copy(_InputIterator __f, _InputIterator __l, _ForwardIterator __r)
{ {
typedef typename iterator_traits<_ForwardIterator>::value_type value_type; typedef typename iterator_traits<_ForwardIterator>::value_type value_type;
#ifndef _LIBCPP_NO_EXCEPTIONS #ifndef _LIBCPP_NO_EXCEPTIONS
_ForwardIterator __s = __r; _ForwardIterator __s = __r;
try try
▲ Show 20 Lines • Show All 274 Lines • ▼ Show 20 Lines
{ {
#ifndef _LIBCPP_NO_EXCEPTIONS #ifndef _LIBCPP_NO_EXCEPTIONS
throw bad_weak_ptr(); throw bad_weak_ptr();
#else #else
_VSTD::abort(); _VSTD::abort();
#endif #endif
} }
template<class _CntrlBlk, class _Tp>
struct __shared_ptr_aligned_block
{
typename _VSTD::aligned_storage<
sizeof(_CntrlBlk),
_VSTD::alignment_of<_CntrlBlk>::value
>::type __cntrl_buff;
ldionneUnsubmitted
Done
ReplyInline Actions

Wait, so you have an array of sizeof(ControlBlock) elements of aligned storage with sizeof(ControlBlock)? That's sizeof(ControlBlock) * sizeof(ControlBlock) bytes of storage. Why do you need that?

ldionne: Wait, so you have an array of `sizeof(ControlBlock)` elements of aligned storage with `sizeof…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

🤦‍♂️ Oops. Good catch. I originally had alignas(_CntrlBlk) char __cntrl_buff[sizeof(_CntrlBlk)] which didn't work in C++03 mode so, I used aligned_storage and seem to have forgotten to remove the array part.

zoecarver: 🤦‍♂️ Oops. Good catch. I originally had `alignas(_CntrlBlk) char __cntrl_buff[sizeof…
typename _VSTD::aligned_storage<
sizeof(_Tp),
_VSTD::alignment_of<_Tp>::value
>::type __value_buff;
ldionneUnsubmitted
Done
ReplyInline Actions

Similar comment but with sizeof(_Tp) * sizeof(_Tp). I also don't understand why this needs to be aligned to alignment_of<ControlBlock>. Can you explain?

ldionne: Similar comment but with `sizeof(_Tp) * sizeof(_Tp)`. I also don't understand why this needs to…
};
template<class _Tp> class _LIBCPP_TEMPLATE_VIS weak_ptr; template<class _Tp> class _LIBCPP_TEMPLATE_VIS weak_ptr;
class _LIBCPP_TYPE_VIS __shared_count class _LIBCPP_TYPE_VIS __shared_count
{ {
__shared_count(const __shared_count&); __shared_count(const __shared_count&);
__shared_count& operator=(const __shared_count&); __shared_count& operator=(const __shared_count&);
protected: protected:
▲ Show 20 Lines • Show All 81 Lines • ▼ Show 20 Linesprivate:
virtual void __on_zero_shared_weak() _NOEXCEPT = 0; virtual void __on_zero_shared_weak() _NOEXCEPT = 0;
}; };
template <class _Tp, class _Dp, class _Alloc> template <class _Tp, class _Dp, class _Alloc>
class __shared_ptr_pointer class __shared_ptr_pointer
: public __shared_weak_count : public __shared_weak_count
{ {
__compressed_pair<__compressed_pair<_Tp, _Dp>, _Alloc> __data_; __compressed_pair<__compressed_pair<_Tp, _Dp>, _Alloc> __data_;
size_t __size;
ldionneUnsubmitted
Not Done
ReplyInline Actions

Isn't this an ABI break? It's not clear to me why we need to store the size -- we should always be calling allocator.deallocate with one instance of sizeof(__shared_ptr_pointer), no?

ldionne: Isn't this an ABI break? It's not clear to me why we need to store the size -- we should always…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

I was worried this might be an ABI break.

We need size, though. Because we need to know how big __shared_ptr_aligned_block is (and later we'll need to know how big __shared_ptr_aligned_block + elements is).

sizeof(__shared_ptr_pointer) would just be the control block. If we knew that __shared_ptr_pointer would only be used by allocate_shared we could do something like sizeof(__shared_ptr_aligned_block< __shared_ptr_pointer , remove_pointer_t<_Tp>>). To do that we'd probably have to duplicate __shared_ptr_pointer which would be an ABI break too, I think.

zoecarver: I was worried this might be an ABI break. We need size, though. Because we need to know how…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Is there a way to verify that this is in fact an ABI break? If this is an ABI break, I have an idea about how to implement this by storing the size at the beginning of __shared_ptr_aligned_block but, I think that will add a lot of complexity, so this is (IMHO) a much better implementation if we can get away with it.

zoecarver: Is there a way to verify that this is in fact an ABI break? If this is an ABI break, I have an…
public: public:
_LIBCPP_INLINE_VISIBILITY _LIBCPP_INLINE_VISIBILITY
__shared_ptr_pointer(_Tp __p, _Dp __d, _Alloc __a) __shared_ptr_pointer(_Tp __p, _Dp __d, _Alloc __a,
: __data_(__compressed_pair<_Tp, _Dp>(__p, _VSTD::move(__d)), _VSTD::move(__a)) {} size_t __size = sizeof(__shared_ptr_pointer))
: __data_(__compressed_pair<_Tp, _Dp>(__p, _VSTD::move(__d)), _VSTD::move(__a)),
__size(__size) {}
#ifndef _LIBCPP_NO_RTTI #ifndef _LIBCPP_NO_RTTI
virtual const void* __get_deleter(const type_info&) const _NOEXCEPT; virtual const void* __get_deleter(const type_info&) const _NOEXCEPT;
#endif #endif
private: private:
virtual void __on_zero_shared() _NOEXCEPT; virtual void __on_zero_shared() _NOEXCEPT;
virtual void __on_zero_shared_weak() _NOEXCEPT; virtual void __on_zero_shared_weak() _NOEXCEPT;
Show All 17 Lines__shared_ptr_pointer<_Tp, _Dp, _Alloc>::__on_zero_shared() _NOEXCEPT
__data_.first().second()(__data_.first().first()); __data_.first().second()(__data_.first().first());
__data_.first().second().~_Dp(); __data_.first().second().~_Dp();
} }
template <class _Tp, class _Dp, class _Alloc> template <class _Tp, class _Dp, class _Alloc>
void void
__shared_ptr_pointer<_Tp, _Dp, _Alloc>::__on_zero_shared_weak() _NOEXCEPT __shared_ptr_pointer<_Tp, _Dp, _Alloc>::__on_zero_shared_weak() _NOEXCEPT
{ {
typedef typename __allocator_traits_rebind<_Alloc, __shared_ptr_pointer>::type _Al; typedef typename __allocator_traits_rebind<_Alloc, char>::type _Al;
typedef allocator_traits<_Al> _ATraits; typedef allocator_traits<_Al> _ATraits;
typedef pointer_traits<typename _ATraits::pointer> _PTraits; typedef pointer_traits<typename _ATraits::pointer> _PTraits;
_Al __a(__data_.second()); _Al __a(__data_.second());
__data_.second().~_Alloc(); __data_.second().~_Alloc();
__a.deallocate(_PTraits::pointer_to(*this), 1); __a.deallocate(_PTraits::pointer_to(*reinterpret_cast<char*>(this)), __size);
ldionneUnsubmitted
Not Done
ReplyInline Actions

Can you explain why, in this patch, using __a.deallocate(_PTraits::pointer_to(*this), 1); is not sufficient?

ldionne: Can you explain why, in this patch, using `__a.deallocate(_PTraits::pointer_to(*this), 1);` is…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Because _Al is rebound to char type and used in _ATraits which is used to get the pointer type for _PTraits so, _PTraits:: pointer_to will be expecting a char*.

zoecarver: Because `_Al` is rebound to `char` type and used in `_ATraits` which is used to get the pointer…
ldionneUnsubmitted
Not Done
ReplyInline Actions

But why do you need to rebind it to char?

ldionne: But why do you need to rebind it to `char`?
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

What other type would you bind it to? Sometimes we're going to need to deallocate sizeof(__shared_ptr_pointer) but sometimes it will need to be sizeof(__shared_ptr_aligned_block< __shared_ptr_pointer , remove_pointer_t<_Tp>>) and later it will need to be that plus the size and number of elements we allocated. We don't really know.

A temporary alternative could be adding another template argument which the allocator could rebind to but, we'd need to add back size in D62641 (to support unbounded array types).

zoecarver: What other type would you bind it to? //Sometimes// we're going to need to deallocate `sizeof…
} }
template <class _Tp, class _Alloc> template <class _Tp, class _Alloc>
class __shared_ptr_emplace class __shared_ptr_emplace
: public __shared_weak_count : public __shared_weak_count
{ {
__compressed_pair<_Alloc, _Tp> __data_; __compressed_pair<_Alloc, _Tp> __data_;
public: public:
▲ Show 20 Lines • Show All 865 Lines • ▼ Show 20 Lines
template<class _Tp, class _Alloc, class ..._Args> template<class _Tp, class _Alloc, class ..._Args>
inline _LIBCPP_INLINE_VISIBILITY inline _LIBCPP_INLINE_VISIBILITY
typename enable_if typename enable_if
< <
!is_array<_Tp>::value, !is_array<_Tp>::value,
shared_ptr<_Tp> shared_ptr<_Tp>
>::type >::type
allocate_shared(const _Alloc& __a, _Args&& ...__args) allocate_shared(const _Alloc& __alloc, _Args&& ...__args)
{ {
static_assert( is_constructible<_Tp, _Args...>::value, "Can't construct object in allocate_shared"); typedef typename __allocator_traits_rebind<_Alloc, _Tp>::type _AllocForTp;
typedef allocator_traits<_AllocForTp> _AllocTraitsForTp;
typedef __allocator_destroyer<_AllocForTp> _DestroyerForTp;
ldionneUnsubmitted
Done
ReplyInline Actions

This should either be there, or not be there. We don't leave commented-out stuff in the code :-)

ldionne: This should either be there, or not be there. We don't leave commented-out stuff in the code :-)
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Oops. I thought I removed this.

zoecarver: Oops. I thought I removed this.
// The allocator here doesn't matter because it's going to be rebound before it's used.
typedef __shared_ptr_pointer<_Tp*, _DestroyerForTp, _Alloc> _CntrlBlk;
ldionneUnsubmitted
Done
ReplyInline Actions

These variable names (like _D1) are not really helpful. I know there weren't better before, but it would be easier to read with a better name.

ldionne: These variable names (like `_D1`) are not really helpful. I know there weren't better before…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

You're right. I'll make these more clear.

zoecarver: You're right. I'll make these more clear.
typedef __shared_ptr_aligned_block<_CntrlBlk, _Tp> _AlignedBlk;
typedef typename __allocator_traits_rebind<_Alloc, _AlignedBlk>::type _AllocForCntrlBlk;
ldionneUnsubmitted
Done
ReplyInline Actions

Shouldn't this be <_Tp*, _D1, _TAlloc> instead of <_Tp*, _D1, _Alloc>? If not, why?

ldionne: Shouldn't this be `<_Tp*, _D1, _TAlloc>` instead of `<_Tp*, _D1, _Alloc>`? If not, why?
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

It doesn't really matter because it gets rebound to char before being called. I'll put a comment here.

zoecarver: It doesn't really matter because it gets rebound to `char` before being called. I'll put a…
typedef __allocator_destructor<_AllocForCntrlBlk> _DestroyerForCntrlBlk;
typedef __shared_ptr_emplace<_Tp, _Alloc> _CntrlBlk; #if _LIBCPP_STD_VER > 17
typedef typename __allocator_traits_rebind<_Alloc, _CntrlBlk>::type _A2; _AllocForTp __alloc_for_tp(__alloc);
typedef __allocator_destructor<_A2> _D2; #endif // _LIBCPP_STD_VER > 17
_AllocForCntrlBlk __alloc_for_cntrl(__alloc);
unique_ptr<_AlignedBlk, _DestroyerForCntrlBlk> __holder(__alloc_for_cntrl.allocate(1),
_DestroyerForCntrlBlk(__alloc_for_cntrl, 1));
_CntrlBlk* __cntrl_buff = reinterpret_cast<_CntrlBlk*>(&__holder.get()->__cntrl_buff);
_Tp* __value_buff = reinterpret_cast<_Tp*>(&__holder.get()->__value_buff);
_A2 __a2(__a); ::new(static_cast<void*>(__cntrl_buff))
unique_ptr<_CntrlBlk, _D2> __hold2(__a2.allocate(1), _D2(__a2, 1)); _CntrlBlk(__value_buff, _DestroyerForTp(__alloc), __alloc_for_cntrl, sizeof(_AlignedBlk));
::new(static_cast<void*>(_VSTD::addressof(*__hold2.get())))
_CntrlBlk(__a, _VSTD::forward<_Args>(__args)...);
typename shared_ptr<_Tp>::element_type *__p = __hold2.get()->get(); #if _LIBCPP_STD_VER > 17
return shared_ptr<_Tp>::__create_with_control_block(__p, _VSTD::addressof(*__hold2.release())); _AllocTraitsForTp::construct(__alloc_for_tp, __value_buff, _VSTD::forward<_Args>(__args)...);
#else
ldionneUnsubmitted
Not Done
ReplyInline Actions

__create_with_control_block needs to be marked as noexcept if you want to do this and make it clear that it's safe. Otherwise we could leak __hold2 in case an exception was thrown from within __create_with_control_block.

ldionne: `__create_with_control_block` needs to be marked as `noexcept` if you want to do this and make…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

OK, I'll make that a separate patch, though. __create_with_control_block was already used here.

zoecarver: OK, I'll make that a separate patch, though. `__create_with_control_block` was already used…
::new(__value_buff) _Tp(_VSTD::forward<_Args>(__args)...);
#endif // _LIBCPP_STD_VER > 17
__holder.release();
return shared_ptr<_Tp>::__create_with_control_block(__value_buff, __cntrl_buff);
} }
template<class _Tp, class _Up> template<class _Tp, class _Up>
inline _LIBCPP_INLINE_VISIBILITY inline _LIBCPP_INLINE_VISIBILITY
bool bool
operator==(const shared_ptr<_Tp>& __x, const shared_ptr<_Up>& __y) _NOEXCEPT operator==(const shared_ptr<_Tp>& __x, const shared_ptr<_Up>& __y) _NOEXCEPT
{ {
return __x.get() == __y.get(); return __x.get() == __y.get();
▲ Show 20 Lines • Show All 933 Lines • Show Last 20 Lines

libcxx/test/libcxx/utilities/memory/util.smartptr/util.smartptr.shared/function_type_default_deleter.fail.cpp

Show All 32 Linesint main(int, char**) {
{ {
SPtr<0> s; // OK SPtr<0> s; // OK
SPtr<1> s1(nullptr); // OK SPtr<1> s1(nullptr); // OK
SPtr<2> s2(getFn<2>(), Deleter{}); // OK SPtr<2> s2(getFn<2>(), Deleter{}); // OK
SPtr<3> s3(nullptr, Deleter{}); // OK SPtr<3> s3(nullptr, Deleter{}); // OK
} }
// expected-error-re@memory:* 2 {{static_assert failed{{.*}} "default_delete cannot be instantiated for function types"}} // expected-error-re@memory:* 2 {{static_assert failed{{.*}} "default_delete cannot be instantiated for function types"}}
{ {
// expected-error@memory:* {{no member named 'value' in '{{.*}}is_empty<{{.*}}__compressed_pair<void (*)(Tag<4>), {{.*}}default_delete<void (Tag<4>)> > >'}}
ldionneUnsubmitted
Done
ReplyInline Actions

This expected-error is brittle, it won't work if we e.g. change the ABI namespace.

ldionne: This `expected-error` is brittle, it won't work if we e.g. change the ABI namespace.
// expected-error@memory:* {{cannot initialize return object of type '{{.*}}__shared_weak_count *' with an rvalue of type '{{.*}}__shared_ptr_pointer<void (*)(Tag<4>), {{.*}}default_delete<void (Tag<4>)>, {{.*}}allocator<{{.*}}__shared_ptr_dummy_rebind_allocator_type> > *'}}
SPtr<4> s4(getFn<4>()); // expected-note {{requested here}} SPtr<4> s4(getFn<4>()); // expected-note {{requested here}}
SPtr<5> s5(getFn<5>(), std::default_delete<FnType<5>>{}); // expected-note {{requested here}} SPtr<5> s5(getFn<5>(), std::default_delete<FnType<5>>{}); // expected-note {{requested here}}
} }
return 0; return 0;
} }

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared.pass.cpp

//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
Lint: Lint
Inline Actions

clang-format suggested style edits found:

Lint: Lint: clang-format suggested style edits found:
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// <memory> // <memory>
▲ Show 20 Lines • Show All 79 Lines • ▼ Show 20 Linesstruct Three
int value; int value;
Three(int v, int, int) : value(v) {++count;} Three(int v, int, int) : value(v) {++count;}
Three(Three const & o) : value(o.value) {++count;} Three(Three const & o) : value(o.value) {++count;}
~Three() {--count;} ~Three() {--count;}
}; };
int Three::count = 0; int Three::count = 0;
#if TEST_STD_VER < 20
template<class T>
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-template<class T>
-struct AllocNoConstruct : std::allocator<T>
-{
-    AllocNoConstruct() = default;
+template <class T>
+struct AllocNoConstruct : std::allocator<T> {
+  AllocNoConstruct() = default;
Lint: Pre-merge checks: clang-format: please reformat the code ``` -template<class T> -struct AllocNoConstruct : std…
struct AllocNoConstruct : std::allocator<T>
{
AllocNoConstruct() = default;
template <class T1>
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-    template <class T1>
-    AllocNoConstruct(AllocNoConstruct<T1>) {}
+  template <class T1>
+  AllocNoConstruct(AllocNoConstruct<T1>) {}
Lint: Pre-merge checks: clang-format: please reformat the code ``` - template <class T1> - AllocNoConstruct…
AllocNoConstruct(AllocNoConstruct<T1>) {}
template <class T1>
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-    template <class T1>
-    struct rebind {
-        typedef AllocNoConstruct<T1> other;
-    };
+  template <class T1>
+  struct rebind {
+    typedef AllocNoConstruct<T1> other;
+  };
Lint: Pre-merge checks: clang-format: please reformat the code ``` - template <class T1> - struct rebind {…
struct rebind {
typedef AllocNoConstruct<T1> other;
};
void construct(void*) { assert(false); }
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-    void construct(void*) { assert(false); }
+  void construct(void*) { assert(false); }
Lint: Pre-merge checks: clang-format: please reformat the code ``` - void construct(void*) { assert(false); } +…
};
#endif // TEST_STD_VER < 20
template <class Alloc> template <class Alloc>
void test() void test()
{ {
int const bad = -1; int const bad = -1;
{ {
std::shared_ptr<Zero> p = std::allocate_shared<Zero>(Alloc()); std::shared_ptr<Zero> p = std::allocate_shared<Zero>(Alloc());
assert(Zero::count == 1); assert(Zero::count == 1);
} }
▲ Show 20 Lines • Show All 51 Lines • ▼ Show 20 Linesint main(int, char**)
char c = 'f'; char c = 'f';
std::shared_ptr<A> p = std::allocate_shared<A>(bare_allocator<void>(), i, c); std::shared_ptr<A> p = std::allocate_shared<A>(bare_allocator<void>(), i, c);
assert(A::count == 1); assert(A::count == 1);
assert(p->get_int() == 68); assert(p->get_int() == 68);
assert(p->get_char() == 'f'); assert(p->get_char() == 'f');
} }
assert(A::count == 0); assert(A::count == 0);
// Test that we don't call construct before C++20.
#if TEST_STD_VER < 20
{
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-    {
-    (void)std::allocate_shared<int>(AllocNoConstruct<int>());
-    }
+    { (void)std::allocate_shared<int>(AllocNoConstruct<int>()); }
Lint: Pre-merge checks: clang-format: please reformat the code ``` - { - (void)std::allocate_shared<int>…
(void)std::allocate_shared<int>(AllocNoConstruct<int>());
}
#endif // TEST_STD_VER < 20
return 0; return 0;
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-  return 0;
+    return 0;
Lint: Pre-merge checks: clang-format: please reformat the code ``` - return 0; + return 0; ```
} }

libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/allocate_shared_construct.pass.cpp

  • This file was added.
//===----------------------------------------------------------------------===//
//
ldionneUnsubmitted
Not Done
ReplyInline Actions

Could you add a test (in another test file) with the reproducer for https://bugs.llvm.org/show_bug.cgi?id=41900, with both private and protected constructors? IIUC, the intent of this patch was primarily to fix that.

ldionne: Could you add a test (in another test file) with the reproducer for https://bugs.llvm.
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Yes. I'll add a test for that. Good call.

zoecarver: Yes. I'll add a test for that. Good call.
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
// UNSUPPORTED: c++03, c++11, c++14, c++17
// <memory>
// shared_ptr
// template<class T, class A, class... Args>
// shared_ptr<T> allocate_shared(const A& a, Args&&... args);
ldionneUnsubmitted
Done
ReplyInline Actions

Please explain why this needs to be tested in the comment. It could be as simple as "[...] as requested in C++20", which is the case I think.

ldionne: Please explain why this needs to be tested in the comment. It could be as simple as "[...] as…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Huh, you're right. I didn't realize this was since C++20. I'll add the comment.

zoecarver: Huh, you're right. I didn't realize this was since C++20. I'll add the comment.
ldionneUnsubmitted
Not Done
ReplyInline Actions

Is this a DR that was back-ported to older standards, or should this "feature" only work in C++20? If so, I would expect something like UNSUPPORTED: c++03, c++11, c++14, c++17.

ldionne: Is this a DR that was back-ported to older standards, or should this "feature" only work in…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

According to cppreference, it should use placement new before C++20 (FWIW that seems a bit odd to me). I'll update this to use placement new before C++20.

zoecarver: [[ https://en.cppreference.com/w/cpp/memory/shared_ptr/allocate_shared | According to…
// This patch tests that allocator_traits::construct is used in allocate_shared
// as requested in C++20.
#include "test_macros.h"
#include <memory>
#include <cassert>
static bool construct_called = false;
static bool destroy_called = false;
static unsigned allocator_id = 0;
template <class T>
struct MyAllocator {
public:
typedef T value_type;
typedef T* pointer;
unsigned id = 0;
MyAllocator() = default;
MyAllocator(int id) : id(id) {}
template <class U>
MyAllocator(MyAllocator<U> const& other) : id(other.id){};
pointer allocate(std::ptrdiff_t n) {
return pointer(static_cast<T*>(::operator new(n * sizeof(T))));
}
void deallocate(pointer p, std::ptrdiff_t) { return ::operator delete(p); }
void construct(T* p) {
construct_called = true;
destroy_called = false;
allocator_id = id;
::new (p) T;
}
void construct(T* p, T x) {
construct_called = true;
destroy_called = false;
allocator_id = id;
::new (p) T(x);
}
void construct(T* p, T x, T y) {
construct_called = true;
destroy_called = false;
allocator_id = id;
::new (p) T(x, y);
}
void destroy(T* p) {
destroy_called = true;
construct_called = false;
allocator_id = id;
p->~T();
}
};
struct Private;
class Factory {
public:
static std::shared_ptr<Private> allocate();
};
template <class T>
struct FactoryAllocator;
struct Private {
int id;
private:
friend FactoryAllocator<Private>;
Private(int id) : id(id) {}
~Private() {}
};
template <class T>
struct FactoryAllocator : std::allocator<T> {
FactoryAllocator() = default;
template <class T1>
FactoryAllocator(FactoryAllocator<T1>) {}
template <class T1>
struct rebind {
typedef FactoryAllocator<T1> other;
};
void construct(void* p, int id) { ::new (p) Private(id); }
ldionneUnsubmitted
Not Done
ReplyInline Actions

Please fix this todo. It's fine to assume that C++03 has variadics, cause we only support C++03 with Clang, which supports variadics.

ldionne: Please fix this `todo`. It's fine to assume that C++03 has variadics, cause we only support…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Yes, I'll fix this. I want to make it a separate patch because it will be a fairly large change.

zoecarver: Yes, I'll fix this. I want to make it a separate patch because it will be a fairly large change.
ldionneUnsubmitted
Not Done
ReplyInline Actions

I guess my point is that you can always assume variadics, even in C++03 mode. We can remove any attempt to emulate variadics in C++03 prior to applying this patch.

ldionne: I guess my point is that you can always assume variadics, even in C++03 mode. We can remove any…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

OK good to know. I'll submit a patch for that today.

zoecarver: OK good to know. I'll submit a patch for that today.
void destroy(Private* p) { p->~Private(); }
};
std::shared_ptr<Private> Factory::allocate() {
FactoryAllocator<Private> factory_alloc;
return std::allocate_shared<Private>(factory_alloc, 42);
}
struct mchar {
char c;
};
struct Foo {
int val;
Foo(int val) : val(val) {}
Foo(Foo a, Foo b) : val(a.val + b.val) {}
};
struct Bar {
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Because of construct is currently implemented, this only works when passed exactly one argument. I'll fix that when I remove the rebind variadics.

zoecarver: Because of `construct` is currently implemented, this only works when passed exactly one…
std::max_align_t y;
};
void test_aligned(void* p, size_t align) {
assert(reinterpret_cast<uintptr_t>(p) % align == 0);
}
int main(int, char**) {
{
std::shared_ptr<int> p = std::allocate_shared<int>(MyAllocator<int>());
assert(construct_called);
}
assert(destroy_called);
{
std::shared_ptr<Foo> p =
std::allocate_shared<Foo>(MyAllocator<Foo>(), Foo(42), Foo(100));
assert(construct_called);
assert(p->val == 142);
}
assert(destroy_called);
{ // Make sure allocator is copied.
std::shared_ptr<int> p = std::allocate_shared<int>(MyAllocator<int>(3));
assert(allocator_id == 3);
allocator_id = 0;
}
assert(allocator_id == 3);
{
std::shared_ptr<int> p = std::allocate_shared<int>(MyAllocator<int>(), 42);
assert(construct_called);
assert(*p == 42);
}
assert(destroy_called);
{ // Make sure allocator is properly re-bound.
std::shared_ptr<int> p =
std::allocate_shared<int>(MyAllocator<mchar>(), 42);
assert(construct_called);
assert(*p == 42);
}
assert(destroy_called);
{
// Make sure that we call the correct allocator::construct. Private has a private constructor
// so the construct method must be called on its friend Factory's allocator
// (Factory::Allocator).
std::shared_ptr<Private> p = Factory().allocate();
assert(p->id == 42);
}
{
std::shared_ptr<Bar> p;
test_aligned(p.get(), alignof(Bar));
}
return 0;
}