* [PATCH] c++: ICE with aggregate assignment and DMI [PR104583]
@ 2022-03-25 22:16 Marek Polacek
2022-03-30 2:19 ` Jason Merrill
0 siblings, 1 reply; 3+ messages in thread
From: Marek Polacek @ 2022-03-25 22:16 UTC (permalink / raw)
To: Jason Merrill, GCC Patches
The attached 93280 test no longer ICEs but looks like it was never added to the
testsuite. The 104583 test, modified so that it closely resembles 93280, still
ICEs.
The problem is that in 104583 we have a value-init from {} (the line A a{};),
so this code in convert_like_internal
7960 /* If we're initializing from {}, it's value-initialization. */
7961 if (BRACE_ENCLOSED_INITIALIZER_P (expr)
7962 && CONSTRUCTOR_NELTS (expr) == 0
7963 && TYPE_HAS_DEFAULT_CONSTRUCTOR (totype)
7964 && !processing_template_decl)
7965 {
7966 bool direct = CONSTRUCTOR_IS_DIRECT_INIT (expr);
...
7974 TARGET_EXPR_DIRECT_INIT_P (expr) = direct;
sets TARGET_EXPR_DIRECT_INIT_P. This does not happen in 93280 where we
initialize from {0}.
In 104583, when gimplifying, the d = {}; line, we have
d = {.a=TARGET_EXPR <D.2474, <<< Unknown tree: aggr_init_expr
4
__ct_comp
D.2474
(struct A *) <<< Unknown tree: void_cst >>> >>>>}
where the TARGET_EXPR is the one with TARGET_EXPR_DIRECT_INIT_P set. In
gimplify_init_ctor_preeval we do
4724 FOR_EACH_VEC_SAFE_ELT (v, ix, ce)
4725 gimplify_init_ctor_preeval (&ce->value, pre_p, post_p, data);
so we gimplify the TARGET_EXPR, crashing at
744 case TARGET_EXPR:
745 /* A TARGET_EXPR that expresses direct-initialization should have
been
746 elided by cp_gimplify_init_expr. */
747 gcc_checking_assert (!TARGET_EXPR_DIRECT_INIT_P (*expr_p));
but there is no INIT_EXPR so cp_gimplify_init_expr was never called!
Now, the fix for c++/93280
<https://gcc.gnu.org/pipermail/gcc-patches/2020-January/538414.html>
says "let's only set TARGET_EXPR_DIRECT_INIT_P when we're using the DMI in
a constructor." and the comment talks about the full initialization. Is
is accurate to say that our TARGET_EXPR does not represent the full
initialization, because it only initializes the 'a' subobject? If so,
then maybe get_nsdmi should clear TARGET_EXPR_DIRECT_INIT_P when in_ctor
is false.
I've compared the 93280.s and 104583.s files, they differ only in one
movl $0, so there are no extra calls and similar.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
PR c++/93280
PR c++/104583
gcc/cp/ChangeLog:
* init.cc (get_nsdmi): Set TARGET_EXPR_DIRECT_INIT_P to in_ctor.
gcc/testsuite/ChangeLog:
* g++.dg/cpp0x/nsdmi-list7.C: New test.
* g++.dg/cpp0x/nsdmi-list8.C: New test.
---
gcc/cp/init.cc | 8 ++++----
gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C | 17 +++++++++++++++++
gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C | 17 +++++++++++++++++
3 files changed, 38 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C
create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C
diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
index 08767679dd4..fd32a8bd90f 100644
--- a/gcc/cp/init.cc
+++ b/gcc/cp/init.cc
@@ -679,10 +679,10 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain)
if (simple_target)
init = TARGET_EXPR_INITIAL (init);
init = break_out_target_exprs (init, /*loc*/true);
- if (in_ctor && init && TREE_CODE (init) == TARGET_EXPR)
- /* This expresses the full initialization, prevent perform_member_init from
- calling another constructor (58162). */
- TARGET_EXPR_DIRECT_INIT_P (init) = true;
+ if (init && TREE_CODE (init) == TARGET_EXPR)
+ /* If this expresses the full initialization, prevent perform_member_init
+ from calling another constructor (58162). */
+ TARGET_EXPR_DIRECT_INIT_P (init) = in_ctor;
if (simple_target && TREE_CODE (init) != CONSTRUCTOR)
/* Now put it back so C++17 copy elision works. */
init = get_target_expr (init);
diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C
new file mode 100644
index 00000000000..62b07429bec
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C
@@ -0,0 +1,17 @@
+// PR c++/93280
+// { dg-do compile { target c++11 } }
+
+struct A {
+ template <typename T> A(T);
+ int c;
+};
+
+struct D {
+ A a{0};
+};
+
+void g()
+{
+ D d;
+ d = {};
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C
new file mode 100644
index 00000000000..fe73da8f98d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C
@@ -0,0 +1,17 @@
+// PR c++/104583
+// { dg-do compile { target c++11 } }
+
+struct A {
+ A();
+ int c;
+};
+
+struct D {
+ A a{};
+};
+
+void g()
+{
+ D d;
+ d = {};
+}
base-commit: bdd7b679e8497c07e25726f6ab6429e4c4d429c7
--
2.35.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] c++: ICE with aggregate assignment and DMI [PR104583]
2022-03-25 22:16 [PATCH] c++: ICE with aggregate assignment and DMI [PR104583] Marek Polacek
@ 2022-03-30 2:19 ` Jason Merrill
2022-03-30 13:55 ` Marek Polacek
0 siblings, 1 reply; 3+ messages in thread
From: Jason Merrill @ 2022-03-30 2:19 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
On 3/25/22 18:16, Marek Polacek wrote:
> The attached 93280 test no longer ICEs but looks like it was never added to the
> testsuite. The 104583 test, modified so that it closely resembles 93280, still
> ICEs.
>
> The problem is that in 104583 we have a value-init from {} (the line A a{};),
> so this code in convert_like_internal
>
> 7960 /* If we're initializing from {}, it's value-initialization. */
> 7961 if (BRACE_ENCLOSED_INITIALIZER_P (expr)
> 7962 && CONSTRUCTOR_NELTS (expr) == 0
> 7963 && TYPE_HAS_DEFAULT_CONSTRUCTOR (totype)
> 7964 && !processing_template_decl)
> 7965 {
> 7966 bool direct = CONSTRUCTOR_IS_DIRECT_INIT (expr);
> ...
> 7974 TARGET_EXPR_DIRECT_INIT_P (expr) = direct;
>
> sets TARGET_EXPR_DIRECT_INIT_P. This does not happen in 93280 where we
> initialize from {0}.
>
> In 104583, when gimplifying, the d = {}; line, we have
>
> d = {.a=TARGET_EXPR <D.2474, <<< Unknown tree: aggr_init_expr
> 4
> __ct_comp
> D.2474
> (struct A *) <<< Unknown tree: void_cst >>> >>>>}
>
> where the TARGET_EXPR is the one with TARGET_EXPR_DIRECT_INIT_P set. In
> gimplify_init_ctor_preeval we do
>
> 4724 FOR_EACH_VEC_SAFE_ELT (v, ix, ce)
> 4725 gimplify_init_ctor_preeval (&ce->value, pre_p, post_p, data);
>
> so we gimplify the TARGET_EXPR, crashing at
>
> 744 case TARGET_EXPR:
> 745 /* A TARGET_EXPR that expresses direct-initialization should have
> been
> 746 elided by cp_gimplify_init_expr. */
> 747 gcc_checking_assert (!TARGET_EXPR_DIRECT_INIT_P (*expr_p));
>
> but there is no INIT_EXPR so cp_gimplify_init_expr was never called!
>
> Now, the fix for c++/93280
> <https://gcc.gnu.org/pipermail/gcc-patches/2020-January/538414.html>
> says "let's only set TARGET_EXPR_DIRECT_INIT_P when we're using the DMI in
> a constructor." and the comment talks about the full initialization. Is
> is accurate to say that our TARGET_EXPR does not represent the full
> initialization, because it only initializes the 'a' subobject? If so,
> then maybe get_nsdmi should clear TARGET_EXPR_DIRECT_INIT_P when in_ctor
> is false.
>
> I've compared the 93280.s and 104583.s files, they differ only in one
> movl $0, so there are no extra calls and similar.
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>
> PR c++/93280
> PR c++/104583
>
> gcc/cp/ChangeLog:
>
> * init.cc (get_nsdmi): Set TARGET_EXPR_DIRECT_INIT_P to in_ctor.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp0x/nsdmi-list7.C: New test.
> * g++.dg/cpp0x/nsdmi-list8.C: New test.
> ---
> gcc/cp/init.cc | 8 ++++----
> gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C | 17 +++++++++++++++++
> gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C | 17 +++++++++++++++++
> 3 files changed, 38 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C
>
> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
> index 08767679dd4..fd32a8bd90f 100644
> --- a/gcc/cp/init.cc
> +++ b/gcc/cp/init.cc
> @@ -679,10 +679,10 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain)
> if (simple_target)
> init = TARGET_EXPR_INITIAL (init);
> init = break_out_target_exprs (init, /*loc*/true);
> - if (in_ctor && init && TREE_CODE (init) == TARGET_EXPR)
> - /* This expresses the full initialization, prevent perform_member_init from
> - calling another constructor (58162). */
> - TARGET_EXPR_DIRECT_INIT_P (init) = true;
> + if (init && TREE_CODE (init) == TARGET_EXPR)
> + /* If this expresses the full initialization, prevent perform_member_init
Maybe "In a constructor, this expresses..." ? The added "if" suggests a
test that I don't see in the code. OK with that change.
> + from calling another constructor (58162). */
> + TARGET_EXPR_DIRECT_INIT_P (init) = in_ctor;
> if (simple_target && TREE_CODE (init) != CONSTRUCTOR)
> /* Now put it back so C++17 copy elision works. */
> init = get_target_expr (init);
> diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C
> new file mode 100644
> index 00000000000..62b07429bec
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C
> @@ -0,0 +1,17 @@
> +// PR c++/93280
> +// { dg-do compile { target c++11 } }
> +
> +struct A {
> + template <typename T> A(T);
> + int c;
> +};
> +
> +struct D {
> + A a{0};
> +};
> +
> +void g()
> +{
> + D d;
> + d = {};
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C b/gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C
> new file mode 100644
> index 00000000000..fe73da8f98d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C
> @@ -0,0 +1,17 @@
> +// PR c++/104583
> +// { dg-do compile { target c++11 } }
> +
> +struct A {
> + A();
> + int c;
> +};
> +
> +struct D {
> + A a{};
> +};
> +
> +void g()
> +{
> + D d;
> + d = {};
> +}
>
> base-commit: bdd7b679e8497c07e25726f6ab6429e4c4d429c7
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] c++: ICE with aggregate assignment and DMI [PR104583]
2022-03-30 2:19 ` Jason Merrill
@ 2022-03-30 13:55 ` Marek Polacek
0 siblings, 0 replies; 3+ messages in thread
From: Marek Polacek @ 2022-03-30 13:55 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Tue, Mar 29, 2022 at 10:19:47PM -0400, Jason Merrill wrote:
> On 3/25/22 18:16, Marek Polacek wrote:
> > The attached 93280 test no longer ICEs but looks like it was never added to the
> > testsuite. The 104583 test, modified so that it closely resembles 93280, still
> > ICEs.
> >
> > The problem is that in 104583 we have a value-init from {} (the line A a{};),
> > so this code in convert_like_internal
> >
> > 7960 /* If we're initializing from {}, it's value-initialization. */
> > 7961 if (BRACE_ENCLOSED_INITIALIZER_P (expr)
> > 7962 && CONSTRUCTOR_NELTS (expr) == 0
> > 7963 && TYPE_HAS_DEFAULT_CONSTRUCTOR (totype)
> > 7964 && !processing_template_decl)
> > 7965 {
> > 7966 bool direct = CONSTRUCTOR_IS_DIRECT_INIT (expr);
> > ...
> > 7974 TARGET_EXPR_DIRECT_INIT_P (expr) = direct;
> >
> > sets TARGET_EXPR_DIRECT_INIT_P. This does not happen in 93280 where we
> > initialize from {0}.
> >
> > In 104583, when gimplifying, the d = {}; line, we have
> >
> > d = {.a=TARGET_EXPR <D.2474, <<< Unknown tree: aggr_init_expr
> > 4
> > __ct_comp
> > D.2474
> > (struct A *) <<< Unknown tree: void_cst >>> >>>>}
> >
> > where the TARGET_EXPR is the one with TARGET_EXPR_DIRECT_INIT_P set. In
> > gimplify_init_ctor_preeval we do
> >
> > 4724 FOR_EACH_VEC_SAFE_ELT (v, ix, ce)
> > 4725 gimplify_init_ctor_preeval (&ce->value, pre_p, post_p, data);
> >
> > so we gimplify the TARGET_EXPR, crashing at
> >
> > 744 case TARGET_EXPR:
> > 745 /* A TARGET_EXPR that expresses direct-initialization should have
> > been
> > 746 elided by cp_gimplify_init_expr. */
> > 747 gcc_checking_assert (!TARGET_EXPR_DIRECT_INIT_P (*expr_p));
> >
> > but there is no INIT_EXPR so cp_gimplify_init_expr was never called!
> >
> > Now, the fix for c++/93280
> > <https://gcc.gnu.org/pipermail/gcc-patches/2020-January/538414.html>
> > says "let's only set TARGET_EXPR_DIRECT_INIT_P when we're using the DMI in
> > a constructor." and the comment talks about the full initialization. Is
> > is accurate to say that our TARGET_EXPR does not represent the full
> > initialization, because it only initializes the 'a' subobject? If so,
> > then maybe get_nsdmi should clear TARGET_EXPR_DIRECT_INIT_P when in_ctor
> > is false.
> >
> > I've compared the 93280.s and 104583.s files, they differ only in one
> > movl $0, so there are no extra calls and similar.
> >
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> >
> > PR c++/93280
> > PR c++/104583
> >
> > gcc/cp/ChangeLog:
> >
> > * init.cc (get_nsdmi): Set TARGET_EXPR_DIRECT_INIT_P to in_ctor.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/cpp0x/nsdmi-list7.C: New test.
> > * g++.dg/cpp0x/nsdmi-list8.C: New test.
> > ---
> > gcc/cp/init.cc | 8 ++++----
> > gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C | 17 +++++++++++++++++
> > gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C | 17 +++++++++++++++++
> > 3 files changed, 38 insertions(+), 4 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-list7.C
> > create mode 100644 gcc/testsuite/g++.dg/cpp0x/nsdmi-list8.C
> >
> > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
> > index 08767679dd4..fd32a8bd90f 100644
> > --- a/gcc/cp/init.cc
> > +++ b/gcc/cp/init.cc
> > @@ -679,10 +679,10 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain)
> > if (simple_target)
> > init = TARGET_EXPR_INITIAL (init);
> > init = break_out_target_exprs (init, /*loc*/true);
> > - if (in_ctor && init && TREE_CODE (init) == TARGET_EXPR)
> > - /* This expresses the full initialization, prevent perform_member_init from
> > - calling another constructor (58162). */
> > - TARGET_EXPR_DIRECT_INIT_P (init) = true;
> > + if (init && TREE_CODE (init) == TARGET_EXPR)
> > + /* If this expresses the full initialization, prevent perform_member_init
>
> Maybe "In a constructor, this expresses..." ? The added "if" suggests a
> test that I don't see in the code. OK with that change.
Ah, the "if" was meant to be about the value of in_ctor. I've changed the
wording and pushed the patch. Thanks!
Marek
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-30 13:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-25 22:16 [PATCH] c++: ICE with aggregate assignment and DMI [PR104583] Marek Polacek
2022-03-30 2:19 ` Jason Merrill
2022-03-30 13:55 ` Marek Polacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).