Page MenuHomePhabricator

Add the C++17 <memory_resource> header (mono-patch)
Needs RevisionPublic

Authored by Quuxplusone on Oct 8 2020, 10:21 AM.

Details

Reviewers
ldionne
K-ballo
famastefano
EricWF
griwes
philnik
Group Reviewers
Restricted Project
Summary

This patch is the rebase and squash of three earlier patches. It supersedes all three of them.

  • D47111: experimental monotonic_buffer_resource.
  • D47358: experimental pool resources.
  • D47360: Copy std::experimental::pmr to std::pmr.

The significant difference between this patch and the-sum-of-those-three is that this patch does not add std::experimental::pmr::monotonic_buffer_resource and so on. This patch simply adds the C++17 standard facilities, and leaves the std::experimental namespace entirely alone.

Diff Detail

Unit TestsFailed

TimeTest
12,060 mslibcxx CI AIX (32-bit) > ibm-libc++-shared-cfg-in.std/utilities/utility/mem_res/mem_res_global::new_delete_resource.pass.cpp
Script: -- : 'COMPILED WITH'; /opt/IBM/openxlC/17.1.0/bin/ibm-clang++_r /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/std/utilities/utility/mem.res/mem.res.global/new_delete_resource.pass.cpp --target=powerpc-ibm-aix -nostdinc++ -D__LIBC_NO_CPP_MATH_OVERLOADS__ -isystem /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/include/c++/v1 -I /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support -include /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -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 -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -nostdlib++ -L /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/lib -lc++ -lc++abi -latomic -Wl,-bbigtoc -o /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/std/utilities/utility/mem.res/mem.res.global/Output/new_delete_resource.pass.cpp.dir/t.tmp.exe
12,530 mslibcxx CI AIX (32-bit) > ibm-libc++-shared-cfg-in.std/utilities/utility/mem_res/mem_res_monotonic_buffer/mem_res_monotonic_buffer_mem::allocate_deallocate.pass.cpp
Script: -- : 'COMPILED WITH'; /opt/IBM/openxlC/17.1.0/bin/ibm-clang++_r /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_deallocate.pass.cpp --target=powerpc-ibm-aix -nostdinc++ -D__LIBC_NO_CPP_MATH_OVERLOADS__ -isystem /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/include/c++/v1 -I /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support -include /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -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 -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -nostdlib++ -L /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/lib -lc++ -lc++abi -latomic -Wl,-bbigtoc -o /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/Output/allocate_deallocate.pass.cpp.dir/t.tmp.exe
12,660 mslibcxx CI AIX (32-bit) > ibm-libc++-shared-cfg-in.std/utilities/utility/mem_res/mem_res_monotonic_buffer/mem_res_monotonic_buffer_mem::allocate_exception_safety.pass.cpp
Script: -- : 'COMPILED WITH'; /opt/IBM/openxlC/17.1.0/bin/ibm-clang++_r /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_exception_safety.pass.cpp --target=powerpc-ibm-aix -nostdinc++ -D__LIBC_NO_CPP_MATH_OVERLOADS__ -isystem /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/include/c++/v1 -I /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support -include /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -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 -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -nostdlib++ -L /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/lib -lc++ -lc++abi -latomic -Wl,-bbigtoc -o /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/Output/allocate_exception_safety.pass.cpp.dir/t.tmp.exe
12,860 mslibcxx CI AIX (32-bit) > ibm-libc++-shared-cfg-in.std/utilities/utility/mem_res/mem_res_monotonic_buffer/mem_res_monotonic_buffer_mem::allocate_from_initial_buffer.pass.cpp
Script: -- : 'COMPILED WITH'; /opt/IBM/openxlC/17.1.0/bin/ibm-clang++_r /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_from_initial_buffer.pass.cpp --target=powerpc-ibm-aix -nostdinc++ -D__LIBC_NO_CPP_MATH_OVERLOADS__ -isystem /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/include/c++/v1 -I /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support -include /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -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 -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -nostdlib++ -L /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/lib -lc++ -lc++abi -latomic -Wl,-bbigtoc -o /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/Output/allocate_from_initial_buffer.pass.cpp.dir/t.tmp.exe
12,660 mslibcxx CI AIX (32-bit) > ibm-libc++-shared-cfg-in.std/utilities/utility/mem_res/mem_res_monotonic_buffer/mem_res_monotonic_buffer_mem::allocate_from_underaligned_buffer.pass.cpp
Script: -- : 'COMPILED WITH'; /opt/IBM/openxlC/17.1.0/bin/ibm-clang++_r /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/allocate_from_underaligned_buffer.pass.cpp --target=powerpc-ibm-aix -nostdinc++ -D__LIBC_NO_CPP_MATH_OVERLOADS__ -isystem /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/include/c++/v1 -I /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support -include /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -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 -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -nostdlib++ -L /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/lib -lc++ -lc++abi -latomic -Wl,-bbigtoc -o /scratch/powerllvm/cpap8009/llvm-project/libcxx-ci/build/aix/std/utilities/utility/mem.res/mem.res.monotonic.buffer/mem.res.monotonic.buffer.mem/Output/allocate_from_underaligned_buffer.pass.cpp.dir/t.tmp.exe
View Full Test Results (30 Failed)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jloser added a subscriber: jloser.Oct 25 2021, 9:07 AM

