This is an archive of the discontinued LLVM Phabricator instance.

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
zoecarver updated this revision to Diff 202507.May 31 2019, 4:37 PM
  • update fail tests
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

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

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?

4545

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.

4547

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.

4545

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

4547

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 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.

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

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

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
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).

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.

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