Page MenuHomePhabricator

Support arrays in make_shared and allocate_shared (P0674R1)
Needs ReviewPublic

Authored by zoecarver on May 29 2019, 7:25 PM.


Group Reviewers
Restricted Project

This patch implements P0674R1. It might be a bit rough around the edges, but I wanted to get it out so it can be reviewed and I can make changes to it as I straighten out the last few parts.

The main changes are how __shared_ptr_pointer deallocates itself and (obviously) the added overloads.

Thanks for the help @mclow.lists and @EricWF

Diff Detail

Unit TestsFailed

3,410 mslibcxx CI GCC/C++20 > libc++.std/utilities/memory/util_smartptr/util_smartptr_shared/util_smartptr_shared_create::create_array.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++ /home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/create_array.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2a -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/build/generic-gcc/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/53940c553bb4-1/llvm-project/libcxx-ci/build/generic-gcc/projects/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/Output/create_array.pass.cpp.dir/t.tmp.exe
2,280 mslibcxx CI MacOS C++20 > libc++.std/utilities/memory/util_smartptr/util_smartptr_shared/util_smartptr_shared_create::create_array.pass.cpp
Script: -- : 'COMPILED WITH'; /Applications/ /private/tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/create_array.pass.cpp -isysroot /Applications/ -v --target=x86_64-apple-darwin19.6.0 -include /tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/__config_site -include /tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/libcxx/include -I/tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/include/c++build -I/tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -std=c++2a -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/private/tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/build/generic-cxx20/projects/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/Output/create_array.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -L/tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/build/generic-cxx20/./lib -Wl,-rpath,/tmp/buildkite-builds/ldionne-13-local-1/llvm-project/libcxx-ci/build/generic-cxx20/./lib -nodefaultlibs -lc++ -lSystem -lc++experimental -o…
6,220 mslibcxx CI UBSAN > libc++.std/utilities/memory/util_smartptr/util_smartptr_shared/util_smartptr_shared_create::create_array.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/089856e482a6-1/llvm-project/libcxx-ci/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/create_array.pass.cpp -v --target=x86_64-unknown-linux-gnu -g -fno-omit-frame-pointer -fsanitize=undefined -fno-sanitize=float-divide-by-zero -fno-sanitize-recover=all -include /home/libcxx-builder/.buildkite-agent/builds/089856e482a6-1/llvm-project/libcxx-ci/build/generic-ubsan/projects/libcxx/__config_site -include /home/libcxx-builder/.buildkite-agent/builds/089856e482a6-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/089856e482a6-1/llvm-project/libcxx-ci/libcxx/include -I/home/libcxx-builder/.buildkite-agent/builds/089856e482a6-1/llvm-project/libcxx-ci/build/generic-ubsan/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/089856e482a6-1/llvm-project/libcxx-ci/libcxx/test/support -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -O2 -std=c++2a -Werror -Wall -Wextra -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=/home/libcxx-builder/.buildkite-agent/builds/089856e482a6-1/llvm-project/libcxx-ci/build/generic-ubsan/projects/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.create/Output/create_array.pass.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -Wno-macro-redefined -D_LIBCPP_ABI_UNSTABLE -L/home/libcxx-builder/.buildkite-agent/builds/089856e482a6-1/llvm-project/libcxx…

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ldionne added inline comments.Jan 11 2021, 2:37 PM

Well, one place where we might want to use it is in the statically-sized array version of the block (see below why I think we should have one).


I don't understand the benefit of passing false_type here when we want to default construct, instead of just relying on variadics and always passing down some args... to __construct_elements (and hence to the object's constructors themselves). I think it would be simpler to do that.

Also, I think it makes sense to mark this constructor as explicit, and it needs to be marked with _LIBCPP_HIDE_FROM_ABI.


I see you're using an int here, but what if construction fails at a very large index, the size_t to int narrowing could be problematic. Is there a reason why you're using int?


The way I understand this is that we should actually always call __destroy_elements(__first, __i + 1); to destroy the half-open range [__first, __first + __i + 1) (which includes the last object that was fully constructed, but not the object that was partially constructed). However, when the current element (__first + __i) is an array, it has already been destroyed before we caught the exception, so we can skip it and just destroy [__first, __first + __i).

