| Summary: | [13/14 Regression] std::to_array no longer works for struct which is trivial but not default constructible | ||
|---|---|---|---|
| Product: | gcc | Reporter: | daniel.klauer |
| Component: | libstdc++ | Assignee: | Jonathan Wakely <redi> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | daniel.kruegler, de34, pinskia, webrown.cpp |
| Priority: | P3 | Keywords: | patch, rejects-valid |
| Version: | 13.3.0 | ||
| Target Milestone: | 13.4 | ||
| URL: | https://gcc.gnu.org/pipermail/gcc-patches/2024-June/655146.html | ||
| See Also: |
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110167 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85723 |
||
| Host: | Target: | ||
| Build: | Known to work: | 12.3.0, 13.2.0 | |
| Known to fail: | 13.3.0, 14.1.0 | Last reconfirmed: | 2024-06-17 00:00:00 |
Confirmed. Looks like the check (that was added in r13-8421-g4c6bb36e88d5c8 / r14-1647-g960de5dd886572711ef86fa1e15e30d3810eccb9 ), constexpr (is_trivial_v<_Tp>) should be added on to, something like: constexpr (is_trivial_v<_Tp> && is_default_constructible_v<_Tp>) Or maybe more, I am not 100% sure. That shouldn't be needed, because a trivial class has to have a trivial default constructor.
The problem is that we default-initialize the array, which requires the const members to be initialized.
We also need to check for assignability, because trivial classes can have deleted assignment operators (for some reason).
--- a/libstdc++-v3/include/std/array
+++ b/libstdc++-v3/include/std/array
@@ -434,7 +434,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
static_assert(is_constructible_v<_Tp, _Tp&>);
if constexpr (is_constructible_v<_Tp, _Tp&>)
{
- if constexpr (is_trivial_v<_Tp>)
+ if constexpr (is_trivial_v<_Tp> && is_default_constructible_v<_Tp>
+ && is_copy_assignable_v<_Tp>)
{
array<remove_cv_t<_Tp>, _Nm> __arr;
if (!__is_constant_evaluated() && _Nm != 0)
@@ -463,7 +464,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
static_assert(is_move_constructible_v<_Tp>);
if constexpr (is_move_constructible_v<_Tp>)
{
- if constexpr (is_trivial_v<_Tp>)
+ if constexpr (is_trivial_v<_Tp> && is_default_constructible_v<_Tp>
+ && is_move_assignable_v<_Tp>)
{
array<remove_cv_t<_Tp>, _Nm> __arr;
if (!__is_constant_evaluated() && _Nm != 0)
(In reply to Jonathan Wakely from comment #3) > static_assert(is_constructible_v<_Tp, _Tp&>); > if constexpr (is_constructible_v<_Tp, _Tp&>) > { > - if constexpr (is_trivial_v<_Tp>) > + if constexpr (is_trivial_v<_Tp> && is_default_constructible_v<_Tp> > + && is_copy_assignable_v<_Tp>) For the testcase above, it would be sufficient to do: if constexpr (is_trivial_v<_Tp> && is_copy_assignable_v<_Tp>) The type with const members isn't assignable. I'm not sure if this covers all cases. I think combined with is_trivial it should do, but then I don't understand why the struct ranges type is trivial in the first place. Ah, it looks like is_trivial is just wrong for types with a deleted default constructor, that's PR 85723 The std::to_array code _should_ work correctly for this case :-\ (In reply to Jonathan Wakely from comment #5) > Ah, it looks like is_trivial is just wrong for types with a deleted default > constructor, that's PR 85723 > > The std::to_array code _should_ work correctly for this case :-\ I guess we still need to check is_default_constructible_v even after __is_trivial gets fixed because the default construct may be protected or private... The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>: https://gcc.gnu.org/g:510ce5eed69ee1bea9c2c696fe3b2301e16d1486 commit r15-1533-g510ce5eed69ee1bea9c2c696fe3b2301e16d1486 Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Jun 18 13:27:02 2024 +0100 libstdc++: Fix std::to_array for trivial-ish types [PR115522] Due to PR c++/85723 the std::is_trivial trait is true for types with a deleted default constructor, so the use of std::is_trivial in std::to_array is not sufficient to ensure the type can be trivially default constructed then filled using memcpy. I also forgot that a type with a deleted assignment operator can still be trivial, so we also need to check that it's assignable because the is_constant_evaluated() path can't use memcpy. Replace the uses of std::is_trivial with std::is_trivially_copyable (needed for memcpy), std::is_trivially_default_constructible (needed so that the default construction is valid and does no work) and std::is_copy_assignable (needed for the constant evaluation case). libstdc++-v3/ChangeLog: PR libstdc++/115522 * include/std/array (to_array): Workaround the fact that std::is_trivial is not sufficient to check that a type is trivially default constructible and assignable. * testsuite/23_containers/array/creation/115522.cc: New test. The releases/gcc-14 branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>: https://gcc.gnu.org/g:21c8708ba638f57cf904c8af3355318d9cf0f9e0 commit r14-10412-g21c8708ba638f57cf904c8af3355318d9cf0f9e0 Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Jun 18 13:27:02 2024 +0100 libstdc++: Fix std::to_array for trivial-ish types [PR115522] Due to PR c++/85723 the std::is_trivial trait is true for types with a deleted default constructor, so the use of std::is_trivial in std::to_array is not sufficient to ensure the type can be trivially default constructed then filled using memcpy. I also forgot that a type with a deleted assignment operator can still be trivial, so we also need to check that it's assignable because the is_constant_evaluated() path can't use memcpy. Replace the uses of std::is_trivial with std::is_trivially_copyable (needed for memcpy), std::is_trivially_default_constructible (needed so that the default construction is valid and does no work) and std::is_copy_assignable (needed for the constant evaluation case). libstdc++-v3/ChangeLog: PR libstdc++/115522 * include/std/array (to_array): Workaround the fact that std::is_trivial is not sufficient to check that a type is trivially default constructible and assignable. * testsuite/23_containers/array/creation/115522.cc: New test. (cherry picked from commit 510ce5eed69ee1bea9c2c696fe3b2301e16d1486) Fixed for 14.2, thanks for the report. Oh whoops, this still needs to be fixed on the gcc-13 branch, reopened. The releases/gcc-13 branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>: https://gcc.gnu.org/g:08463348c5cce84dc3c64ac4fbb20e2795ee104f commit r13-8908-g08463348c5cce84dc3c64ac4fbb20e2795ee104f Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Jun 18 13:27:02 2024 +0100 libstdc++: Fix std::to_array for trivial-ish types [PR115522] Due to PR c++/85723 the std::is_trivial trait is true for types with a deleted default constructor, so the use of std::is_trivial in std::to_array is not sufficient to ensure the type can be trivially default constructed then filled using memcpy. I also forgot that a type with a deleted assignment operator can still be trivial, so we also need to check that it's assignable because the is_constant_evaluated() path can't use memcpy. Replace the uses of std::is_trivial with std::is_trivially_copyable (needed for memcpy), std::is_trivially_default_constructible (needed so that the default construction is valid and does no work) and std::is_copy_assignable (needed for the constant evaluation case). libstdc++-v3/ChangeLog: PR libstdc++/115522 * include/std/array (to_array): Workaround the fact that std::is_trivial is not sufficient to check that a type is trivially default constructible and assignable. * testsuite/23_containers/array/creation/115522.cc: New test. (cherry picked from commit 510ce5eed69ee1bea9c2c696fe3b2301e16d1486) And fixed for 13.4 too. The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>: https://gcc.gnu.org/g:9690fb3a43e5cf26a5fb853283d4200df312a640 commit r15-2146-g9690fb3a43e5cf26a5fb853283d4200df312a640 Author: Marek Polacek <polacek@redhat.com> Date: Tue Jun 18 16:49:24 2024 -0400 c++: implement DR1363 and DR1496 for __is_trivial [PR85723] is_trivial was introduced in <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2230.html> which split POD into is_trivial and is_standard_layout. Later came CWG 1363. Since struct A { A() = default; A(int = 42) {} }; cannot be default-initialized, it should not be trivial, so the definition of what is a trivial class changed. Similarly, CWG 1496 concluded that struct B { B() = delete; }: should not be trivial either. P0848 adjusted the definition further to say "eligible". That means that template<typename T> struct C { C() requires false = default; }; should not be trivial, either, since C::C() is not eligible. Bug 85723 reports that we implement none of the CWGs. I chose to fix this by using type_has_non_deleted_trivial_default_ctor which uses locate_ctor which uses build_new_method_call, which would be used by default-initialization as well. With that, all __is_trivial problems I could find in the Bugzilla are fixed, except for PR96288, which may need changes to trivially-copyable, so I'm not messing with that now. I hope this has no ABI implications. There's effort undergoing to remove "trivial class" from the core language as it's not really meaningful. So the impact of this change should be pretty low except to fix a few libstdc++ problems. PR c++/108769 PR c++/58074 PR c++/115522 PR c++/85723 gcc/cp/ChangeLog: * class.cc (type_has_non_deleted_trivial_default_ctor): Fix formatting. * tree.cc (trivial_type_p): Instead of TYPE_HAS_TRIVIAL_DFLT, use type_has_non_deleted_trivial_default_ctor. gcc/testsuite/ChangeLog: * g++.dg/warn/Wclass-memaccess.C: Add dg-warning. * g++.dg/ext/is_trivial1.C: New test. * g++.dg/ext/is_trivial2.C: New test. * g++.dg/ext/is_trivial3.C: New test. * g++.dg/ext/is_trivial4.C: New test. * g++.dg/ext/is_trivial5.C: New test. * g++.dg/ext/is_trivial6.C: New test. |
Hi, The following example used to compile successfully with gcc 13.2 or older, but it no longer compiles in gcc 13.3 and 14.1: #include <array> #include <type_traits> struct range { const int min, max; }; static_assert(std::is_trivial_v<range>); static_assert(!std::is_default_constructible_v<range>); static_assert(std::is_copy_constructible_v<range>); auto my_array = std::to_array<range>({{0, 0}}); Output from g++ 13.3.0 on x86_64-pc-linux-gnu, built with "--enable-languages=c,c++ --prefix=/opt/gcc-13.3": $ /opt/gcc-13.3/bin/g++ -std=c++20 e.cxx -c In file included from e.cxx:1: /opt/gcc-13.3/include/c++/13.3.0/array: In instantiation of ‘constexpr std::array<typename std::remove_cv< <template-parameter-1-1> >::type, _Nm> std::to_array(_Tp (&&)[_Nm]) [with _Tp = range; long unsigned int _Nm = 1; typename remove_cv< <template-parameter-1-1> >::type = range]’: e.cxx:9:37: required from here /opt/gcc-13.3/include/c++/13.3.0/array:460:44: error: use of deleted function ‘std::array<range, 1>::array()’ 460 | array<remove_cv_t<_Tp>, _Nm> __arr; | ^~~~~ /opt/gcc-13.3/include/c++/13.3.0/array:94:12: note: ‘std::array<range, 1>::array()’ is implicitly deleted because the default definition would be ill-formed: 94 | struct array | ^~~~~ /opt/gcc-13.3/include/c++/13.3.0/array:94:12: error: use of deleted function ‘range::range()’ e.cxx:3:8: note: ‘range::range()’ is implicitly deleted because the default definition would be ill-formed: 3 | struct range { | ^~~~~ e.cxx:3:8: error: uninitialized const member in ‘struct range’ e.cxx:4:15: note: ‘const int range::min’ should be initialized 4 | const int min, max; | ^~~ e.cxx:3:8: error: uninitialized const member in ‘struct range’ 3 | struct range { | ^~~~~ e.cxx:4:20: note: ‘const int range::max’ should be initialized 4 | const int min, max; | ^~~ /opt/gcc-13.3/include/c++/13.3.0/array:465:39: error: use of deleted function ‘range& range::operator=(const range&)’ 465 | __arr._M_elems[__i] = __a[__i]; | ~~~~~~~~~~~~~~~~~~~~^~~~~~ e.cxx:3:8: note: ‘range& range::operator=(const range&)’ is implicitly deleted because the default definition would be ill-formed: 3 | struct range { | ^~~~~ e.cxx:3:8: error: non-static const member ‘const int range::min’, cannot use default assignment operator e.cxx:3:8: error: non-static const member ‘const int range::max’, cannot use default assignment operator It's not clear to me whether it should work in the first place, though it was my understanding that std::to_array does not necessarily require a default constructor, since it is closer to a move/copy operation.