Details
- Reviewers
mclow.lists EricWF rsmith ldionne - Group Reviewers
Restricted Project - Commits
- rG98d3d5b5da66: [libc++] Implement P1004R2 (constexpr std::vector)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
I've had a revision locally for a long long time. It doesn't pass everything
yet, but I've managed to get it down to ~10 failing tests with a few more hacks
locally.
Some quick review of the the acutal changes to the headers
libcxx/include/memory | ||
---|---|---|
884–885 | I am wondering whether we should reuse the simplementation of std::copy_n here (or at the call site for what its worth) | |
909–910 | As said above, we should be able to reuse the internals of std::copy_n here | |
956–966 | I believe the inline is superfluous with the _LIBCPP_CONSTEXPR_AFTER_CXX17 Appears throughout | |
956–966 | From here and below there is _LIBCPP_CONSTEXPR_AFTER_CXX11 rather than _LIBCPP_CONSTEXPR_AFTER_CXX17 Also the ordering of _LIBCPP_INLINE_VISIBILITY and _LIBCPP_CONSTEXPR_AFTER_CXX11 is swapped. | |
libcxx/include/vector | ||
325–326 | Many inline below here |
Rebase onto main.
@miscco's comments have not been addressed yet -- I only did the minimal amount of work to fix merge conflicts with main.
Note that not all tests have been constexpr-ified yet, and the ones that have been constexpr-ified are not all passing. Transferring to @philnik as discussed offline.
- Fix tests
libcxx/include/memory | ||
---|---|---|
884–885 | I think this and the comment below have been addressed. But I'm not 100% sure what you were referring to exactly. Could you comment again at the correct line if the comments haven't been addressed? | |
956–966 | Sorry, I don't know what you are referring to again. I guess they have been addressed since I can't see any _LIBCPP_CONSTEXPR_AFTER_CXX11 of inline in the diff. | |
libcxx/include/vector | ||
325–326 | Again, I guess it has been addressed? |
libcxx/include/__bit_reference | ||
---|---|---|
358–362 | I think we should always call std::fill_n here instead. Indeed, both are equivalent, but std::fill_n is nicer for various reasons. I know fill_n doesn't end up calling memset directly, however the compiler will end up calling memset (when optimized). IIRC, @var-const was actually about to add this optimization to fill_n explicitly, when we realized it didn't have an impact on the code generation. @var-const can you confirm? Also, I think we need to add a comment in std::fill_n that explains this. I think it can be done in this patch since we're switching from memset to fill_n. This comment applies elsewhere in this patch too. | |
503 | Why is this required? | |
958–961 | Instead, I would begin the lifetime of the elements inside __bit_array's constructor if we are during constant evaluation. That way you shouldn't need to change this code at all. | |
libcxx/include/__memory/construct_at.h | ||
43–44 | Please match the style of the class = decltype above, just for consistency. | |
49 | Let's add the _LIBCPP_ASSERT here too. | |
libcxx/include/__utility/move.h | ||
40–49 | I don't think this diff is needed in this patch, see my comment below. | |
libcxx/include/memory | ||
902 | I don't think we *need* to make this change to enable constexpr vector, right? If not, let's pull it into a separate patch, cause I'd like to have a discussion about it. Otherwise, can you please explain why it's needed? | |
909–910 | Given that __begin2 = std::__copy(__begin1, __end1, __begin2); does not compile, I think this overload is never used. Can you please confirm this by adding static_assert(sizeof(_Alloc) == 0, "") and running the tests? If the tests don't fail, then this is a dead code path and we'll need to investigate it. Edit Well, it looks like we've got enable_if<cond>::type above as a template parameter. IOW, we're declaring a NTTP of type void, and that's never valid. 🤦🏼♂️ Clang should really tell us about that. Let's fix it in a separate patch prior to this one. | |
910 | Why are we using std::__copy here? Let's use std::copy instead. | |
931–936 | I think this should all just be uninitialized_copy (or a __ version of it). |
libcxx/include/__bit_reference | ||
---|---|---|
358–362 | I can confirm that working on https://reviews.llvm.org/D118329, I found that the compiler would replace uninitialized_fill{,_n} with calls to memset in an optimized build, which made it unnecessary to write that optimization explicitly. I would expect fill_n to work similarly, but I have only tried with the uninitialized algorithms. FWIW, I have a lingering suspicion that there might be some corner cases (related to aliasing, perhaps) where the compiler would fail to do the optimization, but it was absolutely reliable in all the (simple) cases I checked. |
libcxx/include/__bit_reference | ||
---|---|---|
391–394 | I think it would be more obvious if we at least used static_cast<__storage_type>(-1). Either way we should probably add a comment. | |
libcxx/include/vector | ||
1708 | What happens if we remove this condition? If the issue is with comparing pointers to unrelated areas of memory, what happens when you use std::less to perform the comparison instead? std::less has a special blessing when it comes to comparing unrelated pointers (yes, it's crazy). | |
1805 | What happens if we remove this condition? |
libcxx/include/__bit_reference | ||
---|---|---|
358–362 | @ldionne @var-const Can we get a godbolt showing a call to fill_n that gets lowered to memset? | |
libcxx/include/vector | ||
417 | Is this ever constexpr with the calls to the debug functions? | |
650 | is this function actually constexpr-able? |
libcxx/include/__bit_reference | ||
---|---|---|
358–362 | https://godbolt.org/z/je1PGzGds There is a test for 0 size after the patch, because we do that in fill_n. We might or might not want to look into it, but this can be tackled as a separate issue. |
This commit breaks some code in c++20 mode: https://gcc.godbolt.org/z/h7j93MGsb
Specifically, move assignment of a vector of incomplete type can't be compiled now. Is this expected?
#include <vector> struct S { S(S&& s) { v = std::move(s.v); } struct Inner; std::vector<Inner> v; }; struct S::Inner { int a; };
In file included from <source>:1: /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:540:52: error: arithmetic on a pointer to an incomplete type 'S::Inner' {return static_cast<size_type>(__end_cap() - this->__begin_);} ~~~~~~~~~~~ ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:760:56: note: in instantiation of member function 'std::vector<S::Inner>::capacity' requested here __annotate_contiguous_container(data(), data() + capacity(), ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:431:7: note: in instantiation of member function 'std::vector<S::Inner>::__annotate_delete' requested here __annotate_delete(); ^ <source>:4:3: note: in instantiation of member function 'std::vector<S::Inner>::~vector' requested here S(S&& s) { v = std::move(s.v); } ^ <source>:5:10: note: forward declaration of 'S::Inner' struct Inner; ^ In file included from <source>:1: /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:760:54: error: arithmetic on a pointer to an incomplete type 'const value_type' (aka 'const S::Inner') __annotate_contiguous_container(data(), data() + capacity(), ~~~~~~ ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:431:7: note: in instantiation of member function 'std::vector<S::Inner>::__annotate_delete' requested here __annotate_delete(); ^ <source>:4:3: note: in instantiation of member function 'std::vector<S::Inner>::~vector' requested here S(S&& s) { v = std::move(s.v); } ^ <source>:5:10: note: forward declaration of 'S::Inner' struct Inner; ^ In file included from <source>:1: /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:833:64: error: arithmetic on a pointer to an incomplete type 'S::Inner' __alloc_traits::destroy(__alloc(), _VSTD::__to_address(--__soon_to_be_end)); ^ ~~~~~~~~~~~~~~~~ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:827:29: note: in instantiation of member function 'std::vector<S::Inner>::__base_destruct_at_end' requested here void __clear() _NOEXCEPT {__base_destruct_at_end(this->__begin_);} ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:436:9: note: in instantiation of member function 'std::vector<S::Inner>::__clear' requested here __clear(); ^ <source>:4:3: note: in instantiation of member function 'std::vector<S::Inner>::~vector' requested here S(S&& s) { v = std::move(s.v); } ^ <source>:5:10: note: forward declaration of 'S::Inner' struct Inner; ^ In file included from <source>:1: In file included from /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:296: In file included from /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/__split_buffer:20: /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/__memory/allocator.h:128:58: error: invalid application of 'sizeof' to an incomplete type 'S::Inner' _VSTD::__libcpp_deallocate((void*)__p, __n * sizeof(_Tp), _LIBCPP_ALIGNOF(_Tp)); ^~~~~~~~~~~ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/__memory/allocator_traits.h:282:13: note: in instantiation of member function 'std::allocator<S::Inner>::deallocate' requested here __a.deallocate(__p, __n); ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:437:25: note: in instantiation of member function 'std::allocator_traits<std::allocator<S::Inner>>::deallocate' requested here __alloc_traits::deallocate(__alloc(), this->__begin_, capacity()); ^ <source>:4:3: note: in instantiation of member function 'std::vector<S::Inner>::~vector' requested here S(S&& s) { v = std::move(s.v); } ^ <source>:5:10: note: forward declaration of 'S::Inner' struct Inner; ^ In file included from <source>:1: /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:537:53: error: arithmetic on a pointer to an incomplete type 'S::Inner' {return static_cast<size_type>(this->__end_ - this->__begin_);} ~~~~~~~~~~~~ ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:636:32: note: in instantiation of member function 'std::vector<S::Inner>::size' requested here size_type __old_size = size(); ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:953:9: note: in instantiation of member function 'std::vector<S::Inner>::clear' requested here clear(); ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:1314:5: note: in instantiation of member function 'std::vector<S::Inner>::__vdeallocate' requested here __vdeallocate(); ^ /opt/compiler-explorer/clang-trunk-20220809/bin/../include/c++/v1/vector:1288:5: note: in instantiation of member function 'std::vector<S::Inner>::__move_assign' requested here __move_assign(__x, integral_constant<bool, ^ <source>:4:16: note: in instantiation of member function 'std::vector<S::Inner>::operator=' requested here S(S&& s) { v = std::move(s.v); } ^ <source>:5:10: note: forward declaration of 'S::Inner' struct Inner; ^ 5 errors generated. Compiler returned: 1
@alexfh I'm not sure why this worked before, but you've definitely got UB there: http://eel.is/c++draft/vector#overview-4. Clang is probably less permissive when something is declared constexpr.
For a -DLIBCXX_ENABLE_DEBUG_MODE=ON build this now causes
$ cat test.cc #define _LIBCPP_DEBUG 1 #include <vector> static std::vector<int> v; int main() { v.push_back(0); int a = 0; for (auto i: v) a += i; return a; } $ clang++ -stdlib=libc++ -std=c++20 test.cc $ ./a.out ~/llvm/inst/bin/../include/c++/v1/__iterator/wrap_iter.h:87: assertion ::std::__libcpp_is_constant_evaluated() || (__get_const_db()->__dereferenceable(this)) failed: Attempted to dereference a non-dereferenceable iteratorAborted (core dumped)
I think we should always call std::fill_n here instead. Indeed, both are equivalent, but std::fill_n is nicer for various reasons.
I know fill_n doesn't end up calling memset directly, however the compiler will end up calling memset (when optimized). IIRC, @var-const was actually about to add this optimization to fill_n explicitly, when we realized it didn't have an impact on the code generation. @var-const can you confirm?
Also, I think we need to add a comment in std::fill_n that explains this. I think it can be done in this patch since we're switching from memset to fill_n.
This comment applies elsewhere in this patch too.