public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [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).