* [PATCH] c++: ICE with temporary of class type in DMI [PR100252] @ 2022-04-26 23:02 Marek Polacek 2022-04-27 15:00 ` Patrick Palka 0 siblings, 1 reply; 17+ messages in thread From: Marek Polacek @ 2022-04-26 23:02 UTC (permalink / raw) To: GCC Patches, Jason Merrill; +Cc: Jakub Jelinek, Patrick Palka Consider struct A { int x; int y = x; }; struct B { int x = 0; int y = A{x}.y; // #1 }; where for #1 we end up with {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} that is, two PLACEHOLDER_EXPRs for different types on the same level in a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal on type=A, compound_literal={((struct B *) this)->x}. When digesting this initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't have any object to refer to yet. After digesting, we have {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> Then we get to B b = {}; and call store_init_value, which digest the {}, which produces {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} The call to replace_placeholders in store_init_value will not do anything: we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> stays even though now is the perfect time to replace it because we have an object for it: 'b'. Later, in cp_gimplify_init_expr the *expr_p is D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} where D.2395 is of type A, but we crash because we hit <P_E struct B>, which has a different type. My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the TARGET_EXPR because that means we have an object we can refer to. Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to replace <P_E struct B> with 'b', and we should be good to go. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/11.4? PR c++/100252 gcc/cp/ChangeLog: * semantics.cc (finish_compound_literal): replace_placeholders after creating the TARGET_EXPR. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/nsdmi-aggr14.C: New test. --- gcc/cp/semantics.cc | 31 +++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index ab48f11c9be..770369458bb 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -3296,6 +3296,37 @@ finish_compound_literal (tree type, tree compound_literal, if (TREE_CODE (compound_literal) == CONSTRUCTOR) TREE_HAS_CONSTRUCTOR (compound_literal) = false; compound_literal = get_target_expr_sfinae (compound_literal, complain); + /* We may have A{} in a NSDMI. */ + if (parsing_nsdmi ()) + { + /* Digesting the {} could have introduced a PLACEHOLDER_EXPR + referring to A. Now that we've built up a TARGET_EXPR, we + have an object we can refer to. The reason we bother doing + this here is for code like + + struct A { + int x; + int y = x; + }; + + struct B { + int x = 0; + int y = A{x}.y; // #1 + }; + + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs + for different types on the same level in a {} as in 100252. */ + tree init = TARGET_EXPR_INITIAL (compound_literal); + if (TREE_CODE (init) == CONSTRUCTOR + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) + { + tree obj = TARGET_EXPR_SLOT (compound_literal); + replace_placeholders (compound_literal, obj); + /* We should have dealt with the PLACEHOLDER_EXPRs. */ + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; + gcc_checking_assert (!find_placeholders (init)); + } + } } else /* For e.g. int{42} just make sure it's a prvalue. */ diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C new file mode 100644 index 00000000000..7d508f52b48 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C @@ -0,0 +1,46 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +constexpr B csb1 = { }; +SA(csb1.x == 0 && csb1.y == csb1.x); +constexpr B csb2 = { 1 }; +SA(csb2.x == 1 && csb2.y == csb2.x); +constexpr B csb3 = { 1, 2 }; +SA(csb3.x == 1 && csb3.y == 2); + +B sb1 = { }; +B sb2 = { 1 }; +B sb3 = { 1, 2}; + +int +main () +{ + if (sb1.x != 0 || sb1.x != sb1.y) + __builtin_abort(); + if (sb2.x != 1 || sb2.x != sb2.y) + __builtin_abort(); + if (sb3.x != 1 || sb3.y != 2) + __builtin_abort(); + + B b1 = { }; + B b2 = { 1 }; + B b3 = { 1, 2}; + if (b1.x != 0 || b1.x != b1.y) + __builtin_abort(); + if (b2.x != 1 || b2.x != b2.y) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); +} base-commit: 9ace5d4dab2ab39072b0f07089621a823580f27c -- 2.35.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] c++: ICE with temporary of class type in DMI [PR100252] 2022-04-26 23:02 [PATCH] c++: ICE with temporary of class type in DMI [PR100252] Marek Polacek @ 2022-04-27 15:00 ` Patrick Palka 2022-04-27 20:30 ` [PATCH v2] " Marek Polacek 0 siblings, 1 reply; 17+ messages in thread From: Patrick Palka @ 2022-04-27 15:00 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Jason Merrill, Jakub Jelinek, Patrick Palka On Tue, 26 Apr 2022, Marek Polacek wrote: > Consider > > struct A { > int x; > int y = x; > }; > > struct B { > int x = 0; > int y = A{x}.y; // #1 > }; > > where for #1 we end up with > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > have any object to refer to yet. After digesting, we have > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > Then we get to > > B b = {}; > > and call store_init_value, which digest the {}, which produces > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > The call to replace_placeholders in store_init_value will not do anything: > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > stays even though now is the perfect time to replace it because we have an > object for it: 'b'. > > Later, in cp_gimplify_init_expr the *expr_p is > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > has a different type. > > My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the > TARGET_EXPR because that means we have an object we can refer to. Then clear > CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR > in the {}. Then store_init_value will be able to replace <P_E struct B> with > 'b', and we should be good to go. Makes sense to me. It seems all was well until break_out_target_exprs, called from get_nsdmi for B::y, replaced the 'this' in the initializer (TARGET_EXPR <D.2131, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; with a PLACEHOLDER_EXPR; (TARGET_EXPR <D.2142, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; This seems to be the wrong thing to do when the 'this' appears inside a CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructor because the new PLACEHOLDER_EXPR then can't be resolved correctly. So in light of this I wonder if we should instead perform this handling you added to finish_compound_literal in break_out_target_exprs / bot_manip instead? > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/11.4? > > PR c++/100252 > > gcc/cp/ChangeLog: > > * semantics.cc (finish_compound_literal): replace_placeholders after > creating the TARGET_EXPR. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/nsdmi-aggr14.C: New test. > --- > gcc/cp/semantics.cc | 31 +++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++++++ > 2 files changed, 77 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index ab48f11c9be..770369458bb 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -3296,6 +3296,37 @@ finish_compound_literal (tree type, tree compound_literal, > if (TREE_CODE (compound_literal) == CONSTRUCTOR) > TREE_HAS_CONSTRUCTOR (compound_literal) = false; > compound_literal = get_target_expr_sfinae (compound_literal, complain); > + /* We may have A{} in a NSDMI. */ > + if (parsing_nsdmi ()) > + { > + /* Digesting the {} could have introduced a PLACEHOLDER_EXPR > + referring to A. Now that we've built up a TARGET_EXPR, we > + have an object we can refer to. The reason we bother doing > + this here is for code like > + > + struct A { > + int x; > + int y = x; > + }; > + > + struct B { > + int x = 0; > + int y = A{x}.y; // #1 > + }; > + > + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs > + for different types on the same level in a {} as in 100252. */ > + tree init = TARGET_EXPR_INITIAL (compound_literal); > + if (TREE_CODE (init) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) > + { > + tree obj = TARGET_EXPR_SLOT (compound_literal); > + replace_placeholders (compound_literal, obj); > + /* We should have dealt with the PLACEHOLDER_EXPRs. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; > + gcc_checking_assert (!find_placeholders (init)); > + } > + } > } > else > /* For e.g. int{42} just make sure it's a prvalue. */ > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > new file mode 100644 > index 00000000000..7d508f52b48 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > @@ -0,0 +1,46 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +constexpr B csb1 = { }; > +SA(csb1.x == 0 && csb1.y == csb1.x); > +constexpr B csb2 = { 1 }; > +SA(csb2.x == 1 && csb2.y == csb2.x); > +constexpr B csb3 = { 1, 2 }; > +SA(csb3.x == 1 && csb3.y == 2); > + > +B sb1 = { }; > +B sb2 = { 1 }; > +B sb3 = { 1, 2}; > + > +int > +main () > +{ > + if (sb1.x != 0 || sb1.x != sb1.y) > + __builtin_abort(); > + if (sb2.x != 1 || sb2.x != sb2.y) > + __builtin_abort(); > + if (sb3.x != 1 || sb3.y != 2) > + __builtin_abort(); > + > + B b1 = { }; > + B b2 = { 1 }; > + B b3 = { 1, 2}; > + if (b1.x != 0 || b1.x != b1.y) > + __builtin_abort(); > + if (b2.x != 1 || b2.x != b2.y) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > > base-commit: 9ace5d4dab2ab39072b0f07089621a823580f27c > -- > 2.35.1 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] c++: ICE with temporary of class type in DMI [PR100252] 2022-04-27 15:00 ` Patrick Palka @ 2022-04-27 20:30 ` Marek Polacek 2022-04-28 14:12 ` Patrick Palka 2022-05-03 20:59 ` Jason Merrill 0 siblings, 2 replies; 17+ messages in thread From: Marek Polacek @ 2022-04-27 20:30 UTC (permalink / raw) To: Patrick Palka; +Cc: GCC Patches, Jason Merrill, Jakub Jelinek On Wed, Apr 27, 2022 at 11:00:46AM -0400, Patrick Palka wrote: > On Tue, 26 Apr 2022, Marek Polacek wrote: > > > Consider > > > > struct A { > > int x; > > int y = x; > > }; > > > > struct B { > > int x = 0; > > int y = A{x}.y; // #1 > > }; > > > > where for #1 we end up with > > > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > > have any object to refer to yet. After digesting, we have > > > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > > > Then we get to > > > > B b = {}; > > > > and call store_init_value, which digest the {}, which produces > > > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > > > The call to replace_placeholders in store_init_value will not do anything: > > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > > stays even though now is the perfect time to replace it because we have an > > object for it: 'b'. > > > > Later, in cp_gimplify_init_expr the *expr_p is > > > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > > has a different type. > > > > My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the > > TARGET_EXPR because that means we have an object we can refer to. Then clear > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR > > in the {}. Then store_init_value will be able to replace <P_E struct B> with > > 'b', and we should be good to go. > > Makes sense to me. It seems all was well until break_out_target_exprs, > called from get_nsdmi for B::y, replaced the 'this' in the initializer > > (TARGET_EXPR <D.2131, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; > > with a PLACEHOLDER_EXPR; > > (TARGET_EXPR <D.2142, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; > > This seems to be the wrong thing to do when the 'this' appears inside a > CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructor because the new > PLACEHOLDER_EXPR then can't be resolved correctly. Exactly. > So in light of this I wonder if we should instead perform this handling > you added to finish_compound_literal in break_out_target_exprs / > bot_manip instead? Unfortunately that causes an ICE in gimplify_var_or_parm_decl on the new testcase I've added here. bot_manip is a different context and so I can't use parsing_nsdmi anymore, and it seems we'd replace the placeholders too aggressively in bot_manip. So I'm not sure if that's the best place. -- >8 -- Consider struct A { int x; int y = x; }; struct B { int x = 0; int y = A{x}.y; // #1 }; where for #1 we end up with {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} that is, two PLACEHOLDER_EXPRs for different types on the same level in a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal on type=A, compound_literal={((struct B *) this)->x}. When digesting this initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't have any object to refer to yet. After digesting, we have {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> Then we get to B b = {}; and call store_init_value, which digest the {}, which produces {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} The call to replace_placeholders in store_init_value will not do anything: we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> stays even though now is the perfect time to replace it because we have an object for it: 'b'. Later, in cp_gimplify_init_expr the *expr_p is D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} where D.2395 is of type A, but we crash because we hit <P_E struct B>, which has a different type. My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the TARGET_EXPR because that means we have an object we can refer to. Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to replace <P_E struct B> with 'b', and we should be good to go. PR c++/100252 gcc/cp/ChangeLog: * semantics.cc (finish_compound_literal): replace_placeholders after creating the TARGET_EXPR. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/nsdmi-aggr14.C: New test. * g++.dg/cpp1y/nsdmi-aggr15.C: New test. --- gcc/cp/semantics.cc | 31 +++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++++ 3 files changed, 106 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc index ab48f11c9be..770369458bb 100644 --- a/gcc/cp/semantics.cc +++ b/gcc/cp/semantics.cc @@ -3296,6 +3296,37 @@ finish_compound_literal (tree type, tree compound_literal, if (TREE_CODE (compound_literal) == CONSTRUCTOR) TREE_HAS_CONSTRUCTOR (compound_literal) = false; compound_literal = get_target_expr_sfinae (compound_literal, complain); + /* We may have A{} in a NSDMI. */ + if (parsing_nsdmi ()) + { + /* Digesting the {} could have introduced a PLACEHOLDER_EXPR + referring to A. Now that we've built up a TARGET_EXPR, we + have an object we can refer to. The reason we bother doing + this here is for code like + + struct A { + int x; + int y = x; + }; + + struct B { + int x = 0; + int y = A{x}.y; // #1 + }; + + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs + for different types on the same level in a {} as in 100252. */ + tree init = TARGET_EXPR_INITIAL (compound_literal); + if (TREE_CODE (init) == CONSTRUCTOR + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) + { + tree obj = TARGET_EXPR_SLOT (compound_literal); + replace_placeholders (compound_literal, obj); + /* We should have dealt with the PLACEHOLDER_EXPRs. */ + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; + gcc_checking_assert (!find_placeholders (init)); + } + } } else /* For e.g. int{42} just make sure it's a prvalue. */ diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C new file mode 100644 index 00000000000..7d508f52b48 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C @@ -0,0 +1,46 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +constexpr B csb1 = { }; +SA(csb1.x == 0 && csb1.y == csb1.x); +constexpr B csb2 = { 1 }; +SA(csb2.x == 1 && csb2.y == csb2.x); +constexpr B csb3 = { 1, 2 }; +SA(csb3.x == 1 && csb3.y == 2); + +B sb1 = { }; +B sb2 = { 1 }; +B sb3 = { 1, 2}; + +int +main () +{ + if (sb1.x != 0 || sb1.x != sb1.y) + __builtin_abort(); + if (sb2.x != 1 || sb2.x != sb2.y) + __builtin_abort(); + if (sb3.x != 1 || sb3.y != 2) + __builtin_abort(); + + B b1 = { }; + B b2 = { 1 }; + B b3 = { 1, 2}; + if (b1.x != 0 || b1.x != b1.y) + __builtin_abort(); + if (b2.x != 1 || b2.x != b2.y) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C new file mode 100644 index 00000000000..bc997bb5e1d --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C @@ -0,0 +1,29 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +void +g (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) +{ + if (b1.x != 0 || b1.y != b1.x) + __builtin_abort(); + if (b2.x != 1 || b2.y != b2.x) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); +} + +int +main () +{ + g (); +} base-commit: 409edcca331296b53842c50d3b789e1b1ccc05e5 -- 2.35.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] c++: ICE with temporary of class type in DMI [PR100252] 2022-04-27 20:30 ` [PATCH v2] " Marek Polacek @ 2022-04-28 14:12 ` Patrick Palka 2022-04-28 14:39 ` Marek Polacek 2022-05-03 20:59 ` Jason Merrill 1 sibling, 1 reply; 17+ messages in thread From: Patrick Palka @ 2022-04-28 14:12 UTC (permalink / raw) To: Marek Polacek; +Cc: Patrick Palka, GCC Patches, Jason Merrill, Jakub Jelinek On Wed, 27 Apr 2022, Marek Polacek wrote: > On Wed, Apr 27, 2022 at 11:00:46AM -0400, Patrick Palka wrote: > > On Tue, 26 Apr 2022, Marek Polacek wrote: > > > > > Consider > > > > > > struct A { > > > int x; > > > int y = x; > > > }; > > > > > > struct B { > > > int x = 0; > > > int y = A{x}.y; // #1 > > > }; > > > > > > where for #1 we end up with > > > > > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > > > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > > > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > > > > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > > > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > > > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > > > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > > > have any object to refer to yet. After digesting, we have > > > > > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > > > > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > > > > > Then we get to > > > > > > B b = {}; > > > > > > and call store_init_value, which digest the {}, which produces > > > > > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > > > > > The call to replace_placeholders in store_init_value will not do anything: > > > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > > > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > > > stays even though now is the perfect time to replace it because we have an > > > object for it: 'b'. > > > > > > Later, in cp_gimplify_init_expr the *expr_p is > > > > > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > > > has a different type. > > > > > > My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the > > > TARGET_EXPR because that means we have an object we can refer to. Then clear > > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR > > > in the {}. Then store_init_value will be able to replace <P_E struct B> with > > > 'b', and we should be good to go. > > > > Makes sense to me. It seems all was well until break_out_target_exprs, > > called from get_nsdmi for B::y, replaced the 'this' in the initializer > > > > (TARGET_EXPR <D.2131, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; > > > > with a PLACEHOLDER_EXPR; > > > > (TARGET_EXPR <D.2142, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; > > > > This seems to be the wrong thing to do when the 'this' appears inside a > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructor because the new > > PLACEHOLDER_EXPR then can't be resolved correctly. > > Exactly. > > > So in light of this I wonder if we should instead perform this handling > > you added to finish_compound_literal in break_out_target_exprs / > > bot_manip instead? > > Unfortunately that causes an ICE in gimplify_var_or_parm_decl on the new > testcase I've added here. bot_manip is a different context and so I can't > use parsing_nsdmi anymore, and it seems we'd replace the placeholders too > aggressively in bot_manip. So I'm not sure if that's the best place. Ah :/ good catch... FWIW the transformation itself (doing replace_placeholders followed by clearing CONSTRUCTOR_PLACEHOLDER_BOUNDARY) makes sense to me, and I'm happy with doing it in finish_compound_literal when parsing_nsdmi, so the patch LGTM. (I guess we could also consider doing the transformation in get_nsdmi or digest_nsdmi_init via cp_walk_tree, but I don't have a preference either way..) > > -- >8 -- > Consider > > struct A { > int x; > int y = x; > }; > > struct B { > int x = 0; > int y = A{x}.y; // #1 > }; > > where for #1 we end up with > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > have any object to refer to yet. After digesting, we have > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > Then we get to > > B b = {}; > > and call store_init_value, which digest the {}, which produces > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > The call to replace_placeholders in store_init_value will not do anything: > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > stays even though now is the perfect time to replace it because we have an > object for it: 'b'. > > Later, in cp_gimplify_init_expr the *expr_p is > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > has a different type. > > My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the > TARGET_EXPR because that means we have an object we can refer to. Then clear > CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR > in the {}. Then store_init_value will be able to replace <P_E struct B> with > 'b', and we should be good to go. > > PR c++/100252 > > gcc/cp/ChangeLog: > > * semantics.cc (finish_compound_literal): replace_placeholders after > creating the TARGET_EXPR. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/nsdmi-aggr14.C: New test. > * g++.dg/cpp1y/nsdmi-aggr15.C: New test. > --- > gcc/cp/semantics.cc | 31 +++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++++ > 3 files changed, 106 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index ab48f11c9be..770369458bb 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -3296,6 +3296,37 @@ finish_compound_literal (tree type, tree compound_literal, > if (TREE_CODE (compound_literal) == CONSTRUCTOR) > TREE_HAS_CONSTRUCTOR (compound_literal) = false; > compound_literal = get_target_expr_sfinae (compound_literal, complain); > + /* We may have A{} in a NSDMI. */ > + if (parsing_nsdmi ()) > + { > + /* Digesting the {} could have introduced a PLACEHOLDER_EXPR > + referring to A. Now that we've built up a TARGET_EXPR, we > + have an object we can refer to. The reason we bother doing > + this here is for code like > + > + struct A { > + int x; > + int y = x; > + }; > + > + struct B { > + int x = 0; > + int y = A{x}.y; // #1 > + }; > + > + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs > + for different types on the same level in a {} as in 100252. */ > + tree init = TARGET_EXPR_INITIAL (compound_literal); > + if (TREE_CODE (init) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) > + { > + tree obj = TARGET_EXPR_SLOT (compound_literal); > + replace_placeholders (compound_literal, obj); > + /* We should have dealt with the PLACEHOLDER_EXPRs. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; > + gcc_checking_assert (!find_placeholders (init)); > + } > + } > } > else > /* For e.g. int{42} just make sure it's a prvalue. */ > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > new file mode 100644 > index 00000000000..7d508f52b48 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > @@ -0,0 +1,46 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +constexpr B csb1 = { }; > +SA(csb1.x == 0 && csb1.y == csb1.x); > +constexpr B csb2 = { 1 }; > +SA(csb2.x == 1 && csb2.y == csb2.x); > +constexpr B csb3 = { 1, 2 }; > +SA(csb3.x == 1 && csb3.y == 2); > + > +B sb1 = { }; > +B sb2 = { 1 }; > +B sb3 = { 1, 2}; > + > +int > +main () > +{ > + if (sb1.x != 0 || sb1.x != sb1.y) > + __builtin_abort(); > + if (sb2.x != 1 || sb2.x != sb2.y) > + __builtin_abort(); > + if (sb3.x != 1 || sb3.y != 2) > + __builtin_abort(); > + > + B b1 = { }; > + B b2 = { 1 }; > + B b3 = { 1, 2}; > + if (b1.x != 0 || b1.x != b1.y) > + __builtin_abort(); > + if (b2.x != 1 || b2.x != b2.y) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > new file mode 100644 > index 00000000000..bc997bb5e1d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > @@ -0,0 +1,29 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +void > +g (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) > +{ > + if (b1.x != 0 || b1.y != b1.x) > + __builtin_abort(); > + if (b2.x != 1 || b2.y != b2.x) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > + > +int > +main () > +{ > + g (); > +} > > base-commit: 409edcca331296b53842c50d3b789e1b1ccc05e5 > -- > 2.35.1 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] c++: ICE with temporary of class type in DMI [PR100252] 2022-04-28 14:12 ` Patrick Palka @ 2022-04-28 14:39 ` Marek Polacek 0 siblings, 0 replies; 17+ messages in thread From: Marek Polacek @ 2022-04-28 14:39 UTC (permalink / raw) To: Patrick Palka; +Cc: GCC Patches, Jason Merrill, Jakub Jelinek On Thu, Apr 28, 2022 at 10:12:04AM -0400, Patrick Palka wrote: > On Wed, 27 Apr 2022, Marek Polacek wrote: > > > On Wed, Apr 27, 2022 at 11:00:46AM -0400, Patrick Palka wrote: > > > On Tue, 26 Apr 2022, Marek Polacek wrote: > > > > > > > Consider > > > > > > > > struct A { > > > > int x; > > > > int y = x; > > > > }; > > > > > > > > struct B { > > > > int x = 0; > > > > int y = A{x}.y; // #1 > > > > }; > > > > > > > > where for #1 we end up with > > > > > > > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > > > > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > > > > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > > > > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > > > > > > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > > > > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > > > > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > > > > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > > > > have any object to refer to yet. After digesting, we have > > > > > > > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > > > > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > > > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > > > > > > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > > > > > > > Then we get to > > > > > > > > B b = {}; > > > > > > > > and call store_init_value, which digest the {}, which produces > > > > > > > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > > > > > > > The call to replace_placeholders in store_init_value will not do anything: > > > > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > > > > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > > > > stays even though now is the perfect time to replace it because we have an > > > > object for it: 'b'. > > > > > > > > Later, in cp_gimplify_init_expr the *expr_p is > > > > > > > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > > > > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > > > > has a different type. > > > > > > > > My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the > > > > TARGET_EXPR because that means we have an object we can refer to. Then clear > > > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR > > > > in the {}. Then store_init_value will be able to replace <P_E struct B> with > > > > 'b', and we should be good to go. > > > > > > Makes sense to me. It seems all was well until break_out_target_exprs, > > > called from get_nsdmi for B::y, replaced the 'this' in the initializer > > > > > > (TARGET_EXPR <D.2131, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; > > > > > > with a PLACEHOLDER_EXPR; > > > > > > (TARGET_EXPR <D.2142, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; > > > > > > This seems to be the wrong thing to do when the 'this' appears inside a > > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructor because the new > > > PLACEHOLDER_EXPR then can't be resolved correctly. > > > > Exactly. > > > > > So in light of this I wonder if we should instead perform this handling > > > you added to finish_compound_literal in break_out_target_exprs / > > > bot_manip instead? > > > > Unfortunately that causes an ICE in gimplify_var_or_parm_decl on the new > > testcase I've added here. bot_manip is a different context and so I can't > > use parsing_nsdmi anymore, and it seems we'd replace the placeholders too > > aggressively in bot_manip. So I'm not sure if that's the best place. > > Ah :/ good catch... > > FWIW the transformation itself (doing replace_placeholders followed by > clearing CONSTRUCTOR_PLACEHOLDER_BOUNDARY) makes sense to me, and I'm > happy with doing it in finish_compound_literal when parsing_nsdmi, so > the patch LGTM. Thanks for taking a look. > (I guess we could also consider doing the transformation in get_nsdmi or > digest_nsdmi_init via cp_walk_tree, but I don't have a preference either way..) I actually considered doing it in get_nsdmi, but it's either too early (don't have a TARGET_EXPR yet, therefore no object to refer to), or too late -- we're just about to introduce <PLACEHOLDER_EXPR struct B>. I felt like it may be best to replace the <PLACEHOLDER_EXPR struct A> at the earliest opportunity, which if f_c_l. Marek ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] c++: ICE with temporary of class type in DMI [PR100252] 2022-04-27 20:30 ` [PATCH v2] " Marek Polacek 2022-04-28 14:12 ` Patrick Palka @ 2022-05-03 20:59 ` Jason Merrill 2022-05-07 19:11 ` [PATCH v3] " Marek Polacek 1 sibling, 1 reply; 17+ messages in thread From: Jason Merrill @ 2022-05-03 20:59 UTC (permalink / raw) To: Marek Polacek, Patrick Palka; +Cc: GCC Patches, Jakub Jelinek On 4/27/22 16:30, Marek Polacek wrote: > On Wed, Apr 27, 2022 at 11:00:46AM -0400, Patrick Palka wrote: >> On Tue, 26 Apr 2022, Marek Polacek wrote: >> >>> Consider >>> >>> struct A { >>> int x; >>> int y = x; >>> }; >>> >>> struct B { >>> int x = 0; >>> int y = A{x}.y; // #1 >>> }; >>> >>> where for #1 we end up with >>> >>> {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} >>> >>> that is, two PLACEHOLDER_EXPRs for different types on the same level in >>> a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to >>> avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. >>> >>> Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing >>> cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal >>> on type=A, compound_literal={((struct B *) this)->x}. When digesting this >>> initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't >>> have any object to refer to yet. After digesting, we have >>> >>> {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} >>> >>> and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor >>> CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns >>> >>> TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> >>> >>> Then we get to >>> >>> B b = {}; >>> >>> and call store_init_value, which digest the {}, which produces >>> >>> {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} >>> >>> The call to replace_placeholders in store_init_value will not do anything: >>> we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only >>> a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> >>> stays even though now is the perfect time to replace it because we have an >>> object for it: 'b'. >>> >>> Later, in cp_gimplify_init_expr the *expr_p is >>> >>> D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} >>> >>> where D.2395 is of type A, but we crash because we hit <P_E struct B>, which >>> has a different type. >>> >>> My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the >>> TARGET_EXPR because that means we have an object we can refer to. Then clear >>> CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR >>> in the {}. Then store_init_value will be able to replace <P_E struct B> with >>> 'b', and we should be good to go. >> >> Makes sense to me. It seems all was well until break_out_target_exprs, >> called from get_nsdmi for B::y, replaced the 'this' in the initializer >> >> (TARGET_EXPR <D.2131, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; >> >> with a PLACEHOLDER_EXPR; >> >> (TARGET_EXPR <D.2142, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y; >> >> This seems to be the wrong thing to do when the 'this' appears inside a >> CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructor because the new >> PLACEHOLDER_EXPR then can't be resolved correctly. > > Exactly. > >> So in light of this I wonder if we should instead perform this handling >> you added to finish_compound_literal in break_out_target_exprs / >> bot_manip instead? > > Unfortunately that causes an ICE in gimplify_var_or_parm_decl on the new > testcase I've added here. bot_manip is a different context and so I can't > use parsing_nsdmi anymore, and it seems we'd replace the placeholders too > aggressively in bot_manip. So I'm not sure if that's the best place. > > -- >8 -- > Consider > > struct A { > int x; > int y = x; > }; > > struct B { > int x = 0; > int y = A{x}.y; // #1 > }; > > where for #1 we end up with > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > have any object to refer to yet. After digesting, we have > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > Then we get to > > B b = {}; > > and call store_init_value, which digest the {}, which produces > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > The call to replace_placeholders in store_init_value will not do anything: > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > stays even though now is the perfect time to replace it because we have an > object for it: 'b'. > > Later, in cp_gimplify_init_expr the *expr_p is > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > has a different type. > > My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the > TARGET_EXPR because that means we have an object we can refer to. Then clear > CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR > in the {}. Then store_init_value will be able to replace <P_E struct B> with > 'b', and we should be good to go. > > PR c++/100252 > > gcc/cp/ChangeLog: > > * semantics.cc (finish_compound_literal): replace_placeholders after > creating the TARGET_EXPR. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/nsdmi-aggr14.C: New test. > * g++.dg/cpp1y/nsdmi-aggr15.C: New test. > --- > gcc/cp/semantics.cc | 31 +++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++++ > 3 files changed, 106 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > index ab48f11c9be..770369458bb 100644 > --- a/gcc/cp/semantics.cc > +++ b/gcc/cp/semantics.cc > @@ -3296,6 +3296,37 @@ finish_compound_literal (tree type, tree compound_literal, > if (TREE_CODE (compound_literal) == CONSTRUCTOR) > TREE_HAS_CONSTRUCTOR (compound_literal) = false; > compound_literal = get_target_expr_sfinae (compound_literal, complain); > + /* We may have A{} in a NSDMI. */ > + if (parsing_nsdmi ()) > + { > + /* Digesting the {} could have introduced a PLACEHOLDER_EXPR > + referring to A. Now that we've built up a TARGET_EXPR, we > + have an object we can refer to. The reason we bother doing > + this here is for code like > + > + struct A { > + int x; > + int y = x; > + }; > + > + struct B { > + int x = 0; > + int y = A{x}.y; // #1 > + }; > + > + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs > + for different types on the same level in a {} as in 100252. */ > + tree init = TARGET_EXPR_INITIAL (compound_literal); > + if (TREE_CODE (init) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) > + { > + tree obj = TARGET_EXPR_SLOT (compound_literal); > + replace_placeholders (compound_literal, obj); > + /* We should have dealt with the PLACEHOLDER_EXPRs. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; > + gcc_checking_assert (!find_placeholders (init)); Does this testcase still work with this patch? struct A { const A* p = this; }; struct B { A a = A{}; }; constexpr B b; static_assert (b.a.p == &b.a); > + } > + } > } > else > /* For e.g. int{42} just make sure it's a prvalue. */ > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > new file mode 100644 > index 00000000000..7d508f52b48 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > @@ -0,0 +1,46 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +constexpr B csb1 = { }; > +SA(csb1.x == 0 && csb1.y == csb1.x); > +constexpr B csb2 = { 1 }; > +SA(csb2.x == 1 && csb2.y == csb2.x); > +constexpr B csb3 = { 1, 2 }; > +SA(csb3.x == 1 && csb3.y == 2); > + > +B sb1 = { }; > +B sb2 = { 1 }; > +B sb3 = { 1, 2}; > + > +int > +main () > +{ > + if (sb1.x != 0 || sb1.x != sb1.y) > + __builtin_abort(); > + if (sb2.x != 1 || sb2.x != sb2.y) > + __builtin_abort(); > + if (sb3.x != 1 || sb3.y != 2) > + __builtin_abort(); > + > + B b1 = { }; > + B b2 = { 1 }; > + B b3 = { 1, 2}; > + if (b1.x != 0 || b1.x != b1.y) > + __builtin_abort(); > + if (b2.x != 1 || b2.x != b2.y) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > new file mode 100644 > index 00000000000..bc997bb5e1d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > @@ -0,0 +1,29 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +void > +g (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) > +{ > + if (b1.x != 0 || b1.y != b1.x) > + __builtin_abort(); > + if (b2.x != 1 || b2.y != b2.x) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > + > +int > +main () > +{ > + g (); > +} > > base-commit: 409edcca331296b53842c50d3b789e1b1ccc05e5 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-03 20:59 ` Jason Merrill @ 2022-05-07 19:11 ` Marek Polacek 2022-05-07 22:02 ` Jason Merrill 0 siblings, 1 reply; 17+ messages in thread From: Marek Polacek @ 2022-05-07 19:11 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On Tue, May 03, 2022 at 04:59:38PM -0400, Jason Merrill wrote: > Does this testcase still work with this patch? > > struct A { > const A* p = this; > }; > > struct B { > A a = A{}; > }; > > constexpr B b; > static_assert (b.a.p == &b.a); Ouch, no. Thanks for catching this, it would have been very unpleasant to hit this problem later... The reason your testcase was rejected is that replacing a PLACEHOLDER in the outermost TARGET_EXPR broke guaranteed copy elision. Now that I've spent another day on this, I still think we have to replace the placeholders created when temporary materialization takes place in an NSDMI, but it must happen in digest_nsdmi_init (as Patrick suggested), where we can see the full initializer. That way I can avoid breaking copy elision. I've added a bunch of new testcases and updated the (already long) description. The yuge comment hopefully also explains how this problem is avoided. Thanks again, Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Consider struct A { int x; int y = x; }; struct B { int x = 0; int y = A{x}.y; // #1 }; where for #1 we end up with {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} that is, two PLACEHOLDER_EXPRs for different types on the same level in a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal on type=A, compound_literal={((struct B *) this)->x}. When digesting this initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't have any object to refer to yet. After digesting, we have {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> Then we get to B b = {}; and call store_init_value, which digests the {}, which produces {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} lookup_placeholder in constexpr won't find an object to replace the PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we cannot search outward from D.2395 to find 'b'. The call to replace_placeholders in store_init_value will not do anything: we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> stays even though now is the perfect time to replace it because we have an object for it: 'b'. Later, in cp_gimplify_init_expr the *expr_p is D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} where D.2395 is of type A, but we crash because we hit <P_E struct B>, which has a different type. My idea was to replace <P_E struct A> with D.2384 after creating the TARGET_EXPR because that means we have an object we can refer to. Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to replace <P_E struct B> with 'b', and we should be good to go. We must be careful not to break guaranteed copy elision, so this replacement happens in digest_nsdmi_init where we can see the whole initializer, and avoid replacing any placeholders in the outermost TARGET_EXPR. PR c++/100252 gcc/cp/ChangeLog: * typeck2.cc (replace_placeholders_for_class_temp_r): New. (digest_nsdmi_init): Call it. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/nsdmi-aggr14.C: New test. * g++.dg/cpp1y/nsdmi-aggr15.C: New test. * g++.dg/cpp1y/nsdmi-aggr16.C: New test. * g++.dg/cpp1y/nsdmi-aggr17.C: New test. * g++.dg/cpp1y/nsdmi-aggr18.C: New test. --- gcc/cp/typeck2.cc | 54 +++++++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 19 ++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 47 ++++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 25 +++++++++++ 6 files changed, 220 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc index 63d95c1529a..72d7cfaf1d3 100644 --- a/gcc/cp/typeck2.cc +++ b/gcc/cp/typeck2.cc @@ -1371,6 +1371,34 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) return digest_init_r (type, init, 0, flags, complain); } +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used + in the context of guaranteed copy elision). */ + +static tree +replace_placeholders_for_class_temp_r (tree *tp, int *walk_subtrees, void *data) +{ + tree t = *tp; + tree full_expr = *static_cast<tree *>(data); + + /* We're looking for a TARGET_EXPR nested in the whole expression. */ + if (TREE_CODE (t) == TARGET_EXPR && t != full_expr) + { + tree init = TARGET_EXPR_INITIAL (t); + if (TREE_CODE (init) == CONSTRUCTOR + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) + { + tree obj = TARGET_EXPR_SLOT (t); + replace_placeholders (t, obj); + /* We should have dealt with all PLACEHOLDER_EXPRs. */ + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; + gcc_checking_assert (!find_placeholders (init)); + } + *walk_subtrees = false; + } + + return NULL_TREE; +} + /* Process the initializer INIT for an NSDMI DECL (a FIELD_DECL). */ tree digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) @@ -1390,6 +1418,32 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) && CP_AGGREGATE_TYPE_P (type)) init = reshape_init (type, init, complain); init = digest_init_flags (type, init, flags, complain); + + /* We may have temporary materialization in a NSDMI, if the initializer + has something like A{} in it. Digesting the {} could have introduced + a PLACEHOLDER_EXPR referring to A. Now that we've got a TARGET_EXPR, + we have an object we can refer to. The reason we bother doing this + here is for code like + + struct A { + int x; + int y = x; + }; + + struct B { + int x = 0; + int y = A{x}.y; // #1 + }; + + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs for + different types on the same level in a {} when lookup_placeholder + wouldn't find a named object for the PLACEHOLDER_EXPR for A. Note, + temporary materialization does not occur when initializing an object + from a prvalue of the same type, therefore we must not replace the + placeholder with a temporary object, so that it can be elided. */ + cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init, + nullptr); + return init; } \f diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C new file mode 100644 index 00000000000..7d508f52b48 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C @@ -0,0 +1,46 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +constexpr B csb1 = { }; +SA(csb1.x == 0 && csb1.y == csb1.x); +constexpr B csb2 = { 1 }; +SA(csb2.x == 1 && csb2.y == csb2.x); +constexpr B csb3 = { 1, 2 }; +SA(csb3.x == 1 && csb3.y == 2); + +B sb1 = { }; +B sb2 = { 1 }; +B sb3 = { 1, 2}; + +int +main () +{ + if (sb1.x != 0 || sb1.x != sb1.y) + __builtin_abort(); + if (sb2.x != 1 || sb2.x != sb2.y) + __builtin_abort(); + if (sb3.x != 1 || sb3.y != 2) + __builtin_abort(); + + B b1 = { }; + B b2 = { 1 }; + B b3 = { 1, 2}; + if (b1.x != 0 || b1.x != b1.y) + __builtin_abort(); + if (b2.x != 1 || b2.x != b2.y) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C new file mode 100644 index 00000000000..bc997bb5e1d --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C @@ -0,0 +1,29 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +void +g (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) +{ + if (b1.x != 0 || b1.y != b1.x) + __builtin_abort(); + if (b2.x != 1 || b2.y != b2.x) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); +} + +int +main () +{ + g (); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C new file mode 100644 index 00000000000..a4d7428c1e7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C @@ -0,0 +1,19 @@ +// PR c++/100252 +// { dg-do compile { target c++14 } } + +struct A { + const A* p = this; +}; + +struct B { + A a = A{}; +}; + +constexpr B b; +static_assert (b.a.p == &b.a, ""); +B b1 = { }; + +void +g (B b2 = B{}) +{ +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C new file mode 100644 index 00000000000..c3bfcfa572d --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C @@ -0,0 +1,47 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +struct A { + int x; + int y = x; + const A* p = this; +}; + +struct B { + int x = 42; + A a = A{x}; +}; + +constexpr B b; +static_assert (b.a.p == &b.a, ""); +static_assert (b.x == 42, ""); +B b2 = { }; +B b3 = { 42 }; + +struct C { + int x = 42; + B b = B{x}; +}; + +constexpr C c; +C c2; +C c3; + +void +g (B b4 = B{}, B b5 = B{ 42 }, C c4 = C{}, C c5 = C{ 42 }) +{ + if (b2.x != 42 || b2.a.x != 42 || b2.a.y != b2.a.x) + __builtin_abort (); + if (b3.x != 42 || b3.a.x != 42 || b3.a.y != b3.a.x) + __builtin_abort (); + if (b4.x != 42 || b4.a.x != 42 || b4.a.y != b4.a.x) + __builtin_abort (); + if (b5.x != 42 || b5.a.x != 42 || b5.a.y != b5.a.x) + __builtin_abort (); +} + +int +main () +{ + g (); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C new file mode 100644 index 00000000000..65e2275353b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C @@ -0,0 +1,25 @@ +// PR c++/100252 +// { dg-do compile { target c++14 } } + +struct B { }; + +struct A { + int x; + int y = x; + constexpr operator B() { return B{}; } +}; + +struct C { + int x = 42; + B b = A{x}; +}; + +C c1 = {}; +C c2 = { 42 }; +constexpr C c3 = {}; +constexpr C c4 = { 42 }; + +void +g (C c5 = C{}, C c6 = C{ 42 }) +{ +} base-commit: 0c723bb4be2a67657828b692997855afcdc5d286 -- 2.35.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-07 19:11 ` [PATCH v3] " Marek Polacek @ 2022-05-07 22:02 ` Jason Merrill 2022-05-13 23:41 ` [PATCH v4] " Marek Polacek 0 siblings, 1 reply; 17+ messages in thread From: Jason Merrill @ 2022-05-07 22:02 UTC (permalink / raw) To: Marek Polacek; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On 5/7/22 15:11, Marek Polacek wrote: > On Tue, May 03, 2022 at 04:59:38PM -0400, Jason Merrill wrote: >> Does this testcase still work with this patch? >> >> struct A { >> const A* p = this; >> }; >> >> struct B { >> A a = A{}; >> }; >> >> constexpr B b; >> static_assert (b.a.p == &b.a); > > Ouch, no. Thanks for catching this, it would have been very unpleasant > to hit this problem later... > > The reason your testcase was rejected is that replacing a PLACEHOLDER > in the outermost TARGET_EXPR broke guaranteed copy elision. Now that > I've spent another day on this, I still think we have to replace the > placeholders created when temporary materialization takes place in an > NSDMI, but it must happen in digest_nsdmi_init (as Patrick suggested), > where we can see the full initializer. That way I can avoid breaking > copy elision. > > I've added a bunch of new testcases and updated the > (already long) description. The yuge comment hopefully also explains > how this problem is avoided. Thanks again, > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > Consider > > struct A { > int x; > int y = x; > }; > > struct B { > int x = 0; > int y = A{x}.y; // #1 > }; > > where for #1 we end up with > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > have any object to refer to yet. After digesting, we have > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > Then we get to > > B b = {}; > > and call store_init_value, which digests the {}, which produces > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > lookup_placeholder in constexpr won't find an object to replace the > PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we > cannot search outward from D.2395 to find 'b'. > > The call to replace_placeholders in store_init_value will not do anything: > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > stays even though now is the perfect time to replace it because we have an > object for it: 'b'. > > Later, in cp_gimplify_init_expr the *expr_p is > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > has a different type. > > My idea was to replace <P_E struct A> with D.2384 after creating the > TARGET_EXPR because that means we have an object we can refer to. > Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have > a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to > replace <P_E struct B> with 'b', and we should be good to go. We must > be careful not to break guaranteed copy elision, so this replacement > happens in digest_nsdmi_init where we can see the whole initializer, > and avoid replacing any placeholders in the outermost TARGET_EXPR. > > PR c++/100252 > > gcc/cp/ChangeLog: > > * typeck2.cc (replace_placeholders_for_class_temp_r): New. > (digest_nsdmi_init): Call it. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/nsdmi-aggr14.C: New test. > * g++.dg/cpp1y/nsdmi-aggr15.C: New test. > * g++.dg/cpp1y/nsdmi-aggr16.C: New test. > * g++.dg/cpp1y/nsdmi-aggr17.C: New test. > * g++.dg/cpp1y/nsdmi-aggr18.C: New test. > --- > gcc/cp/typeck2.cc | 54 +++++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 19 ++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 47 ++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 25 +++++++++++ > 6 files changed, 220 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > index 63d95c1529a..72d7cfaf1d3 100644 > --- a/gcc/cp/typeck2.cc > +++ b/gcc/cp/typeck2.cc > @@ -1371,6 +1371,34 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) > return digest_init_r (type, init, 0, flags, complain); > } > > +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used > + in the context of guaranteed copy elision). */ > + > +static tree > +replace_placeholders_for_class_temp_r (tree *tp, int *walk_subtrees, void *data) > +{ > + tree t = *tp; > + tree full_expr = *static_cast<tree *>(data); > + > + /* We're looking for a TARGET_EXPR nested in the whole expression. */ > + if (TREE_CODE (t) == TARGET_EXPR && t != full_expr) Just comparing to full_expr doesn't seem enough to catch all the cases where a prvalue is used as an initializer rather than for temporary materialization; you will also need to handle comma and conditional expressions. And probably PAREN_EXPR. I'm failing to find a term for this in the standard; "potential result" also includes objects that the result is an xvalue subobject of (as in #1 below). Maybe "potential prvalue result", to exclude the xvalue cases? So, && !potential_prvalue_result_of (t, full_expr) ? > + { > + tree init = TARGET_EXPR_INITIAL (t); > + if (TREE_CODE (init) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) > + { > + tree obj = TARGET_EXPR_SLOT (t); > + replace_placeholders (t, obj); > + /* We should have dealt with all PLACEHOLDER_EXPRs. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; > + gcc_checking_assert (!find_placeholders (init)); > + } > + *walk_subtrees = false; > + } > + > + return NULL_TREE; > +} > + > /* Process the initializer INIT for an NSDMI DECL (a FIELD_DECL). */ > tree > digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) > @@ -1390,6 +1418,32 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) > && CP_AGGREGATE_TYPE_P (type)) > init = reshape_init (type, init, complain); > init = digest_init_flags (type, init, flags, complain); > + > + /* We may have temporary materialization in a NSDMI, if the initializer > + has something like A{} in it. Digesting the {} could have introduced > + a PLACEHOLDER_EXPR referring to A. Now that we've got a TARGET_EXPR, > + we have an object we can refer to. The reason we bother doing this > + here is for code like > + > + struct A { > + int x; > + int y = x; > + }; > + > + struct B { > + int x = 0; > + int y = A{x}.y; // #1 > + }; > + > + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs for > + different types on the same level in a {} when lookup_placeholder > + wouldn't find a named object for the PLACEHOLDER_EXPR for A. Note, > + temporary materialization does not occur when initializing an object > + from a prvalue of the same type, therefore we must not replace the > + placeholder with a temporary object, so that it can be elided. */ > + cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init, > + nullptr); > + > return init; > } > \f > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > new file mode 100644 > index 00000000000..7d508f52b48 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > @@ -0,0 +1,46 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +constexpr B csb1 = { }; > +SA(csb1.x == 0 && csb1.y == csb1.x); > +constexpr B csb2 = { 1 }; > +SA(csb2.x == 1 && csb2.y == csb2.x); > +constexpr B csb3 = { 1, 2 }; > +SA(csb3.x == 1 && csb3.y == 2); > + > +B sb1 = { }; > +B sb2 = { 1 }; > +B sb3 = { 1, 2}; > + > +int > +main () > +{ > + if (sb1.x != 0 || sb1.x != sb1.y) > + __builtin_abort(); > + if (sb2.x != 1 || sb2.x != sb2.y) > + __builtin_abort(); > + if (sb3.x != 1 || sb3.y != 2) > + __builtin_abort(); > + > + B b1 = { }; > + B b2 = { 1 }; > + B b3 = { 1, 2}; > + if (b1.x != 0 || b1.x != b1.y) > + __builtin_abort(); > + if (b2.x != 1 || b2.x != b2.y) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > new file mode 100644 > index 00000000000..bc997bb5e1d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > @@ -0,0 +1,29 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +void > +g (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) > +{ > + if (b1.x != 0 || b1.y != b1.x) > + __builtin_abort(); > + if (b2.x != 1 || b2.y != b2.x) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > + > +int > +main () > +{ > + g (); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > new file mode 100644 > index 00000000000..a4d7428c1e7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > @@ -0,0 +1,19 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +struct A { > + const A* p = this; > +}; > + > +struct B { > + A a = A{}; > +}; > + > +constexpr B b; > +static_assert (b.a.p == &b.a, ""); > +B b1 = { }; > + > +void > +g (B b2 = B{}) > +{ > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > new file mode 100644 > index 00000000000..c3bfcfa572d > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > @@ -0,0 +1,47 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +struct A { > + int x; > + int y = x; > + const A* p = this; > +}; > + > +struct B { > + int x = 42; > + A a = A{x}; > +}; > + > +constexpr B b; > +static_assert (b.a.p == &b.a, ""); > +static_assert (b.x == 42, ""); > +B b2 = { }; > +B b3 = { 42 }; > + > +struct C { > + int x = 42; > + B b = B{x}; > +}; > + > +constexpr C c; > +C c2; > +C c3; > + > +void > +g (B b4 = B{}, B b5 = B{ 42 }, C c4 = C{}, C c5 = C{ 42 }) > +{ > + if (b2.x != 42 || b2.a.x != 42 || b2.a.y != b2.a.x) > + __builtin_abort (); > + if (b3.x != 42 || b3.a.x != 42 || b3.a.y != b3.a.x) > + __builtin_abort (); > + if (b4.x != 42 || b4.a.x != 42 || b4.a.y != b4.a.x) > + __builtin_abort (); > + if (b5.x != 42 || b5.a.x != 42 || b5.a.y != b5.a.x) > + __builtin_abort (); > +} > + > +int > +main () > +{ > + g (); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > new file mode 100644 > index 00000000000..65e2275353b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > @@ -0,0 +1,25 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +struct B { }; > + > +struct A { > + int x; > + int y = x; > + constexpr operator B() { return B{}; } > +}; > + > +struct C { > + int x = 42; > + B b = A{x}; > +}; > + > +C c1 = {}; > +C c2 = { 42 }; > +constexpr C c3 = {}; > +constexpr C c4 = { 42 }; > + > +void > +g (C c5 = C{}, C c6 = C{ 42 }) > +{ > +} > > base-commit: 0c723bb4be2a67657828b692997855afcdc5d286 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-07 22:02 ` Jason Merrill @ 2022-05-13 23:41 ` Marek Polacek 2022-05-15 3:13 ` Jason Merrill 0 siblings, 1 reply; 17+ messages in thread From: Marek Polacek @ 2022-05-13 23:41 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On Sat, May 07, 2022 at 06:02:13PM -0400, Jason Merrill wrote: > On 5/7/22 15:11, Marek Polacek wrote: > > On Tue, May 03, 2022 at 04:59:38PM -0400, Jason Merrill wrote: > > > Does this testcase still work with this patch? > > > > > > struct A { > > > const A* p = this; > > > }; > > > > > > struct B { > > > A a = A{}; > > > }; > > > > > > constexpr B b; > > > static_assert (b.a.p == &b.a); > > > > Ouch, no. Thanks for catching this, it would have been very unpleasant > > to hit this problem later... > > > > The reason your testcase was rejected is that replacing a PLACEHOLDER > > in the outermost TARGET_EXPR broke guaranteed copy elision. Now that > > I've spent another day on this, I still think we have to replace the > > placeholders created when temporary materialization takes place in an > > NSDMI, but it must happen in digest_nsdmi_init (as Patrick suggested), > > where we can see the full initializer. That way I can avoid breaking > > copy elision. > > > > I've added a bunch of new testcases and updated the > > (already long) description. The yuge comment hopefully also explains > > how this problem is avoided. Thanks again, > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > -- >8 -- > > Consider > > > > struct A { > > int x; > > int y = x; > > }; > > > > struct B { > > int x = 0; > > int y = A{x}.y; // #1 > > }; > > > > where for #1 we end up with > > > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > > have any object to refer to yet. After digesting, we have > > > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > > > Then we get to > > > > B b = {}; > > > > and call store_init_value, which digests the {}, which produces > > > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > > > lookup_placeholder in constexpr won't find an object to replace the > > PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we > > cannot search outward from D.2395 to find 'b'. > > > > The call to replace_placeholders in store_init_value will not do anything: > > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > > stays even though now is the perfect time to replace it because we have an > > object for it: 'b'. > > > > Later, in cp_gimplify_init_expr the *expr_p is > > > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > > has a different type. > > > > My idea was to replace <P_E struct A> with D.2384 after creating the > > TARGET_EXPR because that means we have an object we can refer to. > > Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have > > a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to > > replace <P_E struct B> with 'b', and we should be good to go. We must > > be careful not to break guaranteed copy elision, so this replacement > > happens in digest_nsdmi_init where we can see the whole initializer, > > and avoid replacing any placeholders in the outermost TARGET_EXPR. > > > > PR c++/100252 > > > > gcc/cp/ChangeLog: > > > > * typeck2.cc (replace_placeholders_for_class_temp_r): New. > > (digest_nsdmi_init): Call it. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp1y/nsdmi-aggr14.C: New test. > > * g++.dg/cpp1y/nsdmi-aggr15.C: New test. > > * g++.dg/cpp1y/nsdmi-aggr16.C: New test. > > * g++.dg/cpp1y/nsdmi-aggr17.C: New test. > > * g++.dg/cpp1y/nsdmi-aggr18.C: New test. > > --- > > gcc/cp/typeck2.cc | 54 +++++++++++++++++++++++ > > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++ > > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++ > > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 19 ++++++++ > > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 47 ++++++++++++++++++++ > > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 25 +++++++++++ > > 6 files changed, 220 insertions(+) > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > > > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > > index 63d95c1529a..72d7cfaf1d3 100644 > > --- a/gcc/cp/typeck2.cc > > +++ b/gcc/cp/typeck2.cc > > @@ -1371,6 +1371,34 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) > > return digest_init_r (type, init, 0, flags, complain); > > } > > +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used > > + in the context of guaranteed copy elision). */ > > + > > +static tree > > +replace_placeholders_for_class_temp_r (tree *tp, int *walk_subtrees, void *data) > > +{ > > + tree t = *tp; > > + tree full_expr = *static_cast<tree *>(data); > > + > > + /* We're looking for a TARGET_EXPR nested in the whole expression. */ > > + if (TREE_CODE (t) == TARGET_EXPR && t != full_expr) > > Just comparing to full_expr doesn't seem enough to catch all the cases where > a prvalue is used as an initializer rather than for temporary > materialization; you will also need to handle comma and conditional > expressions. And probably PAREN_EXPR. > > I'm failing to find a term for this in the standard; "potential result" also > includes objects that the result is an xvalue subobject of (as in #1 below). > Maybe "potential prvalue result", to exclude the xvalue cases? So, > > && !potential_prvalue_result_of (t, full_expr) > > ? Thanks, the previous version indeed did have problems when ?: or , was used. I've greatly enhanced the tests to detect more cases, and added potential_prvalue_result_of. While looking into this, I found PR105550, which turned out to be a big can of worms. So, XFAIL for now. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Consider struct A { int x; int y = x; }; struct B { int x = 0; int y = A{x}.y; // #1 }; where for #1 we end up with {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} that is, two PLACEHOLDER_EXPRs for different types on the same level in a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal on type=A, compound_literal={((struct B *) this)->x}. When digesting this initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't have any object to refer to yet. After digesting, we have {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> Then we get to B b = {}; and call store_init_value, which digests the {}, which produces {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} lookup_placeholder in constexpr won't find an object to replace the PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we cannot search outward from D.2395 to find 'b'. The call to replace_placeholders in store_init_value will not do anything: we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> stays even though now is the perfect time to replace it because we have an object for it: 'b'. Later, in cp_gimplify_init_expr the *expr_p is D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} where D.2395 is of type A, but we crash because we hit <P_E struct B>, which has a different type. My idea was to replace <P_E struct A> with D.2384 after creating the TARGET_EXPR because that means we have an object we can refer to. Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to replace <P_E struct B> with 'b', and we should be good to go. We must be careful not to break guaranteed copy elision, so this replacement happens in digest_nsdmi_init where we can see the whole initializer, and avoid replacing any placeholders in TARGET_EXPRs used in the context of initialization/copy elision. This is achieved via the new function called potential_prvalue_result_of. While fixing this problem, I found PR105550, thus the FIXMEs in the tests. PR c++/100252 gcc/cp/ChangeLog: * typeck2.cc (potential_prvalue_result_of): New. (replace_placeholders_for_class_temp_r): New. (digest_nsdmi_init): Call it. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/nsdmi-aggr14.C: New test. * g++.dg/cpp1y/nsdmi-aggr15.C: New test. * g++.dg/cpp1y/nsdmi-aggr16.C: New test. * g++.dg/cpp1y/nsdmi-aggr17.C: New test. * g++.dg/cpp1y/nsdmi-aggr18.C: New test. * g++.dg/cpp1y/nsdmi-aggr19.C: New test. --- gcc/cp/typeck2.cc | 90 ++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 131 ++++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 80 +++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 58 +++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 138 ++++++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 56 +++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C | 28 +++++ 7 files changed, 581 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc index 1d92310edd0..f60e1df4c27 100644 --- a/gcc/cp/typeck2.cc +++ b/gcc/cp/typeck2.cc @@ -1371,6 +1371,70 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) return digest_init_r (type, init, 0, flags, complain); } +/* Return true if a prvalue is used as an initializer rather than for + temporary materialization. For instance: + + A a = A{}; // initializer + A a = (A{}); // initializer + A a = (1, A{}); // initializer + A a = true ? A{} : A{}; // initializer + auto x = A{}.x; // temporary materialization + auto x = foo(A{}); // temporary materialization + + FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ + +static bool +potential_prvalue_result_of (tree subob, tree full_expr) +{ + if (subob == full_expr) + return true; + else if (TREE_CODE (full_expr) == TARGET_EXPR) + { + tree init = TARGET_EXPR_INITIAL (full_expr); + if (TREE_CODE (init) == COND_EXPR) + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)) + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 2))); + else if (TREE_CODE (init) == COMPOUND_EXPR) + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 0)) + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 1))); + /* ??? I don't know if this can be hit. If so, look inside the ( ) + instead of the assert. */ + else if (TREE_CODE (init) == PAREN_EXPR) + gcc_checking_assert (false); + } + return false; +} + +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used + in the context of guaranteed copy elision). */ + +static tree +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) +{ + tree t = *tp; + tree full_expr = *static_cast<tree *>(data); + + /* We're looking for a TARGET_EXPR nested in the whole expression. */ + if (TREE_CODE (t) == TARGET_EXPR + && !potential_prvalue_result_of (t, full_expr)) + { + tree init = TARGET_EXPR_INITIAL (t); + while (TREE_CODE (init) == COMPOUND_EXPR) + init = TREE_OPERAND (init, 1); + if (TREE_CODE (init) == CONSTRUCTOR + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) + { + tree obj = TARGET_EXPR_SLOT (t); + replace_placeholders (init, obj); + /* We should have dealt with all PLACEHOLDER_EXPRs. */ + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; + gcc_checking_assert (!find_placeholders (init)); + } + } + + return NULL_TREE; +} + /* Process the initializer INIT for an NSDMI DECL (a FIELD_DECL). */ tree digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) @@ -1390,6 +1454,32 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) && CP_AGGREGATE_TYPE_P (type)) init = reshape_init (type, init, complain); init = digest_init_flags (type, init, flags, complain); + + /* We may have temporary materialization in a NSDMI, if the initializer + has something like A{} in it. Digesting the {} could have introduced + a PLACEHOLDER_EXPR referring to A. Now that we've got a TARGET_EXPR, + we have an object we can refer to. The reason we bother doing this + here is for code like + + struct A { + int x; + int y = x; + }; + + struct B { + int x = 0; + int y = A{x}.y; // #1 + }; + + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs for + different types on the same level in a {} when lookup_placeholder + wouldn't find a named object for the PLACEHOLDER_EXPR for A. Note, + temporary materialization does not occur when initializing an object + from a prvalue of the same type, therefore we must not replace the + placeholder with a temporary object so that it can be elided. */ + cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init, + nullptr); + return init; } \f diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C new file mode 100644 index 00000000000..28b908a0a1a --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C @@ -0,0 +1,131 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +constexpr B csb1 = { }; +SA(csb1.x == 0 && csb1.y == csb1.x); +constexpr B csb2 = { 1 }; +SA(csb2.x == 1 && csb2.y == csb2.x); +constexpr B csb3 = { 1, 2 }; +SA(csb3.x == 1 && csb3.y == 2); + +B sb1 = { }; +B sb2 = { 1 }; +B sb3 = { 1, 2}; + +struct C { + int x = 0; + int y = (true, A{x}.y) + (A{x}.y, 0); +}; + +constexpr C csc1 = { }; +SA(csc1.x == 0 && csc1.y == csc1.x); +constexpr C csc2 = { 1 }; +SA(csc2.x == 1 && csc2.y == csc2.x); +constexpr C csc3 = { 1, 2 }; +SA(csc3.x == 1 && csc3.y == 2); + +C sc1 = { }; +C sc2 = { 1 }; +C sc3 = { 1, 2}; + +struct D { + int x = 0; + int y = (A{x}.y); +}; + +constexpr D csd1 = { }; +SA(csd1.x == 0 && csd1.y == csd1.x); +constexpr D csd2 = { 1 }; +SA(csd2.x == 1 && csd2.y == csd2.x); +constexpr D csd3 = { 1, 2 }; +SA(csd3.x == 1 && csd3.y == 2); + +D sd1 = { }; +D sd2 = { 1 }; +D sd3 = { 1, 2}; + +struct E { + int x = 0; + int y = x ? A{x}.y : A{x}.y; +}; + +constexpr E cse1 = { }; +SA(cse1.x == 0 && cse1.y == cse1.x); +constexpr E cse2 = { 1 }; +SA(cse2.x == 1 && cse2.y == cse2.x); +constexpr E cse3 = { 1, 2 }; +SA(cse3.x == 1 && cse3.y == 2); + +E se1 = { }; +E se2 = { 1 }; +E se3 = { 1, 2}; + +int +main () +{ + if (sb1.x != 0 || sb1.x != sb1.y) + __builtin_abort(); + if (sb2.x != 1 || sb2.x != sb2.y) + __builtin_abort(); + if (sb3.x != 1 || sb3.y != 2) + __builtin_abort(); + + if (sc1.x != 0 || sc1.x != sc1.y) + __builtin_abort(); + if (sc2.x != 1 || sc2.x != sc2.y) + __builtin_abort(); + if (sc3.x != 1 || sc3.y != 2) + __builtin_abort(); + + B b1 = { }; + B b2 = { 1 }; + B b3 = { 1, 2}; + if (b1.x != 0 || b1.x != b1.y) + __builtin_abort(); + if (b2.x != 1 || b2.x != b2.y) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); + + C c1 = { }; + C c2 = { 1 }; + C c3 = { 1, 2}; + if (c1.x != 0 || c1.x != c1.y) + __builtin_abort(); + if (c2.x != 1 || c2.x != c2.y) + __builtin_abort(); + if (c3.x != 1 || c3.y != 2) + __builtin_abort(); + + D d1 = { }; + D d2 = { 1 }; + D d3 = { 1, 2}; + if (d1.x != 0 || d1.x != d1.y) + __builtin_abort(); + if (d2.x != 1 || d2.x != d2.y) + __builtin_abort(); + if (d3.x != 1 || d3.y != 2) + __builtin_abort(); + + E e1 = { }; + E e2 = { 1 }; + E e3 = { 1, 2}; + if (e1.x != 0 || e1.x != e1.y) + __builtin_abort(); + if (e2.x != 1 || e2.x != e2.y) + __builtin_abort(); + if (e3.x != 1 || e3.y != 2) + __builtin_abort(); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C new file mode 100644 index 00000000000..d091d693042 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C @@ -0,0 +1,80 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +static void +test_b (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) +{ + if (b1.x != 0 || b1.y != b1.x) + __builtin_abort(); + if (b2.x != 1 || b2.y != b2.x) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); +} + +struct C { + int x = 0; + int y = (true, A{x}.y) + (A{x}.y, 0); +}; + +static void +test_c (C c1 = C{}, C c2 = C{1}, C c3 = C{1, 2}) +{ + if (c1.x != 0 || c1.y != c1.x) + __builtin_abort(); + if (c2.x != 1 || c2.y != c2.x) + __builtin_abort(); + if (c3.x != 1 || c3.y != 2) + __builtin_abort(); +} + +struct D { + int x = 0; + int y = (A{x}.y); +}; + +static void +test_d (D d1 = D{}, D d2 = D{1}, D d3 = D{1, 2}) +{ + if (d1.x != 0 || d1.y != d1.x) + __builtin_abort(); + if (d2.x != 1 || d2.y != d2.x) + __builtin_abort(); + if (d3.x != 1 || d3.y != 2) + __builtin_abort(); +} + +struct E { + int x = 0; + int y = x ? A{x}.y : A{x}.y; +}; + +static void +test_e (E e1 = E{}, E e2 = E{1}, E e3 = E{1, 2}) +{ + if (e1.x != 0 || e1.y != e1.x) + __builtin_abort(); + if (e2.x != 1 || e2.y != e2.x) + __builtin_abort(); + if (e3.x != 1 || e3.y != 2) + __builtin_abort(); +} + +int +main () +{ + test_b (); + test_c (); + test_d (); + test_e (); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C new file mode 100644 index 00000000000..dc6492c1b0b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C @@ -0,0 +1,58 @@ +// PR c++/100252 +// { dg-do compile { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + const A* p = this; +}; + +struct B { + A a = A{}; +}; + +constexpr B b; +SA(b.a.p == &b.a); +B b1 = { }; + +struct C { + A a = (true, A{}); +}; + +constexpr C c; +SA(c.a.p == &c.a); +C c1 = { }; + +struct D { + A a = (A{}); +}; + +constexpr D d; +SA(d.a.p == &d.a); +D d1 = { }; + +static constexpr A global_a; + +struct E { + A a = true ? A{} : A{}; + A b = true ? global_a : (false ? A{} : A{}); + A c = true ? (false ? A{} : A{}) : global_a; + A d = true ? (false ? A{} : A{}) : (false ? A{} : A{}); +}; + +// FIXME: When fixing this, also fix nsdmi-aggr17.C. +constexpr E e; // { dg-bogus "" "PR105550" { xfail *-*-* } } +SA (e.a.p == &e.a); // { dg-bogus "" "PR105550" { xfail *-*-* } } + +E e1 = { }; + +struct F { + bool b = (A{}, true); +}; + +constexpr F f; + +void +g (B b2 = B{}, C c2 = C{}, D d2 = D{}, E e2 = E{}) +{ +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C new file mode 100644 index 00000000000..fc27a2cdac7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C @@ -0,0 +1,138 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + int x; + int y = x; + const A* p = this; +}; + +struct B { + int x = 42; + A a = A{x}; +}; + +constexpr B b; +SA(b.a.p == &b.a); +SA(b.x == 42); +B b2 = { }; +B b3 = { 42 }; + +struct C { + int x = 42; + B b = B{x}; +}; + +constexpr C c; +C c2; +C c3; + +struct D { + int x = 42; + A a = (true, A{x}); +}; + +constexpr D d; +SA(d.a.p == &d.a); +SA(d.x == 42); +D d2 = { }; +D d3 = { 42 }; + +struct E { + int x = 42; + A a = (A{x}); +}; + +constexpr E e; +SA(e.a.p == &e.a); +SA(e.x == 42); +E e2 = { }; +E e3 = { 42 }; + +struct F { + int x = 42; + A a = true ? A{x} : A{x}; +}; + +// FIXME: Doesn't work due to PR105550. +//constexpr F f; +//SA (f.a.p == &f.a); +SA (e.x == 42); +F f2 = { }; +F f3 = { 42 }; + +static void +test_b (B b4 = B{}, B b5 = B{ 42 }) +{ + if (b2.x != 42 || b2.a.x != 42 || b2.a.y != b2.a.x) + __builtin_abort (); + if (b3.x != 42 || b3.a.x != 42 || b3.a.y != b3.a.x) + __builtin_abort (); + if (b4.x != 42 || b4.a.x != 42 || b4.a.y != b4.a.x) + __builtin_abort (); + if (b5.x != 42 || b5.a.x != 42 || b5.a.y != b5.a.x) + __builtin_abort (); +} + +static void +test_c (C c4 = C{}, C c5 = C{ 42 }) +{ + if (c2.b.x != 42 || c2.b.a.x != 42 || c2.b.a.y != c2.b.a.x) + __builtin_abort (); + if (c3.b.x != 42 || c3.b.a.x != 42 || c3.b.a.y != c3.b.a.x) + __builtin_abort (); + if (c4.b.x != 42 || c4.b.a.x != 42 || c4.b.a.y != c4.b.a.x) + __builtin_abort (); + if (c5.b.x != 42 || c5.b.a.x != 42 || c5.b.a.y != c5.b.a.x) + __builtin_abort (); +} + +static void +test_d (D d4 = D{}, D d5 = D{ 42 }) +{ + if (d2.x != 42 || d2.a.x != 42 || d2.a.y != d2.a.x) + __builtin_abort (); + if (d3.x != 42 || d3.a.x != 42 || d3.a.y != d3.a.x) + __builtin_abort (); + if (d4.x != 42 || d4.a.x != 42 || d4.a.y != d4.a.x) + __builtin_abort (); + if (d5.x != 42 || d5.a.x != 42 || d5.a.y != d5.a.x) + __builtin_abort (); +} + +static void +test_e (E e4 = E{}, E e5 = E{ 42 }) +{ + if (e2.x != 42 || e2.a.x != 42 || e2.a.y != e2.a.x) + __builtin_abort (); + if (e3.x != 42 || e3.a.x != 42 || e3.a.y != e3.a.x) + __builtin_abort (); + if (e4.x != 42 || e4.a.x != 42 || e4.a.y != e4.a.x) + __builtin_abort (); + if (e5.x != 42 || e5.a.x != 42 || e5.a.y != e5.a.x) + __builtin_abort (); +} + +static void +test_f (F f4 = F{}, F f5 = F{ 42 }) +{ + if (f2.x != 42 || f2.a.x != 42 || f2.a.y != f2.a.x) + __builtin_abort (); + if (f3.x != 42 || f3.a.x != 42 || f3.a.y != f3.a.x) + __builtin_abort (); + if (f4.x != 42 || f4.a.x != 42 || f4.a.y != f4.a.x) + __builtin_abort (); + if (f5.x != 42 || f5.a.x != 42 || f5.a.y != f5.a.x) + __builtin_abort (); +} +int +main () +{ + test_b (); + test_c (); + test_d (); + test_e (); + test_f (); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C new file mode 100644 index 00000000000..567b8ee96d9 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C @@ -0,0 +1,56 @@ +// PR c++/100252 +// { dg-do compile { target c++14 } } + +struct B { }; + +struct A { + int x; + int y = x; + constexpr operator B() { return B{}; } +}; + +struct C { + int x = 42; + B b = A{x}; +}; + +C c1 = {}; +C c2 = { 42 }; +constexpr C c3 = {}; +constexpr C c4 = { 42 }; + +struct D { + int x = 42; + B b = (true, A{x}); +}; + +D d1 = {}; +D d2 = { 42 }; +constexpr D d3 = {}; +constexpr D d4 = { 42 }; + +struct E { + int x = 42; + B b = (A{x}); +}; + +E e1 = {}; +E e2 = { 42 }; +constexpr E e3 = {}; +constexpr E e4 = { 42 }; + +struct F { + int x = 42; + B b = (A{x}); +}; + +F f1 = {}; +F f2 = { 42 }; +constexpr F f3 = {}; +constexpr F f4 = { 42 }; + +void +g (C c5 = C{}, C c6 = C{ 42 }, D d5 = D{}, D d6 = D{ 42 }, + E e5 = E{}, E e6 = E{ 42 }, F f5 = F{}, F f6 = F{ 42 }) +{ +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C new file mode 100644 index 00000000000..f4892e3379b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C @@ -0,0 +1,28 @@ +// PR c++/100252 +// { dg-do compile { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + const A* p = this; +}; + +struct B { + A a = (A{}, A{}); +}; + +constexpr B b; +SA(b.a.p == &b.a); + +struct C { + int x; + int y = x; +}; + +struct D { + int x = 0; + int y = (C{x}.y, C{x}.y); +}; + +constexpr D d = { }; +D d2 = {}; base-commit: dd7813f05df50d2ad8e0dc34503f2dff0b521d89 -- 2.36.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-13 23:41 ` [PATCH v4] " Marek Polacek @ 2022-05-15 3:13 ` Jason Merrill 2022-05-16 15:36 ` [PATCH v5] " Marek Polacek 0 siblings, 1 reply; 17+ messages in thread From: Jason Merrill @ 2022-05-15 3:13 UTC (permalink / raw) To: Marek Polacek; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On 5/13/22 19:41, Marek Polacek wrote: > On Sat, May 07, 2022 at 06:02:13PM -0400, Jason Merrill wrote: >> On 5/7/22 15:11, Marek Polacek wrote: >>> On Tue, May 03, 2022 at 04:59:38PM -0400, Jason Merrill wrote: >>>> Does this testcase still work with this patch? >>>> >>>> struct A { >>>> const A* p = this; >>>> }; >>>> >>>> struct B { >>>> A a = A{}; >>>> }; >>>> >>>> constexpr B b; >>>> static_assert (b.a.p == &b.a); >>> >>> Ouch, no. Thanks for catching this, it would have been very unpleasant >>> to hit this problem later... >>> >>> The reason your testcase was rejected is that replacing a PLACEHOLDER >>> in the outermost TARGET_EXPR broke guaranteed copy elision. Now that >>> I've spent another day on this, I still think we have to replace the >>> placeholders created when temporary materialization takes place in an >>> NSDMI, but it must happen in digest_nsdmi_init (as Patrick suggested), >>> where we can see the full initializer. That way I can avoid breaking >>> copy elision. >>> >>> I've added a bunch of new testcases and updated the >>> (already long) description. The yuge comment hopefully also explains >>> how this problem is avoided. Thanks again, >>> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>> >>> -- >8 -- >>> Consider >>> >>> struct A { >>> int x; >>> int y = x; >>> }; >>> >>> struct B { >>> int x = 0; >>> int y = A{x}.y; // #1 >>> }; >>> >>> where for #1 we end up with >>> >>> {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} >>> >>> that is, two PLACEHOLDER_EXPRs for different types on the same level in >>> a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to >>> avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. >>> >>> Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing >>> cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal >>> on type=A, compound_literal={((struct B *) this)->x}. When digesting this >>> initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't >>> have any object to refer to yet. After digesting, we have >>> >>> {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} >>> >>> and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor >>> CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns >>> >>> TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> >>> >>> Then we get to >>> >>> B b = {}; >>> >>> and call store_init_value, which digests the {}, which produces >>> >>> {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} >>> >>> lookup_placeholder in constexpr won't find an object to replace the >>> PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we >>> cannot search outward from D.2395 to find 'b'. >>> >>> The call to replace_placeholders in store_init_value will not do anything: >>> we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only >>> a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> >>> stays even though now is the perfect time to replace it because we have an >>> object for it: 'b'. >>> >>> Later, in cp_gimplify_init_expr the *expr_p is >>> >>> D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} >>> >>> where D.2395 is of type A, but we crash because we hit <P_E struct B>, which >>> has a different type. >>> >>> My idea was to replace <P_E struct A> with D.2384 after creating the >>> TARGET_EXPR because that means we have an object we can refer to. >>> Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have >>> a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to >>> replace <P_E struct B> with 'b', and we should be good to go. We must >>> be careful not to break guaranteed copy elision, so this replacement >>> happens in digest_nsdmi_init where we can see the whole initializer, >>> and avoid replacing any placeholders in the outermost TARGET_EXPR. >>> >>> PR c++/100252 >>> >>> gcc/cp/ChangeLog: >>> >>> * typeck2.cc (replace_placeholders_for_class_temp_r): New. >>> (digest_nsdmi_init): Call it. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/cpp1y/nsdmi-aggr14.C: New test. >>> * g++.dg/cpp1y/nsdmi-aggr15.C: New test. >>> * g++.dg/cpp1y/nsdmi-aggr16.C: New test. >>> * g++.dg/cpp1y/nsdmi-aggr17.C: New test. >>> * g++.dg/cpp1y/nsdmi-aggr18.C: New test. >>> --- >>> gcc/cp/typeck2.cc | 54 +++++++++++++++++++++++ >>> gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++ >>> gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++ >>> gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 19 ++++++++ >>> gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 47 ++++++++++++++++++++ >>> gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 25 +++++++++++ >>> 6 files changed, 220 insertions(+) >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C >>> create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C >>> >>> diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc >>> index 63d95c1529a..72d7cfaf1d3 100644 >>> --- a/gcc/cp/typeck2.cc >>> +++ b/gcc/cp/typeck2.cc >>> @@ -1371,6 +1371,34 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) >>> return digest_init_r (type, init, 0, flags, complain); >>> } >>> +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used >>> + in the context of guaranteed copy elision). */ >>> + >>> +static tree >>> +replace_placeholders_for_class_temp_r (tree *tp, int *walk_subtrees, void *data) >>> +{ >>> + tree t = *tp; >>> + tree full_expr = *static_cast<tree *>(data); >>> + >>> + /* We're looking for a TARGET_EXPR nested in the whole expression. */ >>> + if (TREE_CODE (t) == TARGET_EXPR && t != full_expr) >> >> Just comparing to full_expr doesn't seem enough to catch all the cases where >> a prvalue is used as an initializer rather than for temporary >> materialization; you will also need to handle comma and conditional >> expressions. And probably PAREN_EXPR. >> >> I'm failing to find a term for this in the standard; "potential result" also >> includes objects that the result is an xvalue subobject of (as in #1 below). >> Maybe "potential prvalue result", to exclude the xvalue cases? So, >> >> && !potential_prvalue_result_of (t, full_expr) >> >> ? > > Thanks, the previous version indeed did have problems when ?: or , was used. > I've greatly enhanced the tests to detect more cases, and added > potential_prvalue_result_of. > > While looking into this, I found PR105550, which turned out to be a big > can of worms. So, XFAIL for now. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > Consider > > struct A { > int x; > int y = x; > }; > > struct B { > int x = 0; > int y = A{x}.y; // #1 > }; > > where for #1 we end up with > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > have any object to refer to yet. After digesting, we have > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > Then we get to > > B b = {}; > > and call store_init_value, which digests the {}, which produces > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > lookup_placeholder in constexpr won't find an object to replace the > PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we > cannot search outward from D.2395 to find 'b'. > > The call to replace_placeholders in store_init_value will not do anything: > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > stays even though now is the perfect time to replace it because we have an > object for it: 'b'. > > Later, in cp_gimplify_init_expr the *expr_p is > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > has a different type. > > My idea was to replace <P_E struct A> with D.2384 after creating the > TARGET_EXPR because that means we have an object we can refer to. > Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have > a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to > replace <P_E struct B> with 'b', and we should be good to go. We must > be careful not to break guaranteed copy elision, so this replacement > happens in digest_nsdmi_init where we can see the whole initializer, > and avoid replacing any placeholders in TARGET_EXPRs used in the context > of initialization/copy elision. This is achieved via the new function > called potential_prvalue_result_of. > > While fixing this problem, I found PR105550, thus the FIXMEs in the > tests. > > PR c++/100252 > > gcc/cp/ChangeLog: > > * typeck2.cc (potential_prvalue_result_of): New. > (replace_placeholders_for_class_temp_r): New. > (digest_nsdmi_init): Call it. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/nsdmi-aggr14.C: New test. > * g++.dg/cpp1y/nsdmi-aggr15.C: New test. > * g++.dg/cpp1y/nsdmi-aggr16.C: New test. > * g++.dg/cpp1y/nsdmi-aggr17.C: New test. > * g++.dg/cpp1y/nsdmi-aggr18.C: New test. > * g++.dg/cpp1y/nsdmi-aggr19.C: New test. > --- > gcc/cp/typeck2.cc | 90 ++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 131 ++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 80 +++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 58 +++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 138 ++++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 56 +++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C | 28 +++++ > 7 files changed, 581 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > index 1d92310edd0..f60e1df4c27 100644 > --- a/gcc/cp/typeck2.cc > +++ b/gcc/cp/typeck2.cc > @@ -1371,6 +1371,70 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) > return digest_init_r (type, init, 0, flags, complain); > } > > +/* Return true if a prvalue is used as an initializer rather than for > + temporary materialization. For instance: I might say "if SUBOB initializes the same object as FULL_EXPR"; the full-expression could still end up initializing a temporary. > + A a = A{}; // initializer > + A a = (A{}); // initializer > + A a = (1, A{}); // initializer > + A a = true ? A{} : A{}; // initializer > + auto x = A{}.x; // temporary materialization > + auto x = foo(A{}); // temporary materialization > + > + FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ > + > +static bool > +potential_prvalue_result_of (tree subob, tree full_expr) > +{ > + if (subob == full_expr) > + return true; > + else if (TREE_CODE (full_expr) == TARGET_EXPR) > + { > + tree init = TARGET_EXPR_INITIAL (full_expr); > + if (TREE_CODE (init) == COND_EXPR) > + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)) > + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 2))); > + else if (TREE_CODE (init) == COMPOUND_EXPR) > + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 0)) We shouldn't recurse into the LHS of the comma, only the RHS. > + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 1))); > + /* ??? I don't know if this can be hit. If so, look inside the ( ) > + instead of the assert. */ > + else if (TREE_CODE (init) == PAREN_EXPR) > + gcc_checking_assert (false); It seems trivial enough to recurse after the assert, in case it does happen in the wild. > + } > + return false; > +} > + > +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used > + in the context of guaranteed copy elision). */ > + > +static tree > +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) > +{ > + tree t = *tp; > + tree full_expr = *static_cast<tree *>(data); > + > + /* We're looking for a TARGET_EXPR nested in the whole expression. */ > + if (TREE_CODE (t) == TARGET_EXPR > + && !potential_prvalue_result_of (t, full_expr)) > + { > + tree init = TARGET_EXPR_INITIAL (t); > + while (TREE_CODE (init) == COMPOUND_EXPR) > + init = TREE_OPERAND (init, 1); > + if (TREE_CODE (init) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) > + { > + tree obj = TARGET_EXPR_SLOT (t); > + replace_placeholders (init, obj); > + /* We should have dealt with all PLACEHOLDER_EXPRs. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; > + gcc_checking_assert (!find_placeholders (init)); > + } > + } > + > + return NULL_TREE; > +} > + > /* Process the initializer INIT for an NSDMI DECL (a FIELD_DECL). */ > tree > digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) > @@ -1390,6 +1454,32 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) > && CP_AGGREGATE_TYPE_P (type)) > init = reshape_init (type, init, complain); > init = digest_init_flags (type, init, flags, complain); > + > + /* We may have temporary materialization in a NSDMI, if the initializer > + has something like A{} in it. Digesting the {} could have introduced > + a PLACEHOLDER_EXPR referring to A. Now that we've got a TARGET_EXPR, > + we have an object we can refer to. The reason we bother doing this > + here is for code like > + > + struct A { > + int x; > + int y = x; > + }; > + > + struct B { > + int x = 0; > + int y = A{x}.y; // #1 > + }; > + > + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs for > + different types on the same level in a {} when lookup_placeholder > + wouldn't find a named object for the PLACEHOLDER_EXPR for A. Note, > + temporary materialization does not occur when initializing an object > + from a prvalue of the same type, therefore we must not replace the > + placeholder with a temporary object so that it can be elided. */ > + cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init, > + nullptr); > + > return init; > } > \f > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > new file mode 100644 > index 00000000000..28b908a0a1a > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > @@ -0,0 +1,131 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +constexpr B csb1 = { }; > +SA(csb1.x == 0 && csb1.y == csb1.x); > +constexpr B csb2 = { 1 }; > +SA(csb2.x == 1 && csb2.y == csb2.x); > +constexpr B csb3 = { 1, 2 }; > +SA(csb3.x == 1 && csb3.y == 2); > + > +B sb1 = { }; > +B sb2 = { 1 }; > +B sb3 = { 1, 2}; > + > +struct C { > + int x = 0; > + int y = (true, A{x}.y) + (A{x}.y, 0); > +}; > + > +constexpr C csc1 = { }; > +SA(csc1.x == 0 && csc1.y == csc1.x); > +constexpr C csc2 = { 1 }; > +SA(csc2.x == 1 && csc2.y == csc2.x); > +constexpr C csc3 = { 1, 2 }; > +SA(csc3.x == 1 && csc3.y == 2); > + > +C sc1 = { }; > +C sc2 = { 1 }; > +C sc3 = { 1, 2}; > + > +struct D { > + int x = 0; > + int y = (A{x}.y); > +}; > + > +constexpr D csd1 = { }; > +SA(csd1.x == 0 && csd1.y == csd1.x); > +constexpr D csd2 = { 1 }; > +SA(csd2.x == 1 && csd2.y == csd2.x); > +constexpr D csd3 = { 1, 2 }; > +SA(csd3.x == 1 && csd3.y == 2); > + > +D sd1 = { }; > +D sd2 = { 1 }; > +D sd3 = { 1, 2}; > + > +struct E { > + int x = 0; > + int y = x ? A{x}.y : A{x}.y; > +}; > + > +constexpr E cse1 = { }; > +SA(cse1.x == 0 && cse1.y == cse1.x); > +constexpr E cse2 = { 1 }; > +SA(cse2.x == 1 && cse2.y == cse2.x); > +constexpr E cse3 = { 1, 2 }; > +SA(cse3.x == 1 && cse3.y == 2); > + > +E se1 = { }; > +E se2 = { 1 }; > +E se3 = { 1, 2}; > + > +int > +main () > +{ > + if (sb1.x != 0 || sb1.x != sb1.y) > + __builtin_abort(); > + if (sb2.x != 1 || sb2.x != sb2.y) > + __builtin_abort(); > + if (sb3.x != 1 || sb3.y != 2) > + __builtin_abort(); > + > + if (sc1.x != 0 || sc1.x != sc1.y) > + __builtin_abort(); > + if (sc2.x != 1 || sc2.x != sc2.y) > + __builtin_abort(); > + if (sc3.x != 1 || sc3.y != 2) > + __builtin_abort(); > + > + B b1 = { }; > + B b2 = { 1 }; > + B b3 = { 1, 2}; > + if (b1.x != 0 || b1.x != b1.y) > + __builtin_abort(); > + if (b2.x != 1 || b2.x != b2.y) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > + > + C c1 = { }; > + C c2 = { 1 }; > + C c3 = { 1, 2}; > + if (c1.x != 0 || c1.x != c1.y) > + __builtin_abort(); > + if (c2.x != 1 || c2.x != c2.y) > + __builtin_abort(); > + if (c3.x != 1 || c3.y != 2) > + __builtin_abort(); > + > + D d1 = { }; > + D d2 = { 1 }; > + D d3 = { 1, 2}; > + if (d1.x != 0 || d1.x != d1.y) > + __builtin_abort(); > + if (d2.x != 1 || d2.x != d2.y) > + __builtin_abort(); > + if (d3.x != 1 || d3.y != 2) > + __builtin_abort(); > + > + E e1 = { }; > + E e2 = { 1 }; > + E e3 = { 1, 2}; > + if (e1.x != 0 || e1.x != e1.y) > + __builtin_abort(); > + if (e2.x != 1 || e2.x != e2.y) > + __builtin_abort(); > + if (e3.x != 1 || e3.y != 2) > + __builtin_abort(); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > new file mode 100644 > index 00000000000..d091d693042 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > @@ -0,0 +1,80 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +static void > +test_b (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) > +{ > + if (b1.x != 0 || b1.y != b1.x) > + __builtin_abort(); > + if (b2.x != 1 || b2.y != b2.x) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > + > +struct C { > + int x = 0; > + int y = (true, A{x}.y) + (A{x}.y, 0); > +}; > + > +static void > +test_c (C c1 = C{}, C c2 = C{1}, C c3 = C{1, 2}) > +{ > + if (c1.x != 0 || c1.y != c1.x) > + __builtin_abort(); > + if (c2.x != 1 || c2.y != c2.x) > + __builtin_abort(); > + if (c3.x != 1 || c3.y != 2) > + __builtin_abort(); > +} > + > +struct D { > + int x = 0; > + int y = (A{x}.y); > +}; > + > +static void > +test_d (D d1 = D{}, D d2 = D{1}, D d3 = D{1, 2}) > +{ > + if (d1.x != 0 || d1.y != d1.x) > + __builtin_abort(); > + if (d2.x != 1 || d2.y != d2.x) > + __builtin_abort(); > + if (d3.x != 1 || d3.y != 2) > + __builtin_abort(); > +} > + > +struct E { > + int x = 0; > + int y = x ? A{x}.y : A{x}.y; > +}; > + > +static void > +test_e (E e1 = E{}, E e2 = E{1}, E e3 = E{1, 2}) > +{ > + if (e1.x != 0 || e1.y != e1.x) > + __builtin_abort(); > + if (e2.x != 1 || e2.y != e2.x) > + __builtin_abort(); > + if (e3.x != 1 || e3.y != 2) > + __builtin_abort(); > +} > + > +int > +main () > +{ > + test_b (); > + test_c (); > + test_d (); > + test_e (); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > new file mode 100644 > index 00000000000..dc6492c1b0b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > @@ -0,0 +1,58 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + const A* p = this; > +}; > + > +struct B { > + A a = A{}; > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > +B b1 = { }; > + > +struct C { > + A a = (true, A{}); > +}; > + > +constexpr C c; > +SA(c.a.p == &c.a); > +C c1 = { }; > + > +struct D { > + A a = (A{}); > +}; > + > +constexpr D d; > +SA(d.a.p == &d.a); > +D d1 = { }; > + > +static constexpr A global_a; > + > +struct E { > + A a = true ? A{} : A{}; > + A b = true ? global_a : (false ? A{} : A{}); > + A c = true ? (false ? A{} : A{}) : global_a; > + A d = true ? (false ? A{} : A{}) : (false ? A{} : A{}); > +}; > + > +// FIXME: When fixing this, also fix nsdmi-aggr17.C. > +constexpr E e; // { dg-bogus "" "PR105550" { xfail *-*-* } } > +SA (e.a.p == &e.a); // { dg-bogus "" "PR105550" { xfail *-*-* } } > + > +E e1 = { }; > + > +struct F { > + bool b = (A{}, true); > +}; > + > +constexpr F f; > + > +void > +g (B b2 = B{}, C c2 = C{}, D d2 = D{}, E e2 = E{}) > +{ > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > new file mode 100644 > index 00000000000..fc27a2cdac7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > @@ -0,0 +1,138 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > + const A* p = this; > +}; > + > +struct B { > + int x = 42; > + A a = A{x}; > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > +SA(b.x == 42); > +B b2 = { }; > +B b3 = { 42 }; > + > +struct C { > + int x = 42; > + B b = B{x}; > +}; > + > +constexpr C c; > +C c2; > +C c3; > + > +struct D { > + int x = 42; > + A a = (true, A{x}); > +}; > + > +constexpr D d; > +SA(d.a.p == &d.a); > +SA(d.x == 42); > +D d2 = { }; > +D d3 = { 42 }; > + > +struct E { > + int x = 42; > + A a = (A{x}); > +}; > + > +constexpr E e; > +SA(e.a.p == &e.a); > +SA(e.x == 42); > +E e2 = { }; > +E e3 = { 42 }; > + > +struct F { > + int x = 42; > + A a = true ? A{x} : A{x}; > +}; > + > +// FIXME: Doesn't work due to PR105550. > +//constexpr F f; > +//SA (f.a.p == &f.a); > +SA (e.x == 42); > +F f2 = { }; > +F f3 = { 42 }; > + > +static void > +test_b (B b4 = B{}, B b5 = B{ 42 }) > +{ > + if (b2.x != 42 || b2.a.x != 42 || b2.a.y != b2.a.x) > + __builtin_abort (); > + if (b3.x != 42 || b3.a.x != 42 || b3.a.y != b3.a.x) > + __builtin_abort (); > + if (b4.x != 42 || b4.a.x != 42 || b4.a.y != b4.a.x) > + __builtin_abort (); > + if (b5.x != 42 || b5.a.x != 42 || b5.a.y != b5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_c (C c4 = C{}, C c5 = C{ 42 }) > +{ > + if (c2.b.x != 42 || c2.b.a.x != 42 || c2.b.a.y != c2.b.a.x) > + __builtin_abort (); > + if (c3.b.x != 42 || c3.b.a.x != 42 || c3.b.a.y != c3.b.a.x) > + __builtin_abort (); > + if (c4.b.x != 42 || c4.b.a.x != 42 || c4.b.a.y != c4.b.a.x) > + __builtin_abort (); > + if (c5.b.x != 42 || c5.b.a.x != 42 || c5.b.a.y != c5.b.a.x) > + __builtin_abort (); > +} > + > +static void > +test_d (D d4 = D{}, D d5 = D{ 42 }) > +{ > + if (d2.x != 42 || d2.a.x != 42 || d2.a.y != d2.a.x) > + __builtin_abort (); > + if (d3.x != 42 || d3.a.x != 42 || d3.a.y != d3.a.x) > + __builtin_abort (); > + if (d4.x != 42 || d4.a.x != 42 || d4.a.y != d4.a.x) > + __builtin_abort (); > + if (d5.x != 42 || d5.a.x != 42 || d5.a.y != d5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_e (E e4 = E{}, E e5 = E{ 42 }) > +{ > + if (e2.x != 42 || e2.a.x != 42 || e2.a.y != e2.a.x) > + __builtin_abort (); > + if (e3.x != 42 || e3.a.x != 42 || e3.a.y != e3.a.x) > + __builtin_abort (); > + if (e4.x != 42 || e4.a.x != 42 || e4.a.y != e4.a.x) > + __builtin_abort (); > + if (e5.x != 42 || e5.a.x != 42 || e5.a.y != e5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_f (F f4 = F{}, F f5 = F{ 42 }) > +{ > + if (f2.x != 42 || f2.a.x != 42 || f2.a.y != f2.a.x) > + __builtin_abort (); > + if (f3.x != 42 || f3.a.x != 42 || f3.a.y != f3.a.x) > + __builtin_abort (); > + if (f4.x != 42 || f4.a.x != 42 || f4.a.y != f4.a.x) > + __builtin_abort (); > + if (f5.x != 42 || f5.a.x != 42 || f5.a.y != f5.a.x) > + __builtin_abort (); > +} > +int > +main () > +{ > + test_b (); > + test_c (); > + test_d (); > + test_e (); > + test_f (); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > new file mode 100644 > index 00000000000..567b8ee96d9 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > @@ -0,0 +1,56 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +struct B { }; > + > +struct A { > + int x; > + int y = x; > + constexpr operator B() { return B{}; } > +}; > + > +struct C { > + int x = 42; > + B b = A{x}; > +}; > + > +C c1 = {}; > +C c2 = { 42 }; > +constexpr C c3 = {}; > +constexpr C c4 = { 42 }; > + > +struct D { > + int x = 42; > + B b = (true, A{x}); > +}; > + > +D d1 = {}; > +D d2 = { 42 }; > +constexpr D d3 = {}; > +constexpr D d4 = { 42 }; > + > +struct E { > + int x = 42; > + B b = (A{x}); > +}; > + > +E e1 = {}; > +E e2 = { 42 }; > +constexpr E e3 = {}; > +constexpr E e4 = { 42 }; > + > +struct F { > + int x = 42; > + B b = (A{x}); > +}; > + > +F f1 = {}; > +F f2 = { 42 }; > +constexpr F f3 = {}; > +constexpr F f4 = { 42 }; > + > +void > +g (C c5 = C{}, C c6 = C{ 42 }, D d5 = D{}, D d6 = D{ 42 }, > + E e5 = E{}, E e6 = E{ 42 }, F f5 = F{}, F f6 = F{ 42 }) > +{ > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > new file mode 100644 > index 00000000000..f4892e3379b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > @@ -0,0 +1,28 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + const A* p = this; > +}; > + > +struct B { > + A a = (A{}, A{}); > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > + > +struct C { > + int x; > + int y = x; > +}; > + > +struct D { > + int x = 0; > + int y = (C{x}.y, C{x}.y); > +}; > + > +constexpr D d = { }; > +D d2 = {}; > > base-commit: dd7813f05df50d2ad8e0dc34503f2dff0b521d89 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-15 3:13 ` Jason Merrill @ 2022-05-16 15:36 ` Marek Polacek 2022-05-24 11:40 ` Marek Polacek 2022-05-24 12:36 ` Jason Merrill 0 siblings, 2 replies; 17+ messages in thread From: Marek Polacek @ 2022-05-16 15:36 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On Sat, May 14, 2022 at 11:13:28PM -0400, Jason Merrill wrote: > On 5/13/22 19:41, Marek Polacek wrote: > > --- a/gcc/cp/typeck2.cc > > +++ b/gcc/cp/typeck2.cc > > @@ -1371,6 +1371,70 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) > > return digest_init_r (type, init, 0, flags, complain); > > } > > +/* Return true if a prvalue is used as an initializer rather than for > > + temporary materialization. For instance: > > I might say "if SUBOB initializes the same object as FULL_EXPR"; the > full-expression could still end up initializing a temporary. Fixed. > > + A a = A{}; // initializer > > + A a = (A{}); // initializer > > + A a = (1, A{}); // initializer > > + A a = true ? A{} : A{}; // initializer > > + auto x = A{}.x; // temporary materialization > > + auto x = foo(A{}); // temporary materialization > > + > > + FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ > > + > > +static bool > > +potential_prvalue_result_of (tree subob, tree full_expr) > > +{ > > + if (subob == full_expr) > > + return true; > > + else if (TREE_CODE (full_expr) == TARGET_EXPR) > > + { > > + tree init = TARGET_EXPR_INITIAL (full_expr); > > + if (TREE_CODE (init) == COND_EXPR) > > + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)) > > + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 2))); > > + else if (TREE_CODE (init) == COMPOUND_EXPR) > > + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 0)) > > We shouldn't recurse into the LHS of the comma, only the RHS. Fixed. > > + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 1))); > > + /* ??? I don't know if this can be hit. If so, look inside the ( ) > > + instead of the assert. */ > > + else if (TREE_CODE (init) == PAREN_EXPR) > > + gcc_checking_assert (false); > > It seems trivial enough to recurse after the assert, in case it does happen > in the wild. OK, adjusted. Thanks! Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Consider struct A { int x; int y = x; }; struct B { int x = 0; int y = A{x}.y; // #1 }; where for #1 we end up with {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} that is, two PLACEHOLDER_EXPRs for different types on the same level in a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal on type=A, compound_literal={((struct B *) this)->x}. When digesting this initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't have any object to refer to yet. After digesting, we have {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> Then we get to B b = {}; and call store_init_value, which digests the {}, which produces {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} lookup_placeholder in constexpr won't find an object to replace the PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we cannot search outward from D.2395 to find 'b'. The call to replace_placeholders in store_init_value will not do anything: we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> stays even though now is the perfect time to replace it because we have an object for it: 'b'. Later, in cp_gimplify_init_expr the *expr_p is D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} where D.2395 is of type A, but we crash because we hit <P_E struct B>, which has a different type. My idea was to replace <P_E struct A> with D.2384 after creating the TARGET_EXPR because that means we have an object we can refer to. Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to replace <P_E struct B> with 'b', and we should be good to go. We must be careful not to break guaranteed copy elision, so this replacement happens in digest_nsdmi_init where we can see the whole initializer, and avoid replacing any placeholders in TARGET_EXPRs used in the context of initialization/copy elision. This is achieved via the new function called potential_prvalue_result_of. While fixing this problem, I found PR105550, thus the FIXMEs in the tests. PR c++/100252 gcc/cp/ChangeLog: * typeck2.cc (potential_prvalue_result_of): New. (replace_placeholders_for_class_temp_r): New. (digest_nsdmi_init): Call it. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/nsdmi-aggr14.C: New test. * g++.dg/cpp1y/nsdmi-aggr15.C: New test. * g++.dg/cpp1y/nsdmi-aggr16.C: New test. * g++.dg/cpp1y/nsdmi-aggr17.C: New test. * g++.dg/cpp1y/nsdmi-aggr18.C: New test. * g++.dg/cpp1y/nsdmi-aggr19.C: New test. --- gcc/cp/typeck2.cc | 91 ++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 131 ++++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 80 +++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 58 +++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 138 ++++++++++++++++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 56 +++++++++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C | 28 +++++ 7 files changed, 582 insertions(+) create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc index 1d92310edd0..1a96be3d412 100644 --- a/gcc/cp/typeck2.cc +++ b/gcc/cp/typeck2.cc @@ -1371,6 +1371,71 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) return digest_init_r (type, init, 0, flags, complain); } +/* Return true if SUBOB initializes the same object as FULL_EXPR. + For instance: + + A a = A{}; // initializer + A a = (A{}); // initializer + A a = (1, A{}); // initializer + A a = true ? A{} : A{}; // initializer + auto x = A{}.x; // temporary materialization + auto x = foo(A{}); // temporary materialization + + FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ + +static bool +potential_prvalue_result_of (tree subob, tree full_expr) +{ + if (subob == full_expr) + return true; + else if (TREE_CODE (full_expr) == TARGET_EXPR) + { + tree init = TARGET_EXPR_INITIAL (full_expr); + if (TREE_CODE (init) == COND_EXPR) + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)) + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 2))); + else if (TREE_CODE (init) == COMPOUND_EXPR) + return potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)); + /* ??? I don't know if this can be hit. */ + else if (TREE_CODE (init) == PAREN_EXPR) + { + gcc_checking_assert (false); + return potential_prvalue_result_of (subob, TREE_OPERAND (init, 0)); + } + } + return false; +} + +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used + in the context of guaranteed copy elision). */ + +static tree +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) +{ + tree t = *tp; + tree full_expr = *static_cast<tree *>(data); + + /* We're looking for a TARGET_EXPR nested in the whole expression. */ + if (TREE_CODE (t) == TARGET_EXPR + && !potential_prvalue_result_of (t, full_expr)) + { + tree init = TARGET_EXPR_INITIAL (t); + while (TREE_CODE (init) == COMPOUND_EXPR) + init = TREE_OPERAND (init, 1); + if (TREE_CODE (init) == CONSTRUCTOR + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) + { + tree obj = TARGET_EXPR_SLOT (t); + replace_placeholders (init, obj); + /* We should have dealt with all PLACEHOLDER_EXPRs. */ + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; + gcc_checking_assert (!find_placeholders (init)); + } + } + + return NULL_TREE; +} + /* Process the initializer INIT for an NSDMI DECL (a FIELD_DECL). */ tree digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) @@ -1390,6 +1455,32 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) && CP_AGGREGATE_TYPE_P (type)) init = reshape_init (type, init, complain); init = digest_init_flags (type, init, flags, complain); + + /* We may have temporary materialization in a NSDMI, if the initializer + has something like A{} in it. Digesting the {} could have introduced + a PLACEHOLDER_EXPR referring to A. Now that we've got a TARGET_EXPR, + we have an object we can refer to. The reason we bother doing this + here is for code like + + struct A { + int x; + int y = x; + }; + + struct B { + int x = 0; + int y = A{x}.y; // #1 + }; + + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs for + different types on the same level in a {} when lookup_placeholder + wouldn't find a named object for the PLACEHOLDER_EXPR for A. Note, + temporary materialization does not occur when initializing an object + from a prvalue of the same type, therefore we must not replace the + placeholder with a temporary object so that it can be elided. */ + cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init, + nullptr); + return init; } \f diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C new file mode 100644 index 00000000000..28b908a0a1a --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C @@ -0,0 +1,131 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +constexpr B csb1 = { }; +SA(csb1.x == 0 && csb1.y == csb1.x); +constexpr B csb2 = { 1 }; +SA(csb2.x == 1 && csb2.y == csb2.x); +constexpr B csb3 = { 1, 2 }; +SA(csb3.x == 1 && csb3.y == 2); + +B sb1 = { }; +B sb2 = { 1 }; +B sb3 = { 1, 2}; + +struct C { + int x = 0; + int y = (true, A{x}.y) + (A{x}.y, 0); +}; + +constexpr C csc1 = { }; +SA(csc1.x == 0 && csc1.y == csc1.x); +constexpr C csc2 = { 1 }; +SA(csc2.x == 1 && csc2.y == csc2.x); +constexpr C csc3 = { 1, 2 }; +SA(csc3.x == 1 && csc3.y == 2); + +C sc1 = { }; +C sc2 = { 1 }; +C sc3 = { 1, 2}; + +struct D { + int x = 0; + int y = (A{x}.y); +}; + +constexpr D csd1 = { }; +SA(csd1.x == 0 && csd1.y == csd1.x); +constexpr D csd2 = { 1 }; +SA(csd2.x == 1 && csd2.y == csd2.x); +constexpr D csd3 = { 1, 2 }; +SA(csd3.x == 1 && csd3.y == 2); + +D sd1 = { }; +D sd2 = { 1 }; +D sd3 = { 1, 2}; + +struct E { + int x = 0; + int y = x ? A{x}.y : A{x}.y; +}; + +constexpr E cse1 = { }; +SA(cse1.x == 0 && cse1.y == cse1.x); +constexpr E cse2 = { 1 }; +SA(cse2.x == 1 && cse2.y == cse2.x); +constexpr E cse3 = { 1, 2 }; +SA(cse3.x == 1 && cse3.y == 2); + +E se1 = { }; +E se2 = { 1 }; +E se3 = { 1, 2}; + +int +main () +{ + if (sb1.x != 0 || sb1.x != sb1.y) + __builtin_abort(); + if (sb2.x != 1 || sb2.x != sb2.y) + __builtin_abort(); + if (sb3.x != 1 || sb3.y != 2) + __builtin_abort(); + + if (sc1.x != 0 || sc1.x != sc1.y) + __builtin_abort(); + if (sc2.x != 1 || sc2.x != sc2.y) + __builtin_abort(); + if (sc3.x != 1 || sc3.y != 2) + __builtin_abort(); + + B b1 = { }; + B b2 = { 1 }; + B b3 = { 1, 2}; + if (b1.x != 0 || b1.x != b1.y) + __builtin_abort(); + if (b2.x != 1 || b2.x != b2.y) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); + + C c1 = { }; + C c2 = { 1 }; + C c3 = { 1, 2}; + if (c1.x != 0 || c1.x != c1.y) + __builtin_abort(); + if (c2.x != 1 || c2.x != c2.y) + __builtin_abort(); + if (c3.x != 1 || c3.y != 2) + __builtin_abort(); + + D d1 = { }; + D d2 = { 1 }; + D d3 = { 1, 2}; + if (d1.x != 0 || d1.x != d1.y) + __builtin_abort(); + if (d2.x != 1 || d2.x != d2.y) + __builtin_abort(); + if (d3.x != 1 || d3.y != 2) + __builtin_abort(); + + E e1 = { }; + E e2 = { 1 }; + E e3 = { 1, 2}; + if (e1.x != 0 || e1.x != e1.y) + __builtin_abort(); + if (e2.x != 1 || e2.x != e2.y) + __builtin_abort(); + if (e3.x != 1 || e3.y != 2) + __builtin_abort(); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C new file mode 100644 index 00000000000..d091d693042 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C @@ -0,0 +1,80 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +struct A { + int x; + int y = x; +}; + +struct B { + int x = 0; + int y = A{x}.y; +}; + +static void +test_b (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) +{ + if (b1.x != 0 || b1.y != b1.x) + __builtin_abort(); + if (b2.x != 1 || b2.y != b2.x) + __builtin_abort(); + if (b3.x != 1 || b3.y != 2) + __builtin_abort(); +} + +struct C { + int x = 0; + int y = (true, A{x}.y) + (A{x}.y, 0); +}; + +static void +test_c (C c1 = C{}, C c2 = C{1}, C c3 = C{1, 2}) +{ + if (c1.x != 0 || c1.y != c1.x) + __builtin_abort(); + if (c2.x != 1 || c2.y != c2.x) + __builtin_abort(); + if (c3.x != 1 || c3.y != 2) + __builtin_abort(); +} + +struct D { + int x = 0; + int y = (A{x}.y); +}; + +static void +test_d (D d1 = D{}, D d2 = D{1}, D d3 = D{1, 2}) +{ + if (d1.x != 0 || d1.y != d1.x) + __builtin_abort(); + if (d2.x != 1 || d2.y != d2.x) + __builtin_abort(); + if (d3.x != 1 || d3.y != 2) + __builtin_abort(); +} + +struct E { + int x = 0; + int y = x ? A{x}.y : A{x}.y; +}; + +static void +test_e (E e1 = E{}, E e2 = E{1}, E e3 = E{1, 2}) +{ + if (e1.x != 0 || e1.y != e1.x) + __builtin_abort(); + if (e2.x != 1 || e2.y != e2.x) + __builtin_abort(); + if (e3.x != 1 || e3.y != 2) + __builtin_abort(); +} + +int +main () +{ + test_b (); + test_c (); + test_d (); + test_e (); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C new file mode 100644 index 00000000000..dc6492c1b0b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C @@ -0,0 +1,58 @@ +// PR c++/100252 +// { dg-do compile { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + const A* p = this; +}; + +struct B { + A a = A{}; +}; + +constexpr B b; +SA(b.a.p == &b.a); +B b1 = { }; + +struct C { + A a = (true, A{}); +}; + +constexpr C c; +SA(c.a.p == &c.a); +C c1 = { }; + +struct D { + A a = (A{}); +}; + +constexpr D d; +SA(d.a.p == &d.a); +D d1 = { }; + +static constexpr A global_a; + +struct E { + A a = true ? A{} : A{}; + A b = true ? global_a : (false ? A{} : A{}); + A c = true ? (false ? A{} : A{}) : global_a; + A d = true ? (false ? A{} : A{}) : (false ? A{} : A{}); +}; + +// FIXME: When fixing this, also fix nsdmi-aggr17.C. +constexpr E e; // { dg-bogus "" "PR105550" { xfail *-*-* } } +SA (e.a.p == &e.a); // { dg-bogus "" "PR105550" { xfail *-*-* } } + +E e1 = { }; + +struct F { + bool b = (A{}, true); +}; + +constexpr F f; + +void +g (B b2 = B{}, C c2 = C{}, D d2 = D{}, E e2 = E{}) +{ +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C new file mode 100644 index 00000000000..fc27a2cdac7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C @@ -0,0 +1,138 @@ +// PR c++/100252 +// { dg-do run { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + int x; + int y = x; + const A* p = this; +}; + +struct B { + int x = 42; + A a = A{x}; +}; + +constexpr B b; +SA(b.a.p == &b.a); +SA(b.x == 42); +B b2 = { }; +B b3 = { 42 }; + +struct C { + int x = 42; + B b = B{x}; +}; + +constexpr C c; +C c2; +C c3; + +struct D { + int x = 42; + A a = (true, A{x}); +}; + +constexpr D d; +SA(d.a.p == &d.a); +SA(d.x == 42); +D d2 = { }; +D d3 = { 42 }; + +struct E { + int x = 42; + A a = (A{x}); +}; + +constexpr E e; +SA(e.a.p == &e.a); +SA(e.x == 42); +E e2 = { }; +E e3 = { 42 }; + +struct F { + int x = 42; + A a = true ? A{x} : A{x}; +}; + +// FIXME: Doesn't work due to PR105550. +//constexpr F f; +//SA (f.a.p == &f.a); +SA (e.x == 42); +F f2 = { }; +F f3 = { 42 }; + +static void +test_b (B b4 = B{}, B b5 = B{ 42 }) +{ + if (b2.x != 42 || b2.a.x != 42 || b2.a.y != b2.a.x) + __builtin_abort (); + if (b3.x != 42 || b3.a.x != 42 || b3.a.y != b3.a.x) + __builtin_abort (); + if (b4.x != 42 || b4.a.x != 42 || b4.a.y != b4.a.x) + __builtin_abort (); + if (b5.x != 42 || b5.a.x != 42 || b5.a.y != b5.a.x) + __builtin_abort (); +} + +static void +test_c (C c4 = C{}, C c5 = C{ 42 }) +{ + if (c2.b.x != 42 || c2.b.a.x != 42 || c2.b.a.y != c2.b.a.x) + __builtin_abort (); + if (c3.b.x != 42 || c3.b.a.x != 42 || c3.b.a.y != c3.b.a.x) + __builtin_abort (); + if (c4.b.x != 42 || c4.b.a.x != 42 || c4.b.a.y != c4.b.a.x) + __builtin_abort (); + if (c5.b.x != 42 || c5.b.a.x != 42 || c5.b.a.y != c5.b.a.x) + __builtin_abort (); +} + +static void +test_d (D d4 = D{}, D d5 = D{ 42 }) +{ + if (d2.x != 42 || d2.a.x != 42 || d2.a.y != d2.a.x) + __builtin_abort (); + if (d3.x != 42 || d3.a.x != 42 || d3.a.y != d3.a.x) + __builtin_abort (); + if (d4.x != 42 || d4.a.x != 42 || d4.a.y != d4.a.x) + __builtin_abort (); + if (d5.x != 42 || d5.a.x != 42 || d5.a.y != d5.a.x) + __builtin_abort (); +} + +static void +test_e (E e4 = E{}, E e5 = E{ 42 }) +{ + if (e2.x != 42 || e2.a.x != 42 || e2.a.y != e2.a.x) + __builtin_abort (); + if (e3.x != 42 || e3.a.x != 42 || e3.a.y != e3.a.x) + __builtin_abort (); + if (e4.x != 42 || e4.a.x != 42 || e4.a.y != e4.a.x) + __builtin_abort (); + if (e5.x != 42 || e5.a.x != 42 || e5.a.y != e5.a.x) + __builtin_abort (); +} + +static void +test_f (F f4 = F{}, F f5 = F{ 42 }) +{ + if (f2.x != 42 || f2.a.x != 42 || f2.a.y != f2.a.x) + __builtin_abort (); + if (f3.x != 42 || f3.a.x != 42 || f3.a.y != f3.a.x) + __builtin_abort (); + if (f4.x != 42 || f4.a.x != 42 || f4.a.y != f4.a.x) + __builtin_abort (); + if (f5.x != 42 || f5.a.x != 42 || f5.a.y != f5.a.x) + __builtin_abort (); +} +int +main () +{ + test_b (); + test_c (); + test_d (); + test_e (); + test_f (); +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C new file mode 100644 index 00000000000..567b8ee96d9 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C @@ -0,0 +1,56 @@ +// PR c++/100252 +// { dg-do compile { target c++14 } } + +struct B { }; + +struct A { + int x; + int y = x; + constexpr operator B() { return B{}; } +}; + +struct C { + int x = 42; + B b = A{x}; +}; + +C c1 = {}; +C c2 = { 42 }; +constexpr C c3 = {}; +constexpr C c4 = { 42 }; + +struct D { + int x = 42; + B b = (true, A{x}); +}; + +D d1 = {}; +D d2 = { 42 }; +constexpr D d3 = {}; +constexpr D d4 = { 42 }; + +struct E { + int x = 42; + B b = (A{x}); +}; + +E e1 = {}; +E e2 = { 42 }; +constexpr E e3 = {}; +constexpr E e4 = { 42 }; + +struct F { + int x = 42; + B b = (A{x}); +}; + +F f1 = {}; +F f2 = { 42 }; +constexpr F f3 = {}; +constexpr F f4 = { 42 }; + +void +g (C c5 = C{}, C c6 = C{ 42 }, D d5 = D{}, D d6 = D{ 42 }, + E e5 = E{}, E e6 = E{ 42 }, F f5 = F{}, F f6 = F{ 42 }) +{ +} diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C new file mode 100644 index 00000000000..f4892e3379b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C @@ -0,0 +1,28 @@ +// PR c++/100252 +// { dg-do compile { target c++14 } } + +#define SA(X) static_assert ((X),#X) + +struct A { + const A* p = this; +}; + +struct B { + A a = (A{}, A{}); +}; + +constexpr B b; +SA(b.a.p == &b.a); + +struct C { + int x; + int y = x; +}; + +struct D { + int x = 0; + int y = (C{x}.y, C{x}.y); +}; + +constexpr D d = { }; +D d2 = {}; base-commit: 682e587f1021241758f7dfe0b22651008622a312 -- 2.36.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-16 15:36 ` [PATCH v5] " Marek Polacek @ 2022-05-24 11:40 ` Marek Polacek 2022-05-24 12:36 ` Jason Merrill 1 sibling, 0 replies; 17+ messages in thread From: Marek Polacek @ 2022-05-24 11:40 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek Ping. On Mon, May 16, 2022 at 11:36:27AM -0400, Marek Polacek wrote: > On Sat, May 14, 2022 at 11:13:28PM -0400, Jason Merrill wrote: > > On 5/13/22 19:41, Marek Polacek wrote: > > > --- a/gcc/cp/typeck2.cc > > > +++ b/gcc/cp/typeck2.cc > > > @@ -1371,6 +1371,70 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) > > > return digest_init_r (type, init, 0, flags, complain); > > > } > > > +/* Return true if a prvalue is used as an initializer rather than for > > > + temporary materialization. For instance: > > > > I might say "if SUBOB initializes the same object as FULL_EXPR"; the > > full-expression could still end up initializing a temporary. > > Fixed. > > > > + A a = A{}; // initializer > > > + A a = (A{}); // initializer > > > + A a = (1, A{}); // initializer > > > + A a = true ? A{} : A{}; // initializer > > > + auto x = A{}.x; // temporary materialization > > > + auto x = foo(A{}); // temporary materialization > > > + > > > + FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ > > > + > > > +static bool > > > +potential_prvalue_result_of (tree subob, tree full_expr) > > > +{ > > > + if (subob == full_expr) > > > + return true; > > > + else if (TREE_CODE (full_expr) == TARGET_EXPR) > > > + { > > > + tree init = TARGET_EXPR_INITIAL (full_expr); > > > + if (TREE_CODE (init) == COND_EXPR) > > > + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)) > > > + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 2))); > > > + else if (TREE_CODE (init) == COMPOUND_EXPR) > > > + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 0)) > > > > We shouldn't recurse into the LHS of the comma, only the RHS. > > Fixed. > > > > + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 1))); > > > + /* ??? I don't know if this can be hit. If so, look inside the ( ) > > > + instead of the assert. */ > > > + else if (TREE_CODE (init) == PAREN_EXPR) > > > + gcc_checking_assert (false); > > > > It seems trivial enough to recurse after the assert, in case it does happen > > in the wild. > > OK, adjusted. Thanks! > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > Consider > > struct A { > int x; > int y = x; > }; > > struct B { > int x = 0; > int y = A{x}.y; // #1 > }; > > where for #1 we end up with > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > have any object to refer to yet. After digesting, we have > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > Then we get to > > B b = {}; > > and call store_init_value, which digests the {}, which produces > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > lookup_placeholder in constexpr won't find an object to replace the > PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we > cannot search outward from D.2395 to find 'b'. > > The call to replace_placeholders in store_init_value will not do anything: > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > stays even though now is the perfect time to replace it because we have an > object for it: 'b'. > > Later, in cp_gimplify_init_expr the *expr_p is > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > has a different type. > > My idea was to replace <P_E struct A> with D.2384 after creating the > TARGET_EXPR because that means we have an object we can refer to. > Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have > a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to > replace <P_E struct B> with 'b', and we should be good to go. We must > be careful not to break guaranteed copy elision, so this replacement > happens in digest_nsdmi_init where we can see the whole initializer, > and avoid replacing any placeholders in TARGET_EXPRs used in the context > of initialization/copy elision. This is achieved via the new function > called potential_prvalue_result_of. > > While fixing this problem, I found PR105550, thus the FIXMEs in the > tests. > > PR c++/100252 > > gcc/cp/ChangeLog: > > * typeck2.cc (potential_prvalue_result_of): New. > (replace_placeholders_for_class_temp_r): New. > (digest_nsdmi_init): Call it. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/nsdmi-aggr14.C: New test. > * g++.dg/cpp1y/nsdmi-aggr15.C: New test. > * g++.dg/cpp1y/nsdmi-aggr16.C: New test. > * g++.dg/cpp1y/nsdmi-aggr17.C: New test. > * g++.dg/cpp1y/nsdmi-aggr18.C: New test. > * g++.dg/cpp1y/nsdmi-aggr19.C: New test. > --- > gcc/cp/typeck2.cc | 91 ++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 131 ++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 80 +++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 58 +++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 138 ++++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 56 +++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C | 28 +++++ > 7 files changed, 582 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > index 1d92310edd0..1a96be3d412 100644 > --- a/gcc/cp/typeck2.cc > +++ b/gcc/cp/typeck2.cc > @@ -1371,6 +1371,71 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) > return digest_init_r (type, init, 0, flags, complain); > } > > +/* Return true if SUBOB initializes the same object as FULL_EXPR. > + For instance: > + > + A a = A{}; // initializer > + A a = (A{}); // initializer > + A a = (1, A{}); // initializer > + A a = true ? A{} : A{}; // initializer > + auto x = A{}.x; // temporary materialization > + auto x = foo(A{}); // temporary materialization > + > + FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ > + > +static bool > +potential_prvalue_result_of (tree subob, tree full_expr) > +{ > + if (subob == full_expr) > + return true; > + else if (TREE_CODE (full_expr) == TARGET_EXPR) > + { > + tree init = TARGET_EXPR_INITIAL (full_expr); > + if (TREE_CODE (init) == COND_EXPR) > + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)) > + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 2))); > + else if (TREE_CODE (init) == COMPOUND_EXPR) > + return potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)); > + /* ??? I don't know if this can be hit. */ > + else if (TREE_CODE (init) == PAREN_EXPR) > + { > + gcc_checking_assert (false); > + return potential_prvalue_result_of (subob, TREE_OPERAND (init, 0)); > + } > + } > + return false; > +} > + > +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used > + in the context of guaranteed copy elision). */ > + > +static tree > +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) > +{ > + tree t = *tp; > + tree full_expr = *static_cast<tree *>(data); > + > + /* We're looking for a TARGET_EXPR nested in the whole expression. */ > + if (TREE_CODE (t) == TARGET_EXPR > + && !potential_prvalue_result_of (t, full_expr)) > + { > + tree init = TARGET_EXPR_INITIAL (t); > + while (TREE_CODE (init) == COMPOUND_EXPR) > + init = TREE_OPERAND (init, 1); > + if (TREE_CODE (init) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) > + { > + tree obj = TARGET_EXPR_SLOT (t); > + replace_placeholders (init, obj); > + /* We should have dealt with all PLACEHOLDER_EXPRs. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; > + gcc_checking_assert (!find_placeholders (init)); > + } > + } > + > + return NULL_TREE; > +} > + > /* Process the initializer INIT for an NSDMI DECL (a FIELD_DECL). */ > tree > digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) > @@ -1390,6 +1455,32 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) > && CP_AGGREGATE_TYPE_P (type)) > init = reshape_init (type, init, complain); > init = digest_init_flags (type, init, flags, complain); > + > + /* We may have temporary materialization in a NSDMI, if the initializer > + has something like A{} in it. Digesting the {} could have introduced > + a PLACEHOLDER_EXPR referring to A. Now that we've got a TARGET_EXPR, > + we have an object we can refer to. The reason we bother doing this > + here is for code like > + > + struct A { > + int x; > + int y = x; > + }; > + > + struct B { > + int x = 0; > + int y = A{x}.y; // #1 > + }; > + > + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs for > + different types on the same level in a {} when lookup_placeholder > + wouldn't find a named object for the PLACEHOLDER_EXPR for A. Note, > + temporary materialization does not occur when initializing an object > + from a prvalue of the same type, therefore we must not replace the > + placeholder with a temporary object so that it can be elided. */ > + cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init, > + nullptr); > + > return init; > } > \f > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > new file mode 100644 > index 00000000000..28b908a0a1a > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > @@ -0,0 +1,131 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +constexpr B csb1 = { }; > +SA(csb1.x == 0 && csb1.y == csb1.x); > +constexpr B csb2 = { 1 }; > +SA(csb2.x == 1 && csb2.y == csb2.x); > +constexpr B csb3 = { 1, 2 }; > +SA(csb3.x == 1 && csb3.y == 2); > + > +B sb1 = { }; > +B sb2 = { 1 }; > +B sb3 = { 1, 2}; > + > +struct C { > + int x = 0; > + int y = (true, A{x}.y) + (A{x}.y, 0); > +}; > + > +constexpr C csc1 = { }; > +SA(csc1.x == 0 && csc1.y == csc1.x); > +constexpr C csc2 = { 1 }; > +SA(csc2.x == 1 && csc2.y == csc2.x); > +constexpr C csc3 = { 1, 2 }; > +SA(csc3.x == 1 && csc3.y == 2); > + > +C sc1 = { }; > +C sc2 = { 1 }; > +C sc3 = { 1, 2}; > + > +struct D { > + int x = 0; > + int y = (A{x}.y); > +}; > + > +constexpr D csd1 = { }; > +SA(csd1.x == 0 && csd1.y == csd1.x); > +constexpr D csd2 = { 1 }; > +SA(csd2.x == 1 && csd2.y == csd2.x); > +constexpr D csd3 = { 1, 2 }; > +SA(csd3.x == 1 && csd3.y == 2); > + > +D sd1 = { }; > +D sd2 = { 1 }; > +D sd3 = { 1, 2}; > + > +struct E { > + int x = 0; > + int y = x ? A{x}.y : A{x}.y; > +}; > + > +constexpr E cse1 = { }; > +SA(cse1.x == 0 && cse1.y == cse1.x); > +constexpr E cse2 = { 1 }; > +SA(cse2.x == 1 && cse2.y == cse2.x); > +constexpr E cse3 = { 1, 2 }; > +SA(cse3.x == 1 && cse3.y == 2); > + > +E se1 = { }; > +E se2 = { 1 }; > +E se3 = { 1, 2}; > + > +int > +main () > +{ > + if (sb1.x != 0 || sb1.x != sb1.y) > + __builtin_abort(); > + if (sb2.x != 1 || sb2.x != sb2.y) > + __builtin_abort(); > + if (sb3.x != 1 || sb3.y != 2) > + __builtin_abort(); > + > + if (sc1.x != 0 || sc1.x != sc1.y) > + __builtin_abort(); > + if (sc2.x != 1 || sc2.x != sc2.y) > + __builtin_abort(); > + if (sc3.x != 1 || sc3.y != 2) > + __builtin_abort(); > + > + B b1 = { }; > + B b2 = { 1 }; > + B b3 = { 1, 2}; > + if (b1.x != 0 || b1.x != b1.y) > + __builtin_abort(); > + if (b2.x != 1 || b2.x != b2.y) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > + > + C c1 = { }; > + C c2 = { 1 }; > + C c3 = { 1, 2}; > + if (c1.x != 0 || c1.x != c1.y) > + __builtin_abort(); > + if (c2.x != 1 || c2.x != c2.y) > + __builtin_abort(); > + if (c3.x != 1 || c3.y != 2) > + __builtin_abort(); > + > + D d1 = { }; > + D d2 = { 1 }; > + D d3 = { 1, 2}; > + if (d1.x != 0 || d1.x != d1.y) > + __builtin_abort(); > + if (d2.x != 1 || d2.x != d2.y) > + __builtin_abort(); > + if (d3.x != 1 || d3.y != 2) > + __builtin_abort(); > + > + E e1 = { }; > + E e2 = { 1 }; > + E e3 = { 1, 2}; > + if (e1.x != 0 || e1.x != e1.y) > + __builtin_abort(); > + if (e2.x != 1 || e2.x != e2.y) > + __builtin_abort(); > + if (e3.x != 1 || e3.y != 2) > + __builtin_abort(); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > new file mode 100644 > index 00000000000..d091d693042 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > @@ -0,0 +1,80 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +static void > +test_b (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) > +{ > + if (b1.x != 0 || b1.y != b1.x) > + __builtin_abort(); > + if (b2.x != 1 || b2.y != b2.x) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > + > +struct C { > + int x = 0; > + int y = (true, A{x}.y) + (A{x}.y, 0); > +}; > + > +static void > +test_c (C c1 = C{}, C c2 = C{1}, C c3 = C{1, 2}) > +{ > + if (c1.x != 0 || c1.y != c1.x) > + __builtin_abort(); > + if (c2.x != 1 || c2.y != c2.x) > + __builtin_abort(); > + if (c3.x != 1 || c3.y != 2) > + __builtin_abort(); > +} > + > +struct D { > + int x = 0; > + int y = (A{x}.y); > +}; > + > +static void > +test_d (D d1 = D{}, D d2 = D{1}, D d3 = D{1, 2}) > +{ > + if (d1.x != 0 || d1.y != d1.x) > + __builtin_abort(); > + if (d2.x != 1 || d2.y != d2.x) > + __builtin_abort(); > + if (d3.x != 1 || d3.y != 2) > + __builtin_abort(); > +} > + > +struct E { > + int x = 0; > + int y = x ? A{x}.y : A{x}.y; > +}; > + > +static void > +test_e (E e1 = E{}, E e2 = E{1}, E e3 = E{1, 2}) > +{ > + if (e1.x != 0 || e1.y != e1.x) > + __builtin_abort(); > + if (e2.x != 1 || e2.y != e2.x) > + __builtin_abort(); > + if (e3.x != 1 || e3.y != 2) > + __builtin_abort(); > +} > + > +int > +main () > +{ > + test_b (); > + test_c (); > + test_d (); > + test_e (); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > new file mode 100644 > index 00000000000..dc6492c1b0b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > @@ -0,0 +1,58 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + const A* p = this; > +}; > + > +struct B { > + A a = A{}; > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > +B b1 = { }; > + > +struct C { > + A a = (true, A{}); > +}; > + > +constexpr C c; > +SA(c.a.p == &c.a); > +C c1 = { }; > + > +struct D { > + A a = (A{}); > +}; > + > +constexpr D d; > +SA(d.a.p == &d.a); > +D d1 = { }; > + > +static constexpr A global_a; > + > +struct E { > + A a = true ? A{} : A{}; > + A b = true ? global_a : (false ? A{} : A{}); > + A c = true ? (false ? A{} : A{}) : global_a; > + A d = true ? (false ? A{} : A{}) : (false ? A{} : A{}); > +}; > + > +// FIXME: When fixing this, also fix nsdmi-aggr17.C. > +constexpr E e; // { dg-bogus "" "PR105550" { xfail *-*-* } } > +SA (e.a.p == &e.a); // { dg-bogus "" "PR105550" { xfail *-*-* } } > + > +E e1 = { }; > + > +struct F { > + bool b = (A{}, true); > +}; > + > +constexpr F f; > + > +void > +g (B b2 = B{}, C c2 = C{}, D d2 = D{}, E e2 = E{}) > +{ > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > new file mode 100644 > index 00000000000..fc27a2cdac7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > @@ -0,0 +1,138 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > + const A* p = this; > +}; > + > +struct B { > + int x = 42; > + A a = A{x}; > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > +SA(b.x == 42); > +B b2 = { }; > +B b3 = { 42 }; > + > +struct C { > + int x = 42; > + B b = B{x}; > +}; > + > +constexpr C c; > +C c2; > +C c3; > + > +struct D { > + int x = 42; > + A a = (true, A{x}); > +}; > + > +constexpr D d; > +SA(d.a.p == &d.a); > +SA(d.x == 42); > +D d2 = { }; > +D d3 = { 42 }; > + > +struct E { > + int x = 42; > + A a = (A{x}); > +}; > + > +constexpr E e; > +SA(e.a.p == &e.a); > +SA(e.x == 42); > +E e2 = { }; > +E e3 = { 42 }; > + > +struct F { > + int x = 42; > + A a = true ? A{x} : A{x}; > +}; > + > +// FIXME: Doesn't work due to PR105550. > +//constexpr F f; > +//SA (f.a.p == &f.a); > +SA (e.x == 42); > +F f2 = { }; > +F f3 = { 42 }; > + > +static void > +test_b (B b4 = B{}, B b5 = B{ 42 }) > +{ > + if (b2.x != 42 || b2.a.x != 42 || b2.a.y != b2.a.x) > + __builtin_abort (); > + if (b3.x != 42 || b3.a.x != 42 || b3.a.y != b3.a.x) > + __builtin_abort (); > + if (b4.x != 42 || b4.a.x != 42 || b4.a.y != b4.a.x) > + __builtin_abort (); > + if (b5.x != 42 || b5.a.x != 42 || b5.a.y != b5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_c (C c4 = C{}, C c5 = C{ 42 }) > +{ > + if (c2.b.x != 42 || c2.b.a.x != 42 || c2.b.a.y != c2.b.a.x) > + __builtin_abort (); > + if (c3.b.x != 42 || c3.b.a.x != 42 || c3.b.a.y != c3.b.a.x) > + __builtin_abort (); > + if (c4.b.x != 42 || c4.b.a.x != 42 || c4.b.a.y != c4.b.a.x) > + __builtin_abort (); > + if (c5.b.x != 42 || c5.b.a.x != 42 || c5.b.a.y != c5.b.a.x) > + __builtin_abort (); > +} > + > +static void > +test_d (D d4 = D{}, D d5 = D{ 42 }) > +{ > + if (d2.x != 42 || d2.a.x != 42 || d2.a.y != d2.a.x) > + __builtin_abort (); > + if (d3.x != 42 || d3.a.x != 42 || d3.a.y != d3.a.x) > + __builtin_abort (); > + if (d4.x != 42 || d4.a.x != 42 || d4.a.y != d4.a.x) > + __builtin_abort (); > + if (d5.x != 42 || d5.a.x != 42 || d5.a.y != d5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_e (E e4 = E{}, E e5 = E{ 42 }) > +{ > + if (e2.x != 42 || e2.a.x != 42 || e2.a.y != e2.a.x) > + __builtin_abort (); > + if (e3.x != 42 || e3.a.x != 42 || e3.a.y != e3.a.x) > + __builtin_abort (); > + if (e4.x != 42 || e4.a.x != 42 || e4.a.y != e4.a.x) > + __builtin_abort (); > + if (e5.x != 42 || e5.a.x != 42 || e5.a.y != e5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_f (F f4 = F{}, F f5 = F{ 42 }) > +{ > + if (f2.x != 42 || f2.a.x != 42 || f2.a.y != f2.a.x) > + __builtin_abort (); > + if (f3.x != 42 || f3.a.x != 42 || f3.a.y != f3.a.x) > + __builtin_abort (); > + if (f4.x != 42 || f4.a.x != 42 || f4.a.y != f4.a.x) > + __builtin_abort (); > + if (f5.x != 42 || f5.a.x != 42 || f5.a.y != f5.a.x) > + __builtin_abort (); > +} > +int > +main () > +{ > + test_b (); > + test_c (); > + test_d (); > + test_e (); > + test_f (); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > new file mode 100644 > index 00000000000..567b8ee96d9 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > @@ -0,0 +1,56 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +struct B { }; > + > +struct A { > + int x; > + int y = x; > + constexpr operator B() { return B{}; } > +}; > + > +struct C { > + int x = 42; > + B b = A{x}; > +}; > + > +C c1 = {}; > +C c2 = { 42 }; > +constexpr C c3 = {}; > +constexpr C c4 = { 42 }; > + > +struct D { > + int x = 42; > + B b = (true, A{x}); > +}; > + > +D d1 = {}; > +D d2 = { 42 }; > +constexpr D d3 = {}; > +constexpr D d4 = { 42 }; > + > +struct E { > + int x = 42; > + B b = (A{x}); > +}; > + > +E e1 = {}; > +E e2 = { 42 }; > +constexpr E e3 = {}; > +constexpr E e4 = { 42 }; > + > +struct F { > + int x = 42; > + B b = (A{x}); > +}; > + > +F f1 = {}; > +F f2 = { 42 }; > +constexpr F f3 = {}; > +constexpr F f4 = { 42 }; > + > +void > +g (C c5 = C{}, C c6 = C{ 42 }, D d5 = D{}, D d6 = D{ 42 }, > + E e5 = E{}, E e6 = E{ 42 }, F f5 = F{}, F f6 = F{ 42 }) > +{ > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > new file mode 100644 > index 00000000000..f4892e3379b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > @@ -0,0 +1,28 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + const A* p = this; > +}; > + > +struct B { > + A a = (A{}, A{}); > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > + > +struct C { > + int x; > + int y = x; > +}; > + > +struct D { > + int x = 0; > + int y = (C{x}.y, C{x}.y); > +}; > + > +constexpr D d = { }; > +D d2 = {}; > > base-commit: 682e587f1021241758f7dfe0b22651008622a312 > -- > 2.36.1 > Marek ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-16 15:36 ` [PATCH v5] " Marek Polacek 2022-05-24 11:40 ` Marek Polacek @ 2022-05-24 12:36 ` Jason Merrill 2022-05-24 13:55 ` Marek Polacek 1 sibling, 1 reply; 17+ messages in thread From: Jason Merrill @ 2022-05-24 12:36 UTC (permalink / raw) To: Marek Polacek; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On 5/16/22 11:36, Marek Polacek wrote: > On Sat, May 14, 2022 at 11:13:28PM -0400, Jason Merrill wrote: >> On 5/13/22 19:41, Marek Polacek wrote: >>> --- a/gcc/cp/typeck2.cc >>> +++ b/gcc/cp/typeck2.cc >>> @@ -1371,6 +1371,70 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) >>> return digest_init_r (type, init, 0, flags, complain); >>> } >>> +/* Return true if a prvalue is used as an initializer rather than for >>> + temporary materialization. For instance: >> >> I might say "if SUBOB initializes the same object as FULL_EXPR"; the >> full-expression could still end up initializing a temporary. > > Fixed. > >>> + A a = A{}; // initializer >>> + A a = (A{}); // initializer >>> + A a = (1, A{}); // initializer >>> + A a = true ? A{} : A{}; // initializer >>> + auto x = A{}.x; // temporary materialization >>> + auto x = foo(A{}); // temporary materialization >>> + >>> + FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ >>> + >>> +static bool >>> +potential_prvalue_result_of (tree subob, tree full_expr) >>> +{ >>> + if (subob == full_expr) >>> + return true; >>> + else if (TREE_CODE (full_expr) == TARGET_EXPR) >>> + { >>> + tree init = TARGET_EXPR_INITIAL (full_expr); >>> + if (TREE_CODE (init) == COND_EXPR) >>> + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)) >>> + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 2))); >>> + else if (TREE_CODE (init) == COMPOUND_EXPR) >>> + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 0)) >> >> We shouldn't recurse into the LHS of the comma, only the RHS. > > Fixed. > >>> + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 1))); >>> + /* ??? I don't know if this can be hit. If so, look inside the ( ) >>> + instead of the assert. */ >>> + else if (TREE_CODE (init) == PAREN_EXPR) >>> + gcc_checking_assert (false); >> >> It seems trivial enough to recurse after the assert, in case it does happen >> in the wild. > > OK, adjusted. Thanks! > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > Consider > > struct A { > int x; > int y = x; > }; > > struct B { > int x = 0; > int y = A{x}.y; // #1 > }; > > where for #1 we end up with > > {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > that is, two PLACEHOLDER_EXPRs for different types on the same level in > a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it. > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal > on type=A, compound_literal={((struct B *) this)->x}. When digesting this > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't > have any object to refer to yet. After digesting, we have > > {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor > CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns > > TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}> > > Then we get to > > B b = {}; > > and call store_init_value, which digests the {}, which produces > > {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y} > > lookup_placeholder in constexpr won't find an object to replace the > PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we > cannot search outward from D.2395 to find 'b'. > > The call to replace_placeholders in store_init_value will not do anything: > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B> > stays even though now is the perfect time to replace it because we have an > object for it: 'b'. > > Later, in cp_gimplify_init_expr the *expr_p is > > D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which > has a different type. > > My idea was to replace <P_E struct A> with D.2384 after creating the > TARGET_EXPR because that means we have an object we can refer to. > Then clear CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have > a PLACEHOLDER_EXPR in the {}. Then store_init_value will be able to > replace <P_E struct B> with 'b', and we should be good to go. We must > be careful not to break guaranteed copy elision, so this replacement > happens in digest_nsdmi_init where we can see the whole initializer, > and avoid replacing any placeholders in TARGET_EXPRs used in the context > of initialization/copy elision. This is achieved via the new function > called potential_prvalue_result_of. > > While fixing this problem, I found PR105550, thus the FIXMEs in the > tests. > > PR c++/100252 > > gcc/cp/ChangeLog: > > * typeck2.cc (potential_prvalue_result_of): New. > (replace_placeholders_for_class_temp_r): New. > (digest_nsdmi_init): Call it. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp1y/nsdmi-aggr14.C: New test. > * g++.dg/cpp1y/nsdmi-aggr15.C: New test. > * g++.dg/cpp1y/nsdmi-aggr16.C: New test. > * g++.dg/cpp1y/nsdmi-aggr17.C: New test. > * g++.dg/cpp1y/nsdmi-aggr18.C: New test. > * g++.dg/cpp1y/nsdmi-aggr19.C: New test. > --- > gcc/cp/typeck2.cc | 91 ++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 131 ++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 80 +++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 58 +++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 138 ++++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 56 +++++++++ > gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C | 28 +++++ > 7 files changed, 582 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > index 1d92310edd0..1a96be3d412 100644 > --- a/gcc/cp/typeck2.cc > +++ b/gcc/cp/typeck2.cc > @@ -1371,6 +1371,71 @@ digest_init_flags (tree type, tree init, int flags, tsubst_flags_t complain) > return digest_init_r (type, init, 0, flags, complain); > } > > +/* Return true if SUBOB initializes the same object as FULL_EXPR. > + For instance: > + > + A a = A{}; // initializer > + A a = (A{}); // initializer > + A a = (1, A{}); // initializer > + A a = true ? A{} : A{}; // initializer > + auto x = A{}.x; // temporary materialization > + auto x = foo(A{}); // temporary materialization > + > + FULL_EXPR is the whole expression, SUBOB is its TARGET_EXPR subobject. */ > + > +static bool > +potential_prvalue_result_of (tree subob, tree full_expr) > +{ > + if (subob == full_expr) > + return true; > + else if (TREE_CODE (full_expr) == TARGET_EXPR) > + { > + tree init = TARGET_EXPR_INITIAL (full_expr); > + if (TREE_CODE (init) == COND_EXPR) > + return (potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)) > + || potential_prvalue_result_of (subob, TREE_OPERAND (init, 2))); > + else if (TREE_CODE (init) == COMPOUND_EXPR) > + return potential_prvalue_result_of (subob, TREE_OPERAND (init, 1)); > + /* ??? I don't know if this can be hit. */ > + else if (TREE_CODE (init) == PAREN_EXPR) > + { > + gcc_checking_assert (false); > + return potential_prvalue_result_of (subob, TREE_OPERAND (init, 0)); > + } > + } > + return false; > +} > + > +/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used > + in the context of guaranteed copy elision). */ > + > +static tree > +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) > +{ > + tree t = *tp; > + tree full_expr = *static_cast<tree *>(data); > + > + /* We're looking for a TARGET_EXPR nested in the whole expression. */ > + if (TREE_CODE (t) == TARGET_EXPR > + && !potential_prvalue_result_of (t, full_expr)) > + { > + tree init = TARGET_EXPR_INITIAL (t); > + while (TREE_CODE (init) == COMPOUND_EXPR) > + init = TREE_OPERAND (init, 1); Hmm, how do we get a COMPOUND_EXPR around a CONSTRUCTOR? > + if (TREE_CODE (init) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init)) > + { > + tree obj = TARGET_EXPR_SLOT (t); > + replace_placeholders (init, obj); > + /* We should have dealt with all PLACEHOLDER_EXPRs. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false; > + gcc_checking_assert (!find_placeholders (init)); > + } > + } > + > + return NULL_TREE; > +} > + > /* Process the initializer INIT for an NSDMI DECL (a FIELD_DECL). */ > tree > digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) > @@ -1390,6 +1455,32 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain) > && CP_AGGREGATE_TYPE_P (type)) > init = reshape_init (type, init, complain); > init = digest_init_flags (type, init, flags, complain); > + > + /* We may have temporary materialization in a NSDMI, if the initializer > + has something like A{} in it. Digesting the {} could have introduced > + a PLACEHOLDER_EXPR referring to A. Now that we've got a TARGET_EXPR, > + we have an object we can refer to. The reason we bother doing this > + here is for code like > + > + struct A { > + int x; > + int y = x; > + }; > + > + struct B { > + int x = 0; > + int y = A{x}.y; // #1 > + }; > + > + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs for > + different types on the same level in a {} when lookup_placeholder > + wouldn't find a named object for the PLACEHOLDER_EXPR for A. Note, > + temporary materialization does not occur when initializing an object > + from a prvalue of the same type, therefore we must not replace the > + placeholder with a temporary object so that it can be elided. */ > + cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init, > + nullptr); > + > return init; > } > \f > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > new file mode 100644 > index 00000000000..28b908a0a1a > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C > @@ -0,0 +1,131 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +constexpr B csb1 = { }; > +SA(csb1.x == 0 && csb1.y == csb1.x); > +constexpr B csb2 = { 1 }; > +SA(csb2.x == 1 && csb2.y == csb2.x); > +constexpr B csb3 = { 1, 2 }; > +SA(csb3.x == 1 && csb3.y == 2); > + > +B sb1 = { }; > +B sb2 = { 1 }; > +B sb3 = { 1, 2}; > + > +struct C { > + int x = 0; > + int y = (true, A{x}.y) + (A{x}.y, 0); > +}; > + > +constexpr C csc1 = { }; > +SA(csc1.x == 0 && csc1.y == csc1.x); > +constexpr C csc2 = { 1 }; > +SA(csc2.x == 1 && csc2.y == csc2.x); > +constexpr C csc3 = { 1, 2 }; > +SA(csc3.x == 1 && csc3.y == 2); > + > +C sc1 = { }; > +C sc2 = { 1 }; > +C sc3 = { 1, 2}; > + > +struct D { > + int x = 0; > + int y = (A{x}.y); > +}; > + > +constexpr D csd1 = { }; > +SA(csd1.x == 0 && csd1.y == csd1.x); > +constexpr D csd2 = { 1 }; > +SA(csd2.x == 1 && csd2.y == csd2.x); > +constexpr D csd3 = { 1, 2 }; > +SA(csd3.x == 1 && csd3.y == 2); > + > +D sd1 = { }; > +D sd2 = { 1 }; > +D sd3 = { 1, 2}; > + > +struct E { > + int x = 0; > + int y = x ? A{x}.y : A{x}.y; > +}; > + > +constexpr E cse1 = { }; > +SA(cse1.x == 0 && cse1.y == cse1.x); > +constexpr E cse2 = { 1 }; > +SA(cse2.x == 1 && cse2.y == cse2.x); > +constexpr E cse3 = { 1, 2 }; > +SA(cse3.x == 1 && cse3.y == 2); > + > +E se1 = { }; > +E se2 = { 1 }; > +E se3 = { 1, 2}; > + > +int > +main () > +{ > + if (sb1.x != 0 || sb1.x != sb1.y) > + __builtin_abort(); > + if (sb2.x != 1 || sb2.x != sb2.y) > + __builtin_abort(); > + if (sb3.x != 1 || sb3.y != 2) > + __builtin_abort(); > + > + if (sc1.x != 0 || sc1.x != sc1.y) > + __builtin_abort(); > + if (sc2.x != 1 || sc2.x != sc2.y) > + __builtin_abort(); > + if (sc3.x != 1 || sc3.y != 2) > + __builtin_abort(); > + > + B b1 = { }; > + B b2 = { 1 }; > + B b3 = { 1, 2}; > + if (b1.x != 0 || b1.x != b1.y) > + __builtin_abort(); > + if (b2.x != 1 || b2.x != b2.y) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > + > + C c1 = { }; > + C c2 = { 1 }; > + C c3 = { 1, 2}; > + if (c1.x != 0 || c1.x != c1.y) > + __builtin_abort(); > + if (c2.x != 1 || c2.x != c2.y) > + __builtin_abort(); > + if (c3.x != 1 || c3.y != 2) > + __builtin_abort(); > + > + D d1 = { }; > + D d2 = { 1 }; > + D d3 = { 1, 2}; > + if (d1.x != 0 || d1.x != d1.y) > + __builtin_abort(); > + if (d2.x != 1 || d2.x != d2.y) > + __builtin_abort(); > + if (d3.x != 1 || d3.y != 2) > + __builtin_abort(); > + > + E e1 = { }; > + E e2 = { 1 }; > + E e3 = { 1, 2}; > + if (e1.x != 0 || e1.x != e1.y) > + __builtin_abort(); > + if (e2.x != 1 || e2.x != e2.y) > + __builtin_abort(); > + if (e3.x != 1 || e3.y != 2) > + __builtin_abort(); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > new file mode 100644 > index 00000000000..d091d693042 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C > @@ -0,0 +1,80 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +struct A { > + int x; > + int y = x; > +}; > + > +struct B { > + int x = 0; > + int y = A{x}.y; > +}; > + > +static void > +test_b (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2}) > +{ > + if (b1.x != 0 || b1.y != b1.x) > + __builtin_abort(); > + if (b2.x != 1 || b2.y != b2.x) > + __builtin_abort(); > + if (b3.x != 1 || b3.y != 2) > + __builtin_abort(); > +} > + > +struct C { > + int x = 0; > + int y = (true, A{x}.y) + (A{x}.y, 0); > +}; > + > +static void > +test_c (C c1 = C{}, C c2 = C{1}, C c3 = C{1, 2}) > +{ > + if (c1.x != 0 || c1.y != c1.x) > + __builtin_abort(); > + if (c2.x != 1 || c2.y != c2.x) > + __builtin_abort(); > + if (c3.x != 1 || c3.y != 2) > + __builtin_abort(); > +} > + > +struct D { > + int x = 0; > + int y = (A{x}.y); > +}; > + > +static void > +test_d (D d1 = D{}, D d2 = D{1}, D d3 = D{1, 2}) > +{ > + if (d1.x != 0 || d1.y != d1.x) > + __builtin_abort(); > + if (d2.x != 1 || d2.y != d2.x) > + __builtin_abort(); > + if (d3.x != 1 || d3.y != 2) > + __builtin_abort(); > +} > + > +struct E { > + int x = 0; > + int y = x ? A{x}.y : A{x}.y; > +}; > + > +static void > +test_e (E e1 = E{}, E e2 = E{1}, E e3 = E{1, 2}) > +{ > + if (e1.x != 0 || e1.y != e1.x) > + __builtin_abort(); > + if (e2.x != 1 || e2.y != e2.x) > + __builtin_abort(); > + if (e3.x != 1 || e3.y != 2) > + __builtin_abort(); > +} > + > +int > +main () > +{ > + test_b (); > + test_c (); > + test_d (); > + test_e (); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > new file mode 100644 > index 00000000000..dc6492c1b0b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C > @@ -0,0 +1,58 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + const A* p = this; > +}; > + > +struct B { > + A a = A{}; > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > +B b1 = { }; > + > +struct C { > + A a = (true, A{}); > +}; > + > +constexpr C c; > +SA(c.a.p == &c.a); > +C c1 = { }; > + > +struct D { > + A a = (A{}); > +}; > + > +constexpr D d; > +SA(d.a.p == &d.a); > +D d1 = { }; > + > +static constexpr A global_a; > + > +struct E { > + A a = true ? A{} : A{}; > + A b = true ? global_a : (false ? A{} : A{}); > + A c = true ? (false ? A{} : A{}) : global_a; > + A d = true ? (false ? A{} : A{}) : (false ? A{} : A{}); > +}; > + > +// FIXME: When fixing this, also fix nsdmi-aggr17.C. > +constexpr E e; // { dg-bogus "" "PR105550" { xfail *-*-* } } > +SA (e.a.p == &e.a); // { dg-bogus "" "PR105550" { xfail *-*-* } } > + > +E e1 = { }; > + > +struct F { > + bool b = (A{}, true); > +}; > + > +constexpr F f; > + > +void > +g (B b2 = B{}, C c2 = C{}, D d2 = D{}, E e2 = E{}) > +{ > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > new file mode 100644 > index 00000000000..fc27a2cdac7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C > @@ -0,0 +1,138 @@ > +// PR c++/100252 > +// { dg-do run { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + int x; > + int y = x; > + const A* p = this; > +}; > + > +struct B { > + int x = 42; > + A a = A{x}; > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > +SA(b.x == 42); > +B b2 = { }; > +B b3 = { 42 }; > + > +struct C { > + int x = 42; > + B b = B{x}; > +}; > + > +constexpr C c; > +C c2; > +C c3; > + > +struct D { > + int x = 42; > + A a = (true, A{x}); > +}; > + > +constexpr D d; > +SA(d.a.p == &d.a); > +SA(d.x == 42); > +D d2 = { }; > +D d3 = { 42 }; > + > +struct E { > + int x = 42; > + A a = (A{x}); > +}; > + > +constexpr E e; > +SA(e.a.p == &e.a); > +SA(e.x == 42); > +E e2 = { }; > +E e3 = { 42 }; > + > +struct F { > + int x = 42; > + A a = true ? A{x} : A{x}; > +}; > + > +// FIXME: Doesn't work due to PR105550. > +//constexpr F f; > +//SA (f.a.p == &f.a); > +SA (e.x == 42); > +F f2 = { }; > +F f3 = { 42 }; > + > +static void > +test_b (B b4 = B{}, B b5 = B{ 42 }) > +{ > + if (b2.x != 42 || b2.a.x != 42 || b2.a.y != b2.a.x) > + __builtin_abort (); > + if (b3.x != 42 || b3.a.x != 42 || b3.a.y != b3.a.x) > + __builtin_abort (); > + if (b4.x != 42 || b4.a.x != 42 || b4.a.y != b4.a.x) > + __builtin_abort (); > + if (b5.x != 42 || b5.a.x != 42 || b5.a.y != b5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_c (C c4 = C{}, C c5 = C{ 42 }) > +{ > + if (c2.b.x != 42 || c2.b.a.x != 42 || c2.b.a.y != c2.b.a.x) > + __builtin_abort (); > + if (c3.b.x != 42 || c3.b.a.x != 42 || c3.b.a.y != c3.b.a.x) > + __builtin_abort (); > + if (c4.b.x != 42 || c4.b.a.x != 42 || c4.b.a.y != c4.b.a.x) > + __builtin_abort (); > + if (c5.b.x != 42 || c5.b.a.x != 42 || c5.b.a.y != c5.b.a.x) > + __builtin_abort (); > +} > + > +static void > +test_d (D d4 = D{}, D d5 = D{ 42 }) > +{ > + if (d2.x != 42 || d2.a.x != 42 || d2.a.y != d2.a.x) > + __builtin_abort (); > + if (d3.x != 42 || d3.a.x != 42 || d3.a.y != d3.a.x) > + __builtin_abort (); > + if (d4.x != 42 || d4.a.x != 42 || d4.a.y != d4.a.x) > + __builtin_abort (); > + if (d5.x != 42 || d5.a.x != 42 || d5.a.y != d5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_e (E e4 = E{}, E e5 = E{ 42 }) > +{ > + if (e2.x != 42 || e2.a.x != 42 || e2.a.y != e2.a.x) > + __builtin_abort (); > + if (e3.x != 42 || e3.a.x != 42 || e3.a.y != e3.a.x) > + __builtin_abort (); > + if (e4.x != 42 || e4.a.x != 42 || e4.a.y != e4.a.x) > + __builtin_abort (); > + if (e5.x != 42 || e5.a.x != 42 || e5.a.y != e5.a.x) > + __builtin_abort (); > +} > + > +static void > +test_f (F f4 = F{}, F f5 = F{ 42 }) > +{ > + if (f2.x != 42 || f2.a.x != 42 || f2.a.y != f2.a.x) > + __builtin_abort (); > + if (f3.x != 42 || f3.a.x != 42 || f3.a.y != f3.a.x) > + __builtin_abort (); > + if (f4.x != 42 || f4.a.x != 42 || f4.a.y != f4.a.x) > + __builtin_abort (); > + if (f5.x != 42 || f5.a.x != 42 || f5.a.y != f5.a.x) > + __builtin_abort (); > +} > +int > +main () > +{ > + test_b (); > + test_c (); > + test_d (); > + test_e (); > + test_f (); > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > new file mode 100644 > index 00000000000..567b8ee96d9 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C > @@ -0,0 +1,56 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +struct B { }; > + > +struct A { > + int x; > + int y = x; > + constexpr operator B() { return B{}; } > +}; > + > +struct C { > + int x = 42; > + B b = A{x}; > +}; > + > +C c1 = {}; > +C c2 = { 42 }; > +constexpr C c3 = {}; > +constexpr C c4 = { 42 }; > + > +struct D { > + int x = 42; > + B b = (true, A{x}); > +}; > + > +D d1 = {}; > +D d2 = { 42 }; > +constexpr D d3 = {}; > +constexpr D d4 = { 42 }; > + > +struct E { > + int x = 42; > + B b = (A{x}); > +}; > + > +E e1 = {}; > +E e2 = { 42 }; > +constexpr E e3 = {}; > +constexpr E e4 = { 42 }; > + > +struct F { > + int x = 42; > + B b = (A{x}); > +}; > + > +F f1 = {}; > +F f2 = { 42 }; > +constexpr F f3 = {}; > +constexpr F f4 = { 42 }; > + > +void > +g (C c5 = C{}, C c6 = C{ 42 }, D d5 = D{}, D d6 = D{ 42 }, > + E e5 = E{}, E e6 = E{ 42 }, F f5 = F{}, F f6 = F{ 42 }) > +{ > +} > diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > new file mode 100644 > index 00000000000..f4892e3379b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr19.C > @@ -0,0 +1,28 @@ > +// PR c++/100252 > +// { dg-do compile { target c++14 } } > + > +#define SA(X) static_assert ((X),#X) > + > +struct A { > + const A* p = this; > +}; > + > +struct B { > + A a = (A{}, A{}); > +}; > + > +constexpr B b; > +SA(b.a.p == &b.a); > + > +struct C { > + int x; > + int y = x; > +}; > + > +struct D { > + int x = 0; > + int y = (C{x}.y, C{x}.y); > +}; > + > +constexpr D d = { }; > +D d2 = {}; > > base-commit: 682e587f1021241758f7dfe0b22651008622a312 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-24 12:36 ` Jason Merrill @ 2022-05-24 13:55 ` Marek Polacek 2022-05-24 20:01 ` Jason Merrill 0 siblings, 1 reply; 17+ messages in thread From: Marek Polacek @ 2022-05-24 13:55 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On Tue, May 24, 2022 at 08:36:39AM -0400, Jason Merrill wrote: > On 5/16/22 11:36, Marek Polacek wrote: > > +static tree > > +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) > > +{ > > + tree t = *tp; > > + tree full_expr = *static_cast<tree *>(data); > > + > > + /* We're looking for a TARGET_EXPR nested in the whole expression. */ > > + if (TREE_CODE (t) == TARGET_EXPR > > + && !potential_prvalue_result_of (t, full_expr)) > > + { > > + tree init = TARGET_EXPR_INITIAL (t); > > + while (TREE_CODE (init) == COMPOUND_EXPR) > > + init = TREE_OPERAND (init, 1); > > Hmm, how do we get a COMPOUND_EXPR around a CONSTRUCTOR? Sadly, that's possible for code like (from nsdmi-aggr18.C) struct D { int x = 42; B b = (true, A{x}); }; where the TARGET_EXPR_INITIAL is <<< Unknown tree: void_cst >>>, {.x=((struct D *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} Marek ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-24 13:55 ` Marek Polacek @ 2022-05-24 20:01 ` Jason Merrill 2022-05-24 20:21 ` Marek Polacek 0 siblings, 1 reply; 17+ messages in thread From: Jason Merrill @ 2022-05-24 20:01 UTC (permalink / raw) To: Marek Polacek; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On 5/24/22 09:55, Marek Polacek wrote: > On Tue, May 24, 2022 at 08:36:39AM -0400, Jason Merrill wrote: >> On 5/16/22 11:36, Marek Polacek wrote: >>> +static tree >>> +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) >>> +{ >>> + tree t = *tp; >>> + tree full_expr = *static_cast<tree *>(data); >>> + >>> + /* We're looking for a TARGET_EXPR nested in the whole expression. */ >>> + if (TREE_CODE (t) == TARGET_EXPR >>> + && !potential_prvalue_result_of (t, full_expr)) >>> + { >>> + tree init = TARGET_EXPR_INITIAL (t); >>> + while (TREE_CODE (init) == COMPOUND_EXPR) >>> + init = TREE_OPERAND (init, 1); >> >> Hmm, how do we get a COMPOUND_EXPR around a CONSTRUCTOR? > > Sadly, that's possible for code like (from nsdmi-aggr18.C) > > struct D { > int x = 42; > B b = (true, A{x}); > }; > > where the TARGET_EXPR_INITIAL is > <<< Unknown tree: void_cst >>>, {.x=((struct D *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} Hmm, perhaps cp_build_compound_expr should build an additional TARGET_EXPR around the COMPOUND_EXPR but leave the one inside alone. Feel free to investigate that if you'd like, or the patch is OK as is. Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-24 20:01 ` Jason Merrill @ 2022-05-24 20:21 ` Marek Polacek 2022-05-25 12:22 ` Jason Merrill 0 siblings, 1 reply; 17+ messages in thread From: Marek Polacek @ 2022-05-24 20:21 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On Tue, May 24, 2022 at 04:01:37PM -0400, Jason Merrill wrote: > On 5/24/22 09:55, Marek Polacek wrote: > > On Tue, May 24, 2022 at 08:36:39AM -0400, Jason Merrill wrote: > > > On 5/16/22 11:36, Marek Polacek wrote: > > > > +static tree > > > > +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) > > > > +{ > > > > + tree t = *tp; > > > > + tree full_expr = *static_cast<tree *>(data); > > > > + > > > > + /* We're looking for a TARGET_EXPR nested in the whole expression. */ > > > > + if (TREE_CODE (t) == TARGET_EXPR > > > > + && !potential_prvalue_result_of (t, full_expr)) > > > > + { > > > > + tree init = TARGET_EXPR_INITIAL (t); > > > > + while (TREE_CODE (init) == COMPOUND_EXPR) > > > > + init = TREE_OPERAND (init, 1); > > > > > > Hmm, how do we get a COMPOUND_EXPR around a CONSTRUCTOR? > > > > Sadly, that's possible for code like (from nsdmi-aggr18.C) > > > > struct D { > > int x = 42; > > B b = (true, A{x}); > > }; > > > > where the TARGET_EXPR_INITIAL is > > <<< Unknown tree: void_cst >>>, {.x=((struct D *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} > > Hmm, perhaps cp_build_compound_expr should build an additional TARGET_EXPR > around the COMPOUND_EXPR but leave the one inside alone. Feel free to > investigate that if you'd like, or the patch is OK as is. Sorry, I was unclear. The whole expression is: TARGET_EXPR <D.2439, A::operator B (&TARGET_EXPR <D.2405, <<< Unknown tree: void_cst >>>, {.x=((struct D *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>)> so there *is* a TARGET_EXPR around the COMPOUND_EXPR. We'd have to build a TARGET_EXPR around the COMPOUND_EXPR's RHS = the CONSTRUCTOR. Frankly, I'm not sure if it's worth the effort. The while loop is somewhat unsightly but not too bad. Marek ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v5] c++: ICE with temporary of class type in DMI [PR100252] 2022-05-24 20:21 ` Marek Polacek @ 2022-05-25 12:22 ` Jason Merrill 0 siblings, 0 replies; 17+ messages in thread From: Jason Merrill @ 2022-05-25 12:22 UTC (permalink / raw) To: Marek Polacek; +Cc: Patrick Palka, GCC Patches, Jakub Jelinek On 5/24/22 16:21, Marek Polacek wrote: > On Tue, May 24, 2022 at 04:01:37PM -0400, Jason Merrill wrote: >> On 5/24/22 09:55, Marek Polacek wrote: >>> On Tue, May 24, 2022 at 08:36:39AM -0400, Jason Merrill wrote: >>>> On 5/16/22 11:36, Marek Polacek wrote: >>>>> +static tree >>>>> +replace_placeholders_for_class_temp_r (tree *tp, int *, void *data) >>>>> +{ >>>>> + tree t = *tp; >>>>> + tree full_expr = *static_cast<tree *>(data); >>>>> + >>>>> + /* We're looking for a TARGET_EXPR nested in the whole expression. */ >>>>> + if (TREE_CODE (t) == TARGET_EXPR >>>>> + && !potential_prvalue_result_of (t, full_expr)) >>>>> + { >>>>> + tree init = TARGET_EXPR_INITIAL (t); >>>>> + while (TREE_CODE (init) == COMPOUND_EXPR) >>>>> + init = TREE_OPERAND (init, 1); >>>> >>>> Hmm, how do we get a COMPOUND_EXPR around a CONSTRUCTOR? >>> >>> Sadly, that's possible for code like (from nsdmi-aggr18.C) >>> >>> struct D { >>> int x = 42; >>> B b = (true, A{x}); >>> }; >>> >>> where the TARGET_EXPR_INITIAL is >>> <<< Unknown tree: void_cst >>>, {.x=((struct D *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x} >> >> Hmm, perhaps cp_build_compound_expr should build an additional TARGET_EXPR >> around the COMPOUND_EXPR but leave the one inside alone. Feel free to >> investigate that if you'd like, or the patch is OK as is. > > Sorry, I was unclear. The whole expression is: > > TARGET_EXPR <D.2439, A::operator B (&TARGET_EXPR <D.2405, <<< Unknown tree: void_cst >>>, {.x=((struct D *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>)> > > so there *is* a TARGET_EXPR around the COMPOUND_EXPR. Yes, but that's because cp_build_compound_expr changes TARGET_EXPR_INITIAL of the TARGET_EXPR from the CONSTRUCTOR to the COMPOUND_EXPR; I'm suggesting it might build a new one instead. > We'd have to build > a TARGET_EXPR around the COMPOUND_EXPR's RHS = the CONSTRUCTOR. Frankly, > I'm not sure if it's worth the effort. The while loop is somewhat unsightly > but not too bad. Agreed. Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-05-25 12:22 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-26 23:02 [PATCH] c++: ICE with temporary of class type in DMI [PR100252] Marek Polacek 2022-04-27 15:00 ` Patrick Palka 2022-04-27 20:30 ` [PATCH v2] " Marek Polacek 2022-04-28 14:12 ` Patrick Palka 2022-04-28 14:39 ` Marek Polacek 2022-05-03 20:59 ` Jason Merrill 2022-05-07 19:11 ` [PATCH v3] " Marek Polacek 2022-05-07 22:02 ` Jason Merrill 2022-05-13 23:41 ` [PATCH v4] " Marek Polacek 2022-05-15 3:13 ` Jason Merrill 2022-05-16 15:36 ` [PATCH v5] " Marek Polacek 2022-05-24 11:40 ` Marek Polacek 2022-05-24 12:36 ` Jason Merrill 2022-05-24 13:55 ` Marek Polacek 2022-05-24 20:01 ` Jason Merrill 2022-05-24 20:21 ` Marek Polacek 2022-05-25 12:22 ` Jason Merrill
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).