This implementation makes that intent clearest IMO:

// <explanation>
if constexpr (is_array_v<_ValueType>)
    __destroy_elements(__first, __i);
    __destroy_elements(__first, __i + 1);

Do you agree?


Aha! I actually took this from your earlier patch: make_shared can be implemented in terms of allocate_shared, so it can just be one function template with no constraints that calls out to all the various allocate_shared overloads.

I don't think that works, actually. For example, std::make_shared<int[3]>(1, 2, 3) should SFINAE away, but with your implementation that would be a hard error. Can you add a test and see whether that's correct? If so, it means we need to conform to the declarations laid out in the standard (there's often reasons why the standard lists overloads explicitly).


We save on storing the size itself in the control block, and the size is known statically, which could lead to different code being generated (e.g for small arrays). I think it's fairly important to do it, since it isn't very costly for us.

The representation for statically-sized arrays can be:

[[no_unique_address]] _Alloc __alloc_;
union { // make sure the default constructor doesn't initialize __data_
    _Tp __data_[_Np];
This revision now requires changes to proceed.Jan 11 2021, 2:37 PM
zoecarver added inline comments.Jan 12 2021, 12:58 PM

I'm using int because __n will become -1 in the for loop. I could keep it unsigned and just compare __n < "size", but that seems a bit more confusing and would require a second variable. So, I'll make __n a long.

zoecarver updated this revision to Diff 316209.EditedJan 12 2021, 1:06 PM
  • Add static size control block.
  • Fix based on review.
Harbormaster completed remote builds in B84906: Diff 316210.
zoecarver added inline comments.Jan 12 2021, 1:08 PM

Actually, because we're inheriting this base class, the [[no_unique_address]] isn't needed.

zoecarver added inline comments.Jan 12 2021, 2:08 PM

Oops, forgot the SFINAE test. I'll add that now.

zoecarver updated this revision to Diff 317693.Jan 19 2021, 2:15 PM
  • Add SFINAE tests.
  • Remove "no_unique_address".

I know this is complicated to test, but I think it is important. It may make sense to split tests into multiple files to make it easier to understand. Your call.

I personally don't mind the mono-test and it makes things easier to find, IMHO. But I also don't have a strong oppinion at all, would you rather me split this test up?

@ldionne this is ready for re-review when you're ready to re-review it :)

ldionne requested changes to this revision.Feb 9 2021, 8:42 AM

I don't think we gain much from having a single __shared_ptr_array_base common base. Actually, this forces us to have additional virtual methods in both subclasses. Instead, we should just have two different holder classes, one for statically-sized arrays, and one for dynamic arrays.


This should be 13.0 now

This revision now requires changes to proceed.Feb 9 2021, 8:42 AM
zoecarver updated this revision to Diff 322476.Feb 9 2021, 12:54 PM
  • Bump LLVM release version.
  • Remove __shared_ptr_array_base.
ldionne requested changes to this revision.Feb 9 2021, 4:09 PM

Thanks a lot for all your work on this patch. This is a tough one to get right. I know I'm requesting a lot, but I think it's important to get it right and we'll be even happier when we finally ship this :-).


We could also use __first[i] here, I think it's clearer?


I haven't looked for that specifically, but can you confirm that you are testing these two code paths?


Can we name them xx_bounded and xx_unbounded instead? It seems to fit better?


Currently, this function is only used in a single place (which is inside the implementation of this class). Furthermore, we duplicate that logic when we compute the size to allocate down there in make_shared_xx.

One possibility would be to move this to a free function like

template <class T>
size_t __get_shared_ptr_unbounded_block_size(size_t __n) {
    // ...

Then we could reuse it from both places. Do you agree that would be an improvement?


Don't we need to run the destructor of __alloc_ here?


Is this legal C++? I thought that was a compiler extension and arrays had to have at least one element?


Since this is only used in a single place in the body of __shared_ptr_array_bound, I think we could just inline the function at its single point of use and remove it. WDYT?


Same comment regarding destruction of __alloc_.


I *think* this can cause us to under allocate, for example if there needs to be padding between __count_ and __data_:

[[no_unique_address]] _Alloc __alloc_;
size_t __count_;
// padding possible here - would that be counted in sizeof(_ControlBlock)?
_DataBlock __data_[0];

This concern is moot if I'm right about _DataBlock __data_[0] being an extension and if we change to _DataBlock __data_[1] instead.

This revision now requires changes to proceed.Feb 9 2021, 4:09 PM
zoecarver marked 6 inline comments as done.Feb 15 2021, 1:41 PM
zoecarver added inline comments.

Why not pass by value?


Yes, these two code paths are tested. See L324 where we use struct D to throw.


Yep. Added a regression test, too.


Yes, you're right. Fixed. (Supprising that we didn't get a warning, though?)


Agreed. Same with __get_count.


Moot because I fixed __data_[0 -> 1].

However, I thought this was the whole point of using a zero size array at the end of a struct? Otherwise, why not just make an empty struct. So, yes, that padding would be accounted for:

struct UnsizedTail {
  int size;
  alignas(8) char buf[0];

// UnsizedTail = {i32, [4 x i8 padding], [0 x i8]
static_assert(sizeof(UnsizedTail) == 8);

That being said... when would there be padding between size_t and an aligned storage member?

zoecarver marked 3 inline comments as done.Feb 15 2021, 1:44 PM
zoecarver added inline comments.

Whoops. I thought I deleted this comment. This was just a note to myself.

Anyway, I am actually wondering about this. I see _Alloc const& all over the place, but allocators are generally supposed to be small (often empty) types, so why not pass them by value? We're copy constructing the allocator in this function, so it can't be to prevent that.

zoecarver updated this revision to Diff 323831.Feb 15 2021, 1:47 PM
  • Address review comments and add a few more tests.
  • Format test file with clang-format.
ldionne requested changes to this revision.Feb 19 2021, 1:34 PM

This basically LGTM except for the few nitpicks below. We already talked about most of that offline, just adding here so you have a written trace.


Generally, we take by value (and then move into some internal storage) when we take ownership of an object -- or we add a version that takes that object by rvalue reference. You could do that here and move into __value_alloc if you wanted.


We need to handle the case where __n == 0. Also, we should add a test for it since I'm not seeing anything that says it's not allowed by the API.


Can we add a test with overaligned types? (like we discussed offline)


You can remove c++98 from the Lit features, we don't use it anymore.


Can you add a comment mentioning that? It's clever.

This revision now requires changes to proceed.Feb 19 2021, 1:34 PM
zoecarver marked 4 inline comments as done.Feb 21 2021, 12:21 PM
zoecarver added inline comments.

Done. Thanks :)

zoecarver marked an inline comment as done.
  • Address review comments.
  • Re-generate feature test macros after rebase.
  • Format changes with git-clang-format.
  • Poke buildkit.

OK, that seems to have worked. @ldionne for future reference: the CI was failing to apply this patch because I had two parent revisions and a child revision which were abandoned/already landed. I think the abandoned one was causing problems because it had conflicts. Removing those patches fixed the issue, but it would be nice to automatically skip abandoned parent patches.

  • Fix for -fno-exceptions.
  • Destroy alloc before deallocating this.
  • Cast char to size_t in assert.
zoecarver updated this revision to Diff 327001.Feb 28 2021, 3:13 PM
  • Fix UBSAN and GCC build bots.
zoecarver updated this revision to Diff 327002.Feb 28 2021, 3:14 PM
  • Fix ASAN bots.
  • Format test files.
zoecarver added inline comments.Feb 28 2021, 3:17 PM

I really don't like disabling this whole test for sanitizer-new-delete. We need this because of the globalMemCounter tests. This is the same thing the unique_ptr tests use.

@ldionne do you know if there's a macro or some other way we can detect if we're running on sanitizer-new-delete? If so, we could just disable the two tests that actually cause this to fail.

ldionne added inline comments.Mar 1 2021, 10:32 AM

I don't think there's such a macro. You could split it off into its own test file if you wanted.

zoecarver updated this revision to Diff 327614.Mar 2 2021, 4:04 PM
  • Fix UBSAN bot.
  • Fix macOS bot.