@Quuxplusone can you please remove destroy() (and its tests) from polymorphic_allocator? Then you can mark LWG3036 as done and we avoid the issue of whether to mark destroy() as deprecated vs hard removal. Simply not including it when we ship this for the first time is the easiest :)

polymorphic_allocator: Remove the __max_size() private member, and include-what-you-use. Mark the allocate method as [[nodiscard]] (to match C++20). Stop discarding the result of allocate in the tests. Mark destroy as _LIBCPP_DEPRECATED_IN_CXX23. Stop using destroy in the tests.

I've now got a local patch on top of this one that implements p0339 "polymorphic_allocator<> as a vocabulary type," but I still need to write unit tests for it.

libcxx/include/__memory_resource/polymorphic_allocator.h
147–150
@jloser wrote:

@Quuxplusone can you please remove destroy() (and its tests) from polymorphic_allocator? Then you can mark LWG3036 as done and we avoid the issue of whether to mark destroy() as deprecated vs hard removal. Simply not including it when we ship this for the first time is the easiest :)

Sadly, it appears that destroy is only deprecated in C++23, not removed, so if we want to be conforming we should still have it (just marked as [[deprecated]]).
http://eel.is/c++draft/depr#mem.poly.allocator.mem

Or is your point that we could just kinda "forget" to implement destroy (the same way we've "forgotten" to implement all of <memory_resource> for the past 4 years ;)), and just wait for the Standard to catch up to our non-implementation? @ldionne thoughts?

jloser added inline comments.Oct 31 2021, 4:24 PM
libcxx/include/__memory_resource/polymorphic_allocator.h
147–150

Yeah, I was hoping we could skip the deprecated step since libc++ doesn't have std::pmr::polymorphic_allocator right now - it's in the experimental namespace.

dougsonos added inline comments.Dec 21 2021, 10:20 PM
libcxx/include/__memory_resource/polymorphic_allocator.h
147–150

By this same logic, libc++ could neglect to implement all the variants of construct() which were added in C++17 and removed in C++20 :/

philnik requested changes to this revision.Jan 6 2022, 5:12 PM
philnik added a reviewer: philnik.
philnik added a subscriber: philnik.

I didn't get all the way through, but here are about 20 nit picks. I'll look at the tests later.

libcxx/include/__memory_resource/memory_resource.h
30

Is there a reason this isn't constexpr or just directly in lines 35 and 39?

37

I hate this brace style with all my heart, but it's used through out the code base so I guess I can't do anything about it.

40

Nit: Use void* instead of void * or at least do it consistently (see line 35).

69–79
libcxx/include/__memory_resource/synchronized_pool_resource.h
62
libcxx/include/__memory_resource/unsynchronized_pool_resource.h
50–58

Again - is there a reason these aren't constexpr?

97

What do you want to say with // key function? For the vtable?

libcxx/include/deque
3027–3035

Why is this not just in the above std namespace?

libcxx/lib/abi/CHANGELOG.TXT
26–60

Are these symbols not added under linux?

libcxx/src/memory_resource.cpp
18

Doesn't this only work with lld?

117–118
127–136

And with the other set checks as well.

128
177–178
191–192
213

Wouldn't it make more sense to put that in the header so the dylib doesn't have to change depending on wanting these assertions or not?

214–229
336–346
414

Please remove this else as well.

427–428

Why not use std::min and std::max?

Quuxplusone marked 26 inline comments as done.

Address @philnik's review comments.
Mark allocate as [[nodiscard]]: that might be new in C++20, but I think we can get away with backporting it anyway. Should add a test for that, I guess.

libcxx/include/__memory_resource/memory_resource.h
30

The answer in both cases is that this came via copy-paste from <experimental/memory_resource>. I agree s/const/constexpr/g is an improvement. I'll keep it pulled out as a named constant, though, since it's used in two places and it's just barely conceivable that some platform might like to change it.

40

This is one of those places where I think LLVM style is T* x but my-preference and at-least-half-the-time-libc++-style is T *x. I'll change line 35 instead.

libcxx/include/__memory_resource/synchronized_pool_resource.h
62

:) I actually have no idea why I did this here. Will fix.

