Page MenuHomePhabricator

Implement C++20's P0784 (More constexpr containers)
ClosedPublic

Authored by ldionne on Oct 2 2019, 5:40 PM.

Details

Summary

This commit adds std::construct_at, and marks various members of std::allocator_traits and std::allocator as constexpr. It also adds tests and turns the existing tests into hybrid constexpr/runtime tests.

Thanks to Richard Smith for initial work on this, and to Michael Park for D69803, D69132 and D69134, which are superseded by this patch.

Diff Detail

Event Timeline

rsmith created this revision.Oct 2 2019, 5:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2019, 5:40 PM
Herald added a subscriber: christof. · View Herald Transcript
rsmith updated this revision to Diff 222943.Oct 2 2019, 5:47 PM

Moved some changes from the vector patch to here.

Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2019, 5:47 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rsmith updated this revision to Diff 222944.Oct 2 2019, 5:48 PM

Undo adding too many files to this change.

Harbormaster completed remote builds in B38920: Diff 222944.
ldionne commandeered this revision.Oct 3 2019, 7:44 AM
ldionne added a reviewer: rsmith.

Thanks for doing this, Richard.
A few things:

  • Needs tests (as you said)
  • The config macro stuff is all wrong (we build that file from a template)
  • Use __libcpp_is_constant_evaluated instead of __builtin_is_constant_evaluated because it doesn't have to be #ifdefed. (see r364873 and r364884)
  • If we're in a consteval block, we shouldn't be testing for exceptions being disabled, right?
ldionne updated this revision to Diff 292600.Sep 17 2020, 1:12 PM

Complete implementation of P0784 with tests

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 17 2020, 1:12 PM
ldionne retitled this revision from Prototype implementation of P0784R7: mark all members of std::allocator and std::allocator_traits as constexpr. to Implement C++20's P0784 (More constexpr containers).Sep 17 2020, 1:13 PM
ldionne edited the summary of this revision. (Show Details)
ldionne added a reviewer: mpark.

The mechanism by which this interacts with Clang looks good to me. I've not done any detailed analysis to check all the functions made constexpr by P0784 are handled by this patch, though.

libcxx/include/version
254

Should this be conditioned on compiler support being available?

libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.members/allocate_hint.pass.cpp
87

I really like this approach to testing this change :)

The mechanism by which this interacts with Clang looks good to me.

Excellent.

I've not done any detailed analysis to check all the functions made constexpr by P0784 are handled by this patch, though.

I spent like 4-5 hours making sure everything was to the spec :-). The only missing part is the ranges declarations.

libcxx/include/version
254

So.. I've decided not to do that in this patch so far.

The support for constexpr allocation was checked into Clang about a year ago, right? I actually expect this to be a slightly contentious point, but I'd like to assume that we're using a reasonably recent Clang. I don't see a strong point for being able to use new libc++ headers with an old Clang anyway, since vendors usually release the two together. IOW, supporting this would add complexity for virtually no benefit. I do agree it's a slightly more aggressive stance than we've had so far, but this sort of reasonable assumption makes it so much easier to write stuff for libc++.

libcxx/test/std/utilities/memory/allocator.traits/allocator.traits.members/allocate_hint.pass.cpp
87

Thanks! That's how we do for all constexpr-ifications.

rsmith added inline comments.Sep 17 2020, 2:09 PM
libcxx/include/version
254

OK, just a few thoughts then I'm going to bow out of this; this seems like a policy decision for the libc++ maintainers to make.

In favor of dropping support for new libc++ + old clang: we generally don't permit version skew between different components of LLVM. It seems reasonable to expect all wanted parts of a particular LLVM release to be built together.

Against dropping support for new libc++ + old clang: we do support installing more than one version of LLVM (and in particular more than one version of Clang) on the same system, but because libc++ defaults to being installed in /usr/include/c++/v1, we don't seem to encourage installing more than one version of libc++, so -- even assuming we only support the *newest* version of libc++ going into /usr/include/c++/v1 -- new versions of libc++ need to work with old versions of Clang.

I think (largely by accident) Clang will prefer a libc++ installed into /usr/lib/clang/$VER/include over one from /usr/include/c++/v1. If we switched to installing libc++ there, I don't see any technical barrier to version-locking them, though I'm not sure what story that leaves for use of libc++ with GCC and other compilers. It seems worth noting that this is exactly what libstdc++ does in order to need to support only one version of GCC.

ldionne added inline comments.Sep 17 2020, 2:32 PM
libcxx/include/version
254

OK, just a few thoughts then I'm going to bow out of this; this seems like a policy decision for the libc++ maintainers to make.

Hear hear!

I think (largely by accident) Clang will prefer a libc++ installed into /usr/lib/clang/$VER/include over one from /usr/include/c++/v1. If we switched to installing libc++ there, I don't see any technical barrier to version-locking them, though I'm not sure what story that leaves for use of libc++ with GCC and other compilers. It seems worth noting that this is exactly what libstdc++ does in order to need to support only one version of GCC.

I think it would be great to do that. Honestly, this is a huge simplification and makes implementing new features much easier. Also, I think it's reasonable to support not-trunk compilers, like 6 months old or something like that. But not several years back.

One last question: do you know what controls where the libc++ headers are installed as part of the LLVM distribution?

rsmith added inline comments.Sep 17 2020, 5:43 PM
libcxx/include/version
254

Install paths are set in libcxx/include/CMakeLists.txt for the headers and in libcxx/src/CMakeLists.txt for the libraries (search for install() based on cmake variables LIBCXX_INSTALL_HEADER_PREFIX, LIBCXX_INSTALL_PREFIX, and LIBCXX_INSTALL_LIBRARY_DIR.

It would probably make sense to put the libc++ headers into somewhere like /usr/lib/clang/$VER/include/c++ instead of directly in /usr/lib/clang/$VER/include; that'll need some changes to the clang driver to make sure we look there. But of course that's doable if the two are version-locked :)

