| Message ID | ZqlhfPSdIUNW_GH7@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] c++: array new with value-initialization, again [PR115645] | expand |
On 7/30/24 5:56 PM, Marek Polacek wrote: > On Tue, Jul 30, 2024 at 05:38:37PM -0400, Jason Merrill wrote: >> On 7/30/24 4:59 PM, Marek Polacek wrote: >>> On Mon, Jul 29, 2024 at 06:34:40PM -0400, Jason Merrill wrote: >>>> On 7/29/24 4:18 PM, Marek Polacek wrote: >>>>> On Tue, Jul 23, 2024 at 05:18:52PM -0400, Jason Merrill wrote: >>>>>> On 7/17/24 5:33 PM, Marek Polacek wrote: >>>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>>>> >>>>>> Hmm, I thought I had replied to this already. >>>>>> >>>>>>> -- >8 -- >>>>>>> Unfortunately, my r15-1946 fix broke the attached testcase. In it, >>>>>>> we no longer go into the >>>>>>> /* P1009: Array size deduction in new-expressions. */ >>>>>>> block, and instead generate an operator new [] call along with a loop >>>>>>> in build_new_1, which we can't constexpr-evaluate. So this patch >>>>>>> reverts r15-1946 and uses CONSTRUCTOR_IS_PAREN_INIT to distinguish >>>>>>> between () and {} to fix the original testcase (anew7.C). >>>>>>> >>>>>>> PR c++/115645 >>>>>>> >>>>>>> gcc/cp/ChangeLog: >>>>>>> >>>>>>> * call.cc (convert_like_internal) <case ck_user>: Don't report errors >>>>>>> about calling an explicit constructor when the constructor was marked >>>>>>> CONSTRUCTOR_IS_PAREN_INIT. >>>>>>> * init.cc (build_new): Revert r15-1946. Set CONSTRUCTOR_IS_PAREN_INIT. >>>>>>> (build_vec_init): Maybe set CONSTRUCTOR_IS_PAREN_INIT. >>>>>>> >>>>>>> gcc/testsuite/ChangeLog: >>>>>>> >>>>>>> * g++.dg/cpp2a/constexpr-new23.C: New test. >>>>>>> --- >>>>>>> gcc/cp/call.cc | 2 ++ >>>>>>> gcc/cp/init.cc | 17 ++++----- >>>>>>> gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C | 38 ++++++++++++++++++++ >>>>>>> 3 files changed, 49 insertions(+), 8 deletions(-) >>>>>>> create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C >>>>>>> >>>>>>> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc >>>>>>> index a5d3426b70c..2d94d5e0d07 100644 >>>>>>> --- a/gcc/cp/call.cc >>>>>>> +++ b/gcc/cp/call.cc >>>>>>> @@ -8592,6 +8592,8 @@ convert_like_internal (conversion *convs, tree expr, tree fn, int argnum, >>>>>>> && BRACE_ENCLOSED_INITIALIZER_P (expr) >>>>>>> /* Unless this is for direct-list-initialization. */ >>>>>>> && (!CONSTRUCTOR_IS_DIRECT_INIT (expr) || convs->need_temporary_p) >>>>>>> + /* And it wasn't a ()-init. */ >>>>>>> + && !CONSTRUCTOR_IS_PAREN_INIT (expr) >>>>>>> /* And in C++98 a default constructor can't be explicit. */ >>>>>>> && cxx_dialect >= cxx11) >>>>>>> { >>>>>>> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc >>>>>>> index e9561c146d7..4138a6077dd 100644 >>>>>>> --- a/gcc/cp/init.cc >>>>>>> +++ b/gcc/cp/init.cc >>>>>>> @@ -4005,20 +4005,17 @@ build_new (location_t loc, vec<tree, va_gc> **placement, tree type, >>>>>>> /* P1009: Array size deduction in new-expressions. */ >>>>>>> const bool array_p = TREE_CODE (type) == ARRAY_TYPE; >>>>>>> if (*init >>>>>>> - /* If the array didn't specify its bound, we have to deduce it. */ >>>>>>> - && ((array_p && !TYPE_DOMAIN (type)) >>>>>>> - /* For C++20 array with parenthesized-init, we have to process >>>>>>> - the parenthesized-list. But don't do it for (), which is >>>>>>> - value-initialization, and INIT should stay empty. */ >>>>>>> - || (cxx_dialect >= cxx20 >>>>>>> - && (array_p || nelts) >>>>>>> - && !(*init)->is_empty ()))) >>>>>>> + /* If ARRAY_P, we have to deduce the array bound. For C++20 paren-init, >>>>>>> + we have to process the parenthesized-list. But don't do it for (), >>>>>>> + which is value-initialization, and INIT should stay empty. */ >>>>>>> + && (array_p || (cxx_dialect >= cxx20 && nelts && !(*init)->is_empty ()))) >>>>>>> { >>>>>>> /* This means we have 'new T[]()'. */ >>>>>>> if ((*init)->is_empty ()) >>>>>>> { >>>>>>> tree ctor = build_constructor (init_list_type_node, NULL); >>>>>>> CONSTRUCTOR_IS_DIRECT_INIT (ctor) = true; >>>>>>> + CONSTRUCTOR_IS_PAREN_INIT (ctor) = true; >>>>>>> vec_safe_push (*init, ctor); >>>>>>> } >>>>>>> tree &elt = (**init)[0]; >>>>>>> @@ -4735,6 +4732,9 @@ build_vec_init (tree base, tree maxindex, tree init, >>>>>>> bool do_static_init = (DECL_P (obase) && TREE_STATIC (obase)); >>>>>>> bool empty_list = false; >>>>>>> + const bool paren_init_p = (init >>>>>>> + && TREE_CODE (init) == CONSTRUCTOR >>>>>>> + && CONSTRUCTOR_IS_PAREN_INIT (init)); >>>>>> >>>>>> I think rather than recognizing paren-init in general, we want to recognize >>>>>> () specifically, and set explicit_value_init_p... >>>>>> >>>>>>> if (init && BRACE_ENCLOSED_INITIALIZER_P (init) >>>>>>> && CONSTRUCTOR_NELTS (init) == 0) >>>>>>> /* Skip over the handling of non-empty init lists. */ >>>>>>> @@ -4927,6 +4927,7 @@ build_vec_init (tree base, tree maxindex, tree init, >>>>>>> || TREE_CODE (type) == ARRAY_TYPE)) >>>>>>> { >>>>>>> init = build_constructor (init_list_type_node, NULL); >>>>>>> + CONSTRUCTOR_IS_PAREN_INIT (init) = paren_init_p; >>>>>>> } >>>>>>> else >>>>>>> { >>>>>> >>>>>> ...by taking the else branch here. Then we shouldn't need the convert_like >>>>>> change. >>>>> >>>>> Unfortunately that then breaks Jon's test (constexpr-new23.C which this >>>>> patch is adding). The problem is that if we do *not* create a new {}, and >>>>> do explicit_value_init_p, we end up with >>>>> >>>>> int[1] * D.2643; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) (D.2643 = (int[1] *) D.2642) >>>; >>>>> int[1] * D.2644; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) (D.2644 = D.2643) >>>; >>>>> TARGET_EXPR <D.2645, 0>; >>>>> <<< Unknown tree: for_stmt >>>>> D.2645 > -1 >>>>> <<cleanup_point <<< Unknown tree: expr_stmt >>>>> *(int[1] &) int * D.2646; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) (D.2646 = (int *) D.2644) >>>; >>>>> int * D.2647; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) (D.2647 = D.2646) >>>; >>>>> TARGET_EXPR <D.2648, 0>; >>>>> <<< Unknown tree: for_stmt >>>>> >>>>> D.2648 > -1 >>>>> >>>>> <<cleanup_point <<< Unknown tree: expr_stmt >>>>> *D.2647 = 0, --D.2648 >>>>>; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) ++D.2647 >>>; >>>>> >>>; >>>>> D.2646, --D.2645 >>>>>; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) ++D.2644 >>>; >>>>> >>>; >>>>> D.2643 >>>>> >>>>> rather than: >>>>> >>>>> int[1] * D.2643; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) (D.2643 = (int[1] *) D.2642) >>>; >>>>> int[1] * D.2644; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) (D.2644 = D.2643) >>>; >>>>> TARGET_EXPR <D.2645, 0>; >>>>> <<< Unknown tree: for_stmt >>>>> D.2645 > -1 >>>>> <<cleanup_point <<< Unknown tree: expr_stmt >>>>> *D.2644 = {}, --D.2645 >>>>>; >>>>> <<< Unknown tree: expr_stmt >>>>> (void) ++D.2644 >>>; >>>>> >>>; >>>>> D.2643 >>>>> >>>>> In the former, the "*D.2647 = 0" assignment is what breaks constexpr, >>>>> which then complains: >>>>> >>>>> constexpr-new23.C:16:16: error: accessing 'test_array()::U::arr' member instead of initialized 'test_array()::U::x' member in constant expression >>>>> 16 | return ::new((void*)p) T[1](); >>>>> | ^~~~~~~~~~~~~~~~~~~~~~ >>>>> constexpr-new23.C:31:9: note: initializing 'test_array()::U::arr' requires a member access expression as the left operand of the assignment >>>>> 31 | int arr[4]; >>>>> >>>>> >>>>> If there is no bug in constexpr, then it looks like we need to >>>>> initialize using a {} rather than a loop that assigns 0 to each >>>>> element. >>>> >>>> Ah, thanks. >>>> >>>> It looks like the first bug is that build_vec_init wrongly leaves try_const >>>> false for this case (without your patch) because int doesn't have a >>>> constexpr default constructor, failing to consider that value-initialization >>>> of scalars is constexpr. >>> >>> Oh wow, I should have noticed that. >>> >>>> Then, once we're into the looping initialization, we aren't expressing it in >>>> a way that will satisfy the strict checking in constexpr evaluation; it >>>> needs to initialize the array, not just its elements. >>>> >>>> I expect we could fix that with something like >>>> >>>>> /* Start array lifetime before accessing elements. */ >>>>> if (TREE_CODE (atype) == ARRAY_TYPE) >>>>> { >>>>> elt_init = build_constructor (atype, nullptr); >>>>> CONSTRUCTOR_NO_CLEARING (elt_init) = true; >>>>> for_stmt = build2 (INIT_EXPR, atype, obase, elt_init); >>>>> finish_expr_stmt (for_stmt); >>>>> } >>>> >>>> but if we're only concerned about constexpr, fixing the first bug ought to >>>> be enough; in constant evaluation if we don't get a constant initializer >>>> we're failing anyway. >>> >>> This patch fixes the first bug. Thanks! >>> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>> >>> -- >8 -- >>> Unfortunately, my r15-1946 fix broke the attached testcase; the >>> constexpr evaluation reported an error about not being able to >>> evaluate the code emitted by build_vec_init. Jason figured out >>> it's because we were wrongly setting try_const to false, where >>> in fact it should have been true. Value-initialization of scalars >>> is constexpr, so we should check that alongside of >>> type_has_constexpr_default_constructor. >>> >>> PR c++/115645 >>> >>> gcc/cp/ChangeLog: >>> >>> * init.cc (build_vec_init): When initializing a scalar type, try to >>> create a constant initializer. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/cpp2a/constexpr-new23.C: New test. >>> --- >>> gcc/cp/init.cc | 4 ++- >>> gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C | 38 ++++++++++++++++++++ >>> 2 files changed, 41 insertions(+), 1 deletion(-) >>> create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C >>> >>> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc >>> index e9561c146d7..a3a97e2c128 100644 >>> --- a/gcc/cp/init.cc >>> +++ b/gcc/cp/init.cc >>> @@ -4724,7 +4724,9 @@ build_vec_init (tree base, tree maxindex, tree init, >>> && TREE_CONSTANT (maxindex) >>> && (init ? TREE_CODE (init) == CONSTRUCTOR >>> : (type_has_constexpr_default_constructor >>> - (inner_elt_type))) >>> + (inner_elt_type) >>> + /* Value-initialization of scalars is constexpr. */ >>> + || SCALAR_TYPE_P (inner_elt_type))) >> >> I think we also want to check explicit_value_init_p, since default-init of >> scalars is not constexpr. >> >> I don't think we'd actually get here in that case, as callers like >> build_new_1 avoid calling build_vec_init at all, but I'd still like to be >> correct. > > Yeah, OK, updated. So far dg.exp passed. > > Bootstrapped/regtested running on x86_64-pc-linux-gnu, ok for trunk if all is > good? OK. > -- >8 -- > Unfortunately, my r15-1946 fix broke the attached testcase; the > constexpr evaluation reported an error about not being able to > evaluate the code emitted by build_vec_init. Jason figured out > it's because we were wrongly setting try_const to false, where > in fact it should have been true. Value-initialization of scalars > is constexpr, so we should check that alongside of > type_has_constexpr_default_constructor. > > PR c++/115645 > > gcc/cp/ChangeLog: > > * init.cc (build_vec_init): When initializing a scalar type, try to > create a constant initializer. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp2a/constexpr-new23.C: New test. > --- > gcc/cp/init.cc | 5 ++- > gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C | 38 ++++++++++++++++++++ > 2 files changed, 42 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C > > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > index e9561c146d7..de82152bd1d 100644 > --- a/gcc/cp/init.cc > +++ b/gcc/cp/init.cc > @@ -4724,7 +4724,10 @@ build_vec_init (tree base, tree maxindex, tree init, > && TREE_CONSTANT (maxindex) > && (init ? TREE_CODE (init) == CONSTRUCTOR > : (type_has_constexpr_default_constructor > - (inner_elt_type))) > + (inner_elt_type) > + /* Value-initialization of scalars is constexpr. */ > + || (explicit_value_init_p > + && SCALAR_TYPE_P (inner_elt_type)))) > && (literal_type_p (inner_elt_type) > || TYPE_HAS_CONSTEXPR_CTOR (inner_elt_type))); > vec<constructor_elt, va_gc> *const_vec = NULL; > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C > new file mode 100644 > index 00000000000..1abbef18fae > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C > @@ -0,0 +1,38 @@ > +// PR c++/115645 > +// { dg-do compile { target c++20 } } > + > +using size_t = decltype(sizeof(0)); > + > +void* operator new(size_t, void* p) { return p; } > +void* operator new[](size_t, void* p) { return p; } > + > +#define VERIFY(C) if (!(C)) throw > + > +namespace std { > + template<typename T> > + constexpr T* construct_at(T* p) > + { > + if constexpr (__is_array(T)) > + return ::new((void*)p) T[1](); > + else > + return ::new((void*)p) T(); > + } > +} > + > +constexpr void > +test_array() > +{ > + int arr[1] { 99 }; > + std::construct_at(&arr); > + VERIFY( arr[0] == 0 ); > + > + union U { > + long long x = -1; > + int arr[4]; > + } u; > + > + auto p = std::construct_at(&u.arr); > + VERIFY( (*p)[0] == 0 ); > +} > + > +static_assert( [] { test_array(); return true; }() ); > > base-commit: 4883c9571f5fb8fc7e873bb8a31aa164c5cfd0e0
diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc index e9561c146d7..de82152bd1d 100644 --- a/gcc/cp/init.cc +++ b/gcc/cp/init.cc @@ -4724,7 +4724,10 @@ build_vec_init (tree base, tree maxindex, tree init, && TREE_CONSTANT (maxindex) && (init ? TREE_CODE (init) == CONSTRUCTOR : (type_has_constexpr_default_constructor - (inner_elt_type))) + (inner_elt_type) + /* Value-initialization of scalars is constexpr. */ + || (explicit_value_init_p + && SCALAR_TYPE_P (inner_elt_type)))) && (literal_type_p (inner_elt_type) || TYPE_HAS_CONSTEXPR_CTOR (inner_elt_type))); vec<constructor_elt, va_gc> *const_vec = NULL; diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C new file mode 100644 index 00000000000..1abbef18fae --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-new23.C @@ -0,0 +1,38 @@ +// PR c++/115645 +// { dg-do compile { target c++20 } } + +using size_t = decltype(sizeof(0)); + +void* operator new(size_t, void* p) { return p; } +void* operator new[](size_t, void* p) { return p; } + +#define VERIFY(C) if (!(C)) throw + +namespace std { + template<typename T> + constexpr T* construct_at(T* p) + { + if constexpr (__is_array(T)) + return ::new((void*)p) T[1](); + else + return ::new((void*)p) T(); + } +} + +constexpr void +test_array() +{ + int arr[1] { 99 }; + std::construct_at(&arr); + VERIFY( arr[0] == 0 ); + + union U { + long long x = -1; + int arr[4]; + } u; + + auto p = std::construct_at(&u.arr); + VERIFY( (*p)[0] == 0 ); +} + +static_assert( [] { test_array(); return true; }() );