libcxx/include/__memory_resource/unsynchronized_pool_resource.h
97

Yes, that's right; see also

../libcxx/include/__filesystem/filesystem_error.h:  ~filesystem_error() override; // key function
libcxx/include/deque
3027–3035

Here and also in the C++20 Ranges stuff, I've got a habit of "returning to zero" in between unrelated facilities, i.e. I'll nest things like (x)((y))(x) rather than (x(y)x). IMO this makes it easier to keep track of the nesting levels. It's not to the level of a conscious guideline with me, necessarily — it's mostly muscle memory / personal style — but if you explicitly ask me to pick one style or the other, well, I'll still pick this one.
Leaving as-is, at least until a consensus builds against it. ;)

libcxx/lib/abi/CHANGELOG.TXT
26–60

They are; I just was too lazy to dig up the build artifact that lists them. Will fix after a build completes again.

libcxx/src/memory_resource.cpp
18

Copied from experimental/memory_resource.cpp, so it'll work as well as that one does (for better and worse).

127–136

This was originally copy-pasted from src/experimental/... but the early-return rewrite strikes me as harder to read, so I'm keeping it as-is for now.

128

PSA: You flipped the sense of this; you meant != nullptr. Fixed.

213

I agree with your goal, but this guy's caller is unsynchronized_pool_resource::do_deallocate (also in src/), so I don't think it's possible. Not without leaking a lot more stuff into the headers, which conflicts with our other goal of trying to maintain some semblance of ability-to-change-implementation.

427–428

