This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Fix copyright headers. Maybe fix the failing monotonic-buffer-resource test, finally!
Still to do: Change "header" to "footer" throughout.
Still to do: Maybe rename e.g. "chunk_size_in_bytes => num_bytes_in_chunk".

Renamed chunk_header to chunk_footer and the corresponding header_{size,align} to footer_{size,align}. This actually makes the math in aligned_capacity easier to understand, IMO! I left the loop variable h alone, rather than rename it to f, because f seems uglier to me.

I left vacancy_header alone, because it is actually stored in the initial bytes of the chunk. (Right?)

So far, I've left initial_header alone, even though technically it is neither a header nor a footer, because it is stored outside of the initial buffer. Perhaps it could be renamed something like initial_info? Any thoughts on terminology there, @griwes?

Also in this update I've merged in a couple of threading-related fixes that have been made to <experimental/memory_resource> in the past year.

No change since last time. Rebase on D93137, and poke buildbots.

So far, I've left initial_header alone, even though technically it is neither a header nor a footer, because it is stored outside of the initial buffer. Perhaps it could be renamed something like initial_info? Any thoughts on terminology there, @griwes?

I think the name I've been using for things like that is "descriptor".

Quuxplusone edited the summary of this revision. (Show Details)

Rename "initial_header" to "initial_descriptor" (since technically it's neither a header nor a footer).
Rebase on main.

chfast added a subscriber: chfast.Mar 24 2021, 3:05 AM

Rebased and split into "new-style" granular headers.
This deliberately doesn't include any C++20 stuff; just the C++17 stuff for now.

@ldionne: I'll need your help to generate the "abilist" text files for this patch. Buildkite refuses to run any of the usual tests until the abilist is correct, and I have no way to generate a correct abilist without access to x86-64 and ARM64 Linux machines.

Sample failure: https://buildkite.com/llvm-project/libcxx-ci/builds/4991

@ldionne: I'll need your help to generate the "abilist" text files for this patch. Buildkite refuses to run any of the usual tests until the abilist is correct, and I have no way to generate a correct abilist without access to x86-64 and ARM64 Linux machines.

Sample failure: https://buildkite.com/llvm-project/libcxx-ci/builds/4991

The abilist text files should be available as artifact of the failed build.
Sample X86_64 abi list: https://buildkite.com/organizations/llvm-project/pipelines/libcxx-ci/builds/5037/jobs/669a7ee3-2214-46f5-84dd-b171d94cf4ca/artifacts/23ab7856-9a88-45dd-8345-0129748b172a

Rebase, add abilist artifacts (thanks @Mordante!), fix some formatting

Cosmetic improvements in the tests, and make more of them pass. (Still got trouble on 32-bit builds.)
Update another abilist.

Include <version> in <memory_resource>.
Fix the build when size_t is 32-bit.

Fix some tests for lack of aligned allocation.

One final(?) test-suite update. Use ASSERT_WITH_LIBRARY_INTERNAL_ALLOCATIONS to fix the Windows DLL buildkite (hooray!).

Apple 10.9 through 10.13 pass the overaligned-request tests, because they define TEST_HAS_NO_ALIGNED_ALLOCATION. Apple 10.14 and 10.15 fail, because the system dylib doesn't contain the required symbols from memory_resource.cpp.

@ldionne @griwes what do we think this year?

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 ↗(On Diff #398211)

This should be 15.0 since we slipped again.

libcxx/docs/Status/Cxx2bIssues.csv
19 ↗(On Diff #398211)

Same, 15.0.

libcxx/include/memory_resource
3

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

ldionne added inline comments.Feb 8 2022, 3:55 PM
libcxx/include/__memory_resource/polymorphic_allocator.h
66

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.

154–156

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
19

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

libcxx/include/__memory_resource/unsynchronized_pool_resource.h
106–110

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
21

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.

144

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

189

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
2

No file name!

14

Please indent nested #includes.

29

Why not remove this altogether?

33

Let's always include this.

54

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.

112

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
3

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
11

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
13–21

Sort?

libcxx/utils/generate_feature_test_macro_components.py
478

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
368

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

388–389
471–472
libcxx/test/libcxx/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_piecewise_pair.pass.cpp
43–49

Please reorder the includes

57–59
74–75
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.ctor/copy.pass.cpp
27–32
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.ctor/other_alloc.pass.cpp
29–40
libcxx/test/std/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/construct_pair.pass.cpp
43

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
40

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
68–72
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.

philnik commandeered this revision.Sep 6 2022, 7:10 AM
philnik edited reviewers, added: Quuxplusone; removed: philnik.
philnik updated this revision to Diff 458166.Sep 6 2022, 7:11 AM
  • Rebased
  • clang-formatted
  • Replaced _VSTD with std
philnik updated this revision to Diff 461516.Sep 20 2022, 2:13 AM
  • Try to fix CI
philnik updated this revision to Diff 461854.Sep 21 2022, 4:49 AM
  • Fix transitive includes list
ldionne accepted this revision.Sep 23 2022, 8:49 AM

LGTM once the CI passes.

libcxx/lib/abi/CHANGELOG.TXT
144

This should be moved to 16.0.

This revision is now accepted and ready to land.Sep 23 2022, 8:49 AM
philnik updated this revision to Diff 463295.Sep 27 2022, 11:43 AM
  • Try to fix CI
philnik updated this revision to Diff 463553.Sep 28 2022, 7:31 AM
  • Next try
philnik updated this revision to Diff 463867.Sep 29 2022, 6:22 AM
  • Update ABI lists
philnik updated this revision to Diff 463923.Sep 29 2022, 8:55 AM
  • Rebased
  • Fix transitive_includes tests
  • XFAIL macOS
philnik updated this revision to Diff 463950.Sep 29 2022, 10:04 AM
  • Generate files
ldionne added inline comments.Sep 29 2022, 10:09 AM
libcxx/test/libcxx/utilities/utility/mem.res/mem.poly.allocator.class/mem.poly.allocator.mem/db_deallocate.pass.cpp
2

Please rename this to debug.deallocate.pass.cpp for consistency.

10–12
philnik updated this revision to Diff 464113.Sep 29 2022, 5:35 PM
  • Try to fix CI
philnik updated this revision to Diff 464193.Sep 30 2022, 2:39 AM
  • Next try
ldionne accepted this revision.Oct 6 2022, 8:14 AM
ldionne added inline comments.
libcxx/lib/abi/CHANGELOG.TXT
146–147
149–189
philnik updated this revision to Diff 465859.Oct 6 2022, 1:38 PM
philnik marked 4 inline comments as done.
  • Rebased
  • Address comments