Re: [PATCH v2] c++: fix array initialization wrong code [PR126335]
Marek Polacek <[email protected]>
| Newsgroups | gmane.comp.gcc.patches |
|---|---|
| Message-ID | <[email protected]> |
On Thu, Jul 30, 2026 at 11:48:55AM -0400, Jason Merrill wrote:
> On 7/27/26 6:44 PM, Marek Polacek wrote:
> > On Sun, Jul 26, 2026 at 10:46:28AM -0400, Jason Merrill wrote:
> > > On 7/24/26 6:59 PM, Marek Polacek wrote:
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > >
> > > > -- >8 --
> > > > This is a wrong-code problem starting with the recent check_initializer
> > > > simplification (r17-1661). I thought the fix would be to bring some of
> > > > those dropped conditions back, but now I think the change just uncovered
> > > > a latent bug.
> > > >
> > > > Since r17-1661, when initializing 'm' of type 'M[2]' we no longer call
> > > > build_aggr_init_full_exprs in check_initializer; instead, we go on to
> > > > store_init_value -> split_nonconstant_init. There we arrive with:
> > > >
> > > > {{.a=TARGET_EXPR <D.3136, <<< Unknown tree: aggr_init_expr
> > > > 3
> > > > operator""_s
> > > > D.3136 >>>>, .b=TARGET_EXPR <D.3139, <<< Unknown tree: aggr_init_expr
> > > > 3
> > > > operator""_s
> > > > D.3139 >>>>}, {.a={.p=&empty.str}, .b={.p=&empty.str}}}
> > > >
> > > > which so far seems OK. The type is an array so split_nonconstant_init_1
> > > > delegates to build_vec_init and returns true which, as the comment says,
> > > > should mean that "the whole of the value was initialized by the generated
> > > > statements". This is inaccurate: since try_const and do_static_init are
> > > > both true in build_vec_init, we have split out the constant initializer
> > > > (the {.a={.p=&empty.str}, .b={.p=&empty.str}} part) into DECL_INITIAL:
> > > >
> > > > 5374 else if (do_static_init && !vec_safe_is_empty (const_vec))
> > > > 5375 DECL_INITIAL (obase) = build_constructor (atype, const_vec);
> > > >
> > > > so we have both dynamic and static initializers. But since
> > > > split_nonconstant_init_1 returns bool, it's not ready to signal this case
> > > > to split_nonconstant_init, which then does:
> > > >
> > > > 943 if (split_nonconstant_init_1 (dest, init, true, &flags))
> > > > 944 init = NULL_TREE;
> > > >
> > > > and then overwrites DECL_INITIAL (dest). So we've lost a half of the
> > > > initializer and got wrong-code as the result.
> > > >
> > > > This patch fixes it by not throwing away the DECL_INITIAL that
> > > > build_vec_init set for us. I suppose another approach would be
> > > > to somehow change split_nonconstant_init_1/ARRAY_TYPE to follow
> > > > the element pruning/add_stmt like the rest of the function, but that
> > > > seems more complicated.
> > >
> > > Hmm, tricky.
> > >
> > > So split_nonconstant_init expects that _1 will remove elements from init as
> > > needed, and return true if it removed everything. In the non-array case, _1
> > > leaves any constant elements in init, so we want to put init in
> > > DECL_INITIAL, but in the array case it puts them in DECL_INITIAL and signals
> > > that init no longer contains anything useful. So your change to make no
> > > change to DECL_INITIAL makes sense.
> > >
> > > I'd adjust the _1 function comment to reflect this expanded sense, e.g.
> > > "...by the generated statements or modifying DECL_INITIAL"
> >
> > Done.
> >
> > > I'm nervous about a case where DECL_INITIAL was already set, we're not
> > > dealing with an array, and split out everything, and now would not properly
> > > clear DECL_INITIAL.
> > >
> > > Maybe we should clear DECL_INITIAL before recursing into
> > > split_nonconstant_init_1, or assert that it's already null?
> >
> > That's a good idea, done. Thanks!
> >
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> >
> > -- >8 --
> > This is a wrong-code problem starting with the recent check_initializer
> > simplification (r17-1661). I thought the fix would be to bring some of
> > those dropped conditions back, but now I think the change just uncovered
> > a latent bug.
> >
> > Since r17-1661, when initializing 'm' of type 'M[2]' we no longer call
> > build_aggr_init_full_exprs in check_initializer; instead, we go on to
> > store_init_value -> split_nonconstant_init. There we arrive with:
> >
> > {{.a=TARGET_EXPR <D.3136, <<< Unknown tree: aggr_init_expr
> > 3
> > operator""_s
> > D.3136 >>>>, .b=TARGET_EXPR <D.3139, <<< Unknown tree: aggr_init_expr
> > 3
> > operator""_s
> > D.3139 >>>>}, {.a={.p=&empty.str}, .b={.p=&empty.str}}}
> >
> > which so far seems OK. The type is an array so split_nonconstant_init_1
> > delegates to build_vec_init and returns true which, as the comment says,
> > should mean that "the whole of the value was initialized by the generated
> > statements". This is inaccurate: since try_const and do_static_init are
> > both true in build_vec_init, we have split out the constant initializer
> > (the {.a={.p=&empty.str}, .b={.p=&empty.str}} part) into DECL_INITIAL:
> >
> > 5374 else if (do_static_init && !vec_safe_is_empty (const_vec))
> > 5375 DECL_INITIAL (obase) = build_constructor (atype, const_vec);
> >
> > so we have both dynamic and static initializers. But since
> > split_nonconstant_init_1 returns bool, it's not ready to signal this case
> > to split_nonconstant_init, which then does:
> >
> > 943 if (split_nonconstant_init_1 (dest, init, true, &flags))
> > 944 init = NULL_TREE;
> >
> > and then overwrites DECL_INITIAL (dest). So we've lost a half of the
> > initializer and got wrong-code as the result.
> >
> > This patch fixes it by not throwing away the DECL_INITIAL that
> > build_vec_init set for us. I suppose another approach would be
> > to somehow change split_nonconstant_init_1/ARRAY_TYPE to follow
> > the element pruning/add_stmt like the rest of the function, but that
> > seems more complicated.
> >
> > PR c++/126335
> >
> > gcc/cp/ChangeLog:
> >
> > * typeck2.cc (split_nonconstant_init): Assert that DECL_INITIAL
> > is initially null. Don't clear DECL_INITIAL if build_vec_init
> > set it.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/init/array68.C: New test.
> > ---
> > gcc/cp/typeck2.cc | 18 ++++++++++++++++--
> > gcc/testsuite/g++.dg/init/array68.C | 19 +++++++++++++++++++
> > 2 files changed, 35 insertions(+), 2 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/init/array68.C
> >
> > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
> > index 2d7c65c3a51..d94a7939aa9 100644
> > --- a/gcc/cp/typeck2.cc
> > +++ b/gcc/cp/typeck2.cc
> > @@ -671,7 +671,7 @@ build_disable_temp_cleanup (tree f)
> > /* The recursive part of split_nonconstant_init. DEST is an lvalue
> > expression to which INIT should be assigned. INIT is a CONSTRUCTOR.
> > Return true if the whole of the value was initialized by the
> > - generated statements. */
> > + generated statements or modifying DECL_INITIAL. */
> > static bool
> > split_nonconstant_init_1 (tree dest, tree init, bool last,
> > @@ -940,6 +940,13 @@ split_nonconstant_init (tree dest, tree init)
> > if (TREE_CODE (TREE_TYPE (dest)) != ARRAY_TYPE)
> > flags = make_tree_vector ();
> > + /* We are about to call split_nonconstant_init_1 which might
> > + set DECL_INITIAL, so make sure we aren't overwriting an
> > + existing initializer. Also, if we split out everything,
> > + we clear INIT so won't set DECL_INITIAL below. Make
> > + sure it's null so that we're not forgetting to clear it. */
> > + gcc_assert (!(VAR_P (dest) && DECL_INITIAL (dest)));
> > +
> > if (split_nonconstant_init_1 (dest, init, true, &flags))
> > init = NULL_TREE;
> > @@ -950,7 +957,14 @@ split_nonconstant_init (tree dest, tree init)
> > code = pop_stmt_list (code);
> > if (VAR_P (dest) && !is_local_temp (dest))
> > {
> > - DECL_INITIAL (dest) = init;
> > + /* If we are initializing an array, split_nonconstant_init_1
> > + might've delegated to build_vec_init in which case it always
> > + returns true so we clear INIT. But if we're initializing
> > + a static array, build_vec_init can put constant initializers
> > + into DECL_INITIAL. Clearing it would mean losing some of the
> > + initializers as in c++/126335. */
> > + if (init)
> > + DECL_INITIAL (dest) = init;
> > TREE_READONLY (dest) = 0;
>
> While we're looking at this, maybe we want to only clear TREE_READONLY if
> TREE_SIDE_EFFECTS (code)?
>
> OK either way.
Ok. Here's what I pushed after the usual testing, thanks.
-- >8 --
This is a wrong-code problem starting with the recent check_initializer
simplification (r17-1661). I thought the fix would be to bring some of
those dropped conditions back, but now I think the change just uncovered
a latent bug.
Since r17-1661, when initializing 'm' of type 'M[2]' we no longer call
build_aggr_init_full_exprs in check_initializer; instead, we go on to
store_init_value -> split_nonconstant_init. There we arrive with:
{{.a=TARGET_EXPR <D.3136, <<< Unknown tree: aggr_init_expr
3
operator""_s
D.3136 >>>>, .b=TARGET_EXPR <D.3139, <<< Unknown tree: aggr_init_expr
3
operator""_s
D.3139 >>>>}, {.a={.p=&empty.str}, .b={.p=&empty.str}}}
which so far seems OK. The type is an array so split_nonconstant_init_1
delegates to build_vec_init and returns true which, as the comment says,
should mean that "the whole of the value was initialized by the generated
statements". This is inaccurate: since try_const and do_static_init are
both true in build_vec_init, we have split out the constant initializer
(the {.a={.p=&empty.str}, .b={.p=&empty.str}} part) into DECL_INITIAL:
5374 else if (do_static_init && !vec_safe_is_empty (const_vec))
5375 DECL_INITIAL (obase) = build_constructor (atype, const_vec);
so we have both dynamic and static initializers. But since
split_nonconstant_init_1 returns bool, it's not ready to signal this case
to split_nonconstant_init, which then does:
943 if (split_nonconstant_init_1 (dest, init, true, &flags))
944 init = NULL_TREE;
and then overwrites DECL_INITIAL (dest). So we've lost a half of the
initializer and got wrong-code as the result.
This patch fixes it by not throwing away the DECL_INITIAL that
build_vec_init set for us. I suppose another approach would be
to somehow change split_nonconstant_init_1/ARRAY_TYPE to follow
the element pruning/add_stmt like the rest of the function, but that
seems more complicated.
PR c++/126335
gcc/cp/ChangeLog:
* typeck2.cc (split_nonconstant_init): Assert that DECL_INITIAL
is initially null. Don't clear DECL_INITIAL if build_vec_init
set it. Only clear TREE_READONLY if CODE has side-effects.
gcc/testsuite/ChangeLog:
* g++.dg/init/array68.C: New test.
Reviewed-by: Jason Merrill <[email protected]>
---
gcc/cp/typeck2.cc | 21 ++++++++++++++++++---
gcc/testsuite/g++.dg/init/array68.C | 19 +++++++++++++++++++
2 files changed, 37 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/init/array68.C
diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
index 2d7c65c3a51..6c4ed50cb94 100644
--- a/gcc/cp/typeck2.cc
+++ b/gcc/cp/typeck2.cc
@@ -671,7 +671,7 @@ build_disable_temp_cleanup (tree f)
/* The recursive part of split_nonconstant_init. DEST is an lvalue
expression to which INIT should be assigned. INIT is a CONSTRUCTOR.
Return true if the whole of the value was initialized by the
- generated statements. */
+ generated statements or modifying DECL_INITIAL. */
static bool
split_nonconstant_init_1 (tree dest, tree init, bool last,
@@ -940,6 +940,13 @@ split_nonconstant_init (tree dest, tree init)
if (TREE_CODE (TREE_TYPE (dest)) != ARRAY_TYPE)
flags = make_tree_vector ();
+ /* We are about to call split_nonconstant_init_1 which might
+ set DECL_INITIAL, so make sure we aren't overwriting an
+ existing initializer. Also, if we split out everything,
+ we clear INIT so won't set DECL_INITIAL below. Make
+ sure it's null so that we're not forgetting to clear it. */
+ gcc_assert (!(VAR_P (dest) && DECL_INITIAL (dest)));
+
if (split_nonconstant_init_1 (dest, init, true, &flags))
init = NULL_TREE;
@@ -950,8 +957,16 @@ split_nonconstant_init (tree dest, tree init)
code = pop_stmt_list (code);
if (VAR_P (dest) && !is_local_temp (dest))
{
- DECL_INITIAL (dest) = init;
- TREE_READONLY (dest) = 0;
+ /* If we are initializing an array, split_nonconstant_init_1
+ might've delegated to build_vec_init in which case it always
+ returns true so we clear INIT. But if we're initializing
+ a static array, build_vec_init can put constant initializers
+ into DECL_INITIAL. Clearing it would mean losing some of the
+ initializers as in c++/126335. */
+ if (init)
+ DECL_INITIAL (dest) = init;
+ if (TREE_SIDE_EFFECTS (code))
+ TREE_READONLY (dest) = 0;
}
else if (init)
{
diff --git a/gcc/testsuite/g++.dg/init/array68.C b/gcc/testsuite/g++.dg/init/array68.C
new file mode 100644
index 00000000000..f9afda7eb53
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/array68.C
@@ -0,0 +1,19 @@
+// PR c++/126335
+// { dg-do run { target c++20 } }
+
+struct D {};
+struct Lit {
+ constexpr Lit(char const *) {}
+ D str;
+};
+auto empty = Lit("");
+struct S {
+ D * p;
+ constexpr S() { p = &empty.str; }
+ S(Lit) {}
+ ~S() {}
+};
+template<Lit L> S operator ""_s() { return L; }
+struct M { S a, b; };
+static M m[2]{{""_s, ""_s}, {}};
+int main() { return m[1].a.p == nullptr; }
base-commit: 32657f29f918712ad7110bd54d8ebb3bf6b0a2a8
--
2.55.0