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?
-- >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
--
2.45.2