Originally, either because <algorithm> had not been granularized and I didn't want to drag in all of <algorithm> for this (although that's a bad rationale), or because these can deal with mixed types (e.g. min(sizeof(int), 4) whereas _VSTD::min would require casting.
But now: sure, OK.

449–452

FYI, this is the call that requires a cast after switching to _VSTD::min.

ldionne requested changes to this revision.Feb 8 2022, 3:55 PM
ldionne added inline comments.
libcxx/docs/Status/Cxx20Issues.csv
236

This should be 15.0 since we slipped again.

libcxx/docs/Status/Cxx2bIssues.csv
19

Same, 15.0.

libcxx/include/__memory_resource/polymorphic_allocator.h
65

Please use numeric_limits<size_t>::max(). I know they are equivalent, but being close to the Standard is good. I don't mind taking an additional dependency on <limits> at all. This applies elsewhere too.

153–155

This is nitpicky on a style matter, but can you please just do

memory_resource *resource() const noexcept { 
    return __res_;
}

or

memory_resource *resource() const noexcept { return __res_; }

if you really care about saving a line. This applies elsewhere too.

The reason I'm asking this is that we don't use the pattern you have here anywhere else (that I can tell), and I don't want to introduce yet another stylistic inconsistency.

libcxx/include/__memory_resource/synchronized_pool_resource.h
18

Please indent includes when they are nested to near-located #ifs.

libcxx/include/__memory_resource/unsynchronized_pool_resource.h
105–109

It sucks that we have to commit to an ABI right here. It would be great if we could instead store a pimpl, but that would require allocating and that's not a great idea.

libcxx/include/memory_resource
2

We don't include the name of the file anymore!

20

Can we get section names like // [mem.res.class]?

libcxx/include/regex
6801

Any reason for not putting those declarations inside the _LIBCPP_BEGIN_NAMESPACE_STD that was closed just above?

6805

Suggestion: you don't have to, but you could move all your new code to std:: instead of _VSTD::.

libcxx/include/string
4593

I think this needs to be guarded by _LIBCPP_HAS_NO_WIDE_CHARACTERS. I would expect this to be caught by the CI.

libcxx/lib/abi/CHANGELOG.TXT
1

[commenting here for lack of better place]

Another thing we'll need to add is availability macros for the newly added dylib functionality. You can look at include/__availability for how that's done.

111

This should be moved under a new section 15.0. Sorry for the churn.

156

You can figure this one out by running ./libcxx/utils/ci/run-buildbot-container locally (requires Docker) and then running the generate-cxx-abilist target on Linux.

libcxx/src/memory_resource.cpp
1

No file name!

13

Please indent nested #includes.

28

Why not remove this altogether?

32

Let's always include this.

53

So when aligned allocation is not supported, we allocate unaligned and try to align the pointer within the allocated buffer. If that works, we're happy, otherwise we fail. I don't understand why that would ever succeed, can you explain?

Also, I've always been a bit confused by the _LIBCPP_HAS_NO_ALIGNED_ALLOCATION situation. It would be worth looking into whether we can just assume that it's provided when we build the library.

111

Is there a reason why you don't just assume that atomics are available? I would do:

#if defined(_LIBCPP_HAS_NO_THREADS)

    <no threading implementation>

#else

    <use atomic>

#endif
libcxx/src/memory_resource_init_helper.h
2

Once you rebase, you'll need to use _LIBCPP_CONSTINIT.

libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/resource.pass.cpp
10

Here and everywhere else, you'll need to also add

// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx{{11|12}}
libcxx/test/support/test_std_memory_resource.h
12–20

Sort?

libcxx/utils/generate_feature_test_macro_components.py
460

Can we add a release note?

This revision now requires changes to proceed.Feb 8 2022, 3:55 PM

Could you add comments in the TODO.txt for all the experimental things that can be removed in libc++17 (hopefully)? I think most experimental headers could be removed.

libcxx/src/memory_resource.cpp
367

It's not a strong feeling, but I'd rather see *= 2 than <<= 1. FWIW it produces the same asm, even at -O0

387–388
470–471
libcxx/test/libcxx/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_piecewise_pair.pass.cpp
42–48

Please reorder the includes

56–58
73–74
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.ctor/copy.pass.cpp
26–31
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.ctor/other_alloc.pass.cpp
28–39
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_pair.pass.cpp
42

Couldn't we just use a char ptr[sizeof(P)]? Seems overkill to malloc() for that.

libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_pair_rvalue.pass.cpp
39

Please choose PtrT* v or PtrT *v, but not PtrT * v for all that is sacred. I don't want to multiply my types!

libcxx/test/std/utilities/utility/mem.res/mem.res.global/default_resource.pass.cpp
67–71
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2022, 8:32 PM

Is this still being worked on? It would be nice (from a user point of view) if this could be in one of the next releases of libc++.

(I'm sorry for the noise if bumping dists like this is frowned upon.)

Is this still being worked on? It would be nice (from a user point of view) if this could be in one of the next releases of libc++.

I'm not aware of anybody working on this so I don't expect it to become part of the next libc++ release.
I'm not going to work on it, my todo list has enough items to keep me busy.

Is this still being worked on? It would be nice (from a user point of view) if this could be in one of the next releases of libc++.

I'm not aware of anybody working on this so I don't expect it to become part of the next libc++ release.
I'm not going to work on it, my todo list has enough items to keep me busy.

I plan to work on it after the LLVM 15 release branch if we don't get things sorted with Arthur until then. I really want to get this into LLVM 16, but who knows how long it will take.