ldionne added inline comments.
libcxx/include/new
243

This breaks GCC (as of GCC 9). I don't know what mechanism GCC uses to tie into constexpr allocation, so I don't know what the fix is.

@jwakely Can you throw some hints at me?

libcxx/include/version
254

Oh, I thought it was more complicated than that for the LLVM distribution. Ok, thanks for the info.

ldionne updated this revision to Diff 293429.Sep 22 2020, 5:53 AM

Add some tests from D69134.

jwakely added inline comments.Sep 22 2020, 6:15 AM
libcxx/include/new
243

GCC 9 doesn't support constexpr allocation at all.

ldionne accepted this revision as: Restricted Project.EditedSep 22 2020, 6:17 AM

I'll go ahead and ship this even though I know it will cause problems on GCC 9, if GCC 10 supports constexpr allocation.

libcxx/include/new
243

Does GCC 10 support it?

jwakely added inline comments.Sep 22 2020, 6:17 AM
libcxx/include/new
243

GCC 9 doesn't support constexpr allocation at all. You can use __cpp_constexpr_dynamic_alloc to test for the feature.

jwakely added inline comments.Sep 22 2020, 6:19 AM
libcxx/include/new
243

Yes

This revision was not accepted when it landed; it landed in state Needs Review.Sep 22 2020, 8:20 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

Hi. Our GCC builders are failing with

[182/12433] CXX kernel-x64-gcc/obj/kernel/lib/ktl/ktl.dummy-new.cc.o
FAILED: kernel-x64-gcc/obj/kernel/lib/ktl/ktl.dummy-new.cc.o
/b/s/w/ir/k/fuchsia/prebuilt/third_party/goma/linux-x64/gomacc  ../../prebuilt/third_party/gcc/linux-x64/bin/x86_64-elf-g++ -MD -MF kernel-x64-gcc/obj/kernel/lib/ktl/ktl.dummy-new.cc.o.d -o kernel-x64...
In file included from ../../zircon/kernel/lib/ktl/dummy-new.cc:7:
../../prebuilt/third_party/clang/linux-x64/include/c++/v1/new: In function 'constexpr void* std::__2::__libcpp_allocate(size_t, size_t)':
../../prebuilt/third_party/clang/linux-x64/include/c++/v1/new:252:24: error: call to non-'constexpr' function 'void* operator new(size_t)'
  252 |   return ::operator new(__size);
      |          ~~~~~~~~~~~~~~^~~~~~~~
In file included from ../../zircon/kernel/lib/ktl/dummy-new.cc:7:
../../prebuilt/third_party/clang/linux-x64/include/c++/v1/new:186:66: note: 'void* operator new(size_t)' declared here
  186 | _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_OVERRIDABLE_FUNC_VIS void* operator new(std::size_t __sz) _THROW_BAD_ALLOC;
      |                                                                  ^~~~~~~~

I presume it's because _LIBCPP_HAS_NO_BUILTIN_OPERATOR_NEW_DELETE is defined from

#if !__has_builtin(__builtin_operator_new) || !__has_builtin(__builtin_operator_delete)
#define _LIBCPP_HAS_NO_BUILTIN_OPERATOR_NEW_DELETE
#endif

Is there a recommended way for working around this? We're using GCC 10.2.1. Thanks.

jwakely added a comment.EditedSep 25 2020, 3:27 AM

Is there a recommended way for working around this? We're using GCC 10.2.1. Thanks.

I don't think your implementation is valid. I think P0784 only allows new-expressions and calls to std::allocator::allocate in constexpr functions, not calls to operator new.

Can you use something like if (__builtin_is_constant_evaluated()) return new unsigned char[n]; and the equivalent in the corresponding deallocation function?

Is there a recommended way for working around this? We're using GCC 10.2.1. Thanks.

I don't think your implementation is valid. I think P0784 only allows new-expressions and calls to std::allocator::allocate in constexpr functions, not calls to operator new.

Hmm, right. It's actually impossible to implement __libcpp_allocate in a constexpr-friendly manner, since it's just allocating bytes.

Can you use something like if (__builtin_is_constant_evaluated()) return new unsigned char[n]; and the equivalent in the corresponding deallocation function?

That doesn't work, since we can't use unsigned chars as storage for arbitrary Ts in constexpr.

I fixed it by marking __libcpp_allocate as non-constexpr and calling operator new in allocator<T>::allocate, which seems to work on GCC.

I don't think your implementation is valid. I think P0784 only allows new-expressions and calls to std::allocator::allocate in constexpr functions, not calls to operator new.

Hmm, right. It's actually impossible to implement __libcpp_allocate in a constexpr-friendly manner, since it's just allocating bytes.

Right, we need to rely on implementation magic. I wrote up a document proposing how to do this a year or so ago, and shared it with some GCC folks (I don't remember whether it was mailed to the GCC mailing list or just to Jason or what) suggesting that we follow option 3 of that doc -- in particular, this permits calls to operator new "transitively within a member of std::allocator<T>" (emphasis mine), precisely so that stdlib implementations can pull out helper functions as libc++ does, and don't need to branch on is_constant_evaluated() for this at all. That said, this is all vendor extension territory, and GCC is of course under no obligation to follow my proposal, and indeed seems not to have followed this detail of it. So be it.

But... there's no question of "valid" here :)

Hi Louis,

ARM and AArch64 libcxx bots are still broken after this commit (host
compiler is clang-8), logs are available here:
http://lab.llvm.org:8011/builders/libcxx-libcxxabi-libunwind-aarch64-linux/builds/3143

Cheers,
Yvan