C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Marek Polacek]

In this testcase we have
C c = bar (X{1});
which store_init_value sees as
c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
that walks the whole tree to substitute the placeholders.  Eventually we find
the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
all; it has nothing to with "c", it's bar's argument.

It occurred to me that we shouldn't step into CALL_EXPRs and leave the
placeholders in function arguments to cp_gimplify_init_expr which calls
replace_placeholders for constructors.  Not sure if it's enough to handle
CALL_EXPRs like this, anything else?

Bootstrapped/regtested on x86_64-linux, ok for trunk and 6?

2017-03-07  Marek Polacek  <polacek@redhat.com>

	PR c++/79937 - ICE in replace_placeholders_r
	* tree.c (replace_placeholders_r): Don't walk into CALL_EXPRs.

	* g++.dg/cpp1y/nsdmi-aggr7.C: New test.

diff --git gcc/cp/tree.c gcc/cp/tree.c
index d3c63b8..6a4f065 100644
--- gcc/cp/tree.c
+++ gcc/cp/tree.c
@@ -2751,6 +2751,11 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_)
 
   switch (TREE_CODE (*t))
     {
+    case CALL_EXPR:
+      /* Don't mess with placeholders in an unrelated object.  */
+      *walk_subtrees = false;
+      break;
+
     case PLACEHOLDER_EXPR:
       {
 	tree x = obj;
diff --git gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
index e69de29..c2fd404 100644
--- gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
+++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
@@ -0,0 +1,21 @@
+// PR c++/79937
+// { dg-do compile { target c++14 } }
+
+struct C {};
+
+struct X {
+  unsigned i;
+  unsigned n = i;
+};
+
+C
+bar (X)
+{
+  return {};
+}
+
+void
+foo ()
+{
+  C c = bar (X{1});
+}

	Marek

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Marek Polacek]

Ping.

On Tue, Mar 07, 2017 at 06:10:48PM +0100, Marek Polacek wrote:
> In this testcase we have
> C c = bar (X{1});
> which store_init_value sees as
> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
> that walks the whole tree to substitute the placeholders.  Eventually we find
> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
> all; it has nothing to with "c", it's bar's argument.
> 
> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
> placeholders in function arguments to cp_gimplify_init_expr which calls
> replace_placeholders for constructors.  Not sure if it's enough to handle
> CALL_EXPRs like this, anything else?
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk and 6?
> 
> 2017-03-07  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/79937 - ICE in replace_placeholders_r
> 	* tree.c (replace_placeholders_r): Don't walk into CALL_EXPRs.
> 
> 	* g++.dg/cpp1y/nsdmi-aggr7.C: New test.
> 
> diff --git gcc/cp/tree.c gcc/cp/tree.c
> index d3c63b8..6a4f065 100644
> --- gcc/cp/tree.c
> +++ gcc/cp/tree.c
> @@ -2751,6 +2751,11 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_)
>  
>    switch (TREE_CODE (*t))
>      {
> +    case CALL_EXPR:
> +      /* Don't mess with placeholders in an unrelated object.  */
> +      *walk_subtrees = false;
> +      break;
> +
>      case PLACEHOLDER_EXPR:
>        {
>  	tree x = obj;
> diff --git gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
> index e69de29..c2fd404 100644
> --- gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
> +++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr7.C
> @@ -0,0 +1,21 @@
> +// PR c++/79937
> +// { dg-do compile { target c++14 } }
> +
> +struct C {};
> +
> +struct X {
> +  unsigned i;
> +  unsigned n = i;
> +};
> +
> +C
> +bar (X)
> +{
> +  return {};
> +}
> +
> +void
> +foo ()
> +{
> +  C c = bar (X{1});
> +}
> 
> 	Marek

	Marek

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Jason Merrill]

On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
> In this testcase we have
> C c = bar (X{1});
> which store_init_value sees as
> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
> that walks the whole tree to substitute the placeholders.  Eventually we find
> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
> all; it has nothing to with "c", it's bar's argument.
>
> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
> placeholders in function arguments to cp_gimplify_init_expr which calls
> replace_placeholders for constructors.  Not sure if it's enough to handle
> CALL_EXPRs like this, anything else?

Hmm, we might have a DMI containing a call with an argument referring
to *this, i.e.

struct A
{
  int i;
  int j = frob (this->i);
};

The TARGET_EXPR seems like a more likely barrier, but even there we
could have something like

struct A { int i; };
struct B
{
  int i;
  A a = A{this->i};
};

I think we need replace_placeholders to keep a stack of objects, so
that when we see a TARGET_EXPR we add it to the stack and therefore
can properly replace a PLACEHOLDER_EXPR of its type.

Jason

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Jason Merrill]

On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill <jason@redhat.com> wrote:
> On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
>> In this testcase we have
>> C c = bar (X{1});
>> which store_init_value sees as
>> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
>> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
>> that walks the whole tree to substitute the placeholders.  Eventually we find
>> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
>> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
>> all; it has nothing to with "c", it's bar's argument.
>>
>> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
>> placeholders in function arguments to cp_gimplify_init_expr which calls
>> replace_placeholders for constructors.  Not sure if it's enough to handle
>> CALL_EXPRs like this, anything else?
>
> Hmm, we might have a DMI containing a call with an argument referring
> to *this, i.e.
>
> struct A
> {
>   int i;
>   int j = frob (this->i);
> };
>
> The TARGET_EXPR seems like a more likely barrier, but even there we
> could have something like
>
> struct A { int i; };
> struct B
> {
>   int i;
>   A a = A{this->i};
> };
>
> I think we need replace_placeholders to keep a stack of objects, so
> that when we see a TARGET_EXPR we add it to the stack and therefore
> can properly replace a PLACEHOLDER_EXPR of its type.

Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
it for later when we lower the TARGET_EXPR.

Jason

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Marek Polacek]

On Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote:
> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill <jason@redhat.com> wrote:
> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> In this testcase we have
> >> C c = bar (X{1});
> >> which store_init_value sees as
> >> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
> >> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
> >> that walks the whole tree to substitute the placeholders.  Eventually we find
> >> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
> >> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
> >> all; it has nothing to with "c", it's bar's argument.
> >>
> >> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
> >> placeholders in function arguments to cp_gimplify_init_expr which calls
> >> replace_placeholders for constructors.  Not sure if it's enough to handle
> >> CALL_EXPRs like this, anything else?
> >
> > Hmm, we might have a DMI containing a call with an argument referring
> > to *this, i.e.
> >
> > struct A
> > {
> >   int i;
> >   int j = frob (this->i);
> > };
> >
> > The TARGET_EXPR seems like a more likely barrier, but even there we
> > could have something like
> >
> > struct A { int i; };
> > struct B
> > {
> >   int i;
> >   A a = A{this->i};
> > };
> >
> > I think we need replace_placeholders to keep a stack of objects, so
> > that when we see a TARGET_EXPR we add it to the stack and therefore
> > can properly replace a PLACEHOLDER_EXPR of its type.
> 
> Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
> it for later when we lower the TARGET_EXPR.

Sorry, I don't really follow.  I have a patch that puts TARGET_EXPRs on
a stack, but I don't know how that helps.  E.g. with nsdmi-aggr3.C
we have
B b = TARGET_EXPR <D1, {.a = TARGET_EXPR <D2, (struct A *) &<PLACEHOLDER_EXPR struct B>>}>
so when we get to that PLACEHOLDER_EXPR, on the stack there's
TARGET_EXPR with type struct A
TARGET_EXPR with type struct B
so the type of the PLACEHOLDER_EXPR doesn't match the type of the current
TARGET_EXPR, but we still want to replace it in this case.

So -- could you expand a bit on what you had in mind, please?

Thanks,

	Marek

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Jason Merrill]

On Thu, Mar 23, 2017 at 4:34 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote:
>> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill <jason@redhat.com> wrote:
>> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
>> >> In this testcase we have
>> >> C c = bar (X{1});
>> >> which store_init_value sees as
>> >> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
>> >> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
>> >> that walks the whole tree to substitute the placeholders.  Eventually we find
>> >> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
>> >> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
>> >> all; it has nothing to with "c", it's bar's argument.
>> >>
>> >> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
>> >> placeholders in function arguments to cp_gimplify_init_expr which calls
>> >> replace_placeholders for constructors.  Not sure if it's enough to handle
>> >> CALL_EXPRs like this, anything else?
>> >
>> > Hmm, we might have a DMI containing a call with an argument referring
>> > to *this, i.e.
>> >
>> > struct A
>> > {
>> >   int i;
>> >   int j = frob (this->i);
>> > };
>> >
>> > The TARGET_EXPR seems like a more likely barrier, but even there we
>> > could have something like
>> >
>> > struct A { int i; };
>> > struct B
>> > {
>> >   int i;
>> >   A a = A{this->i};
>> > };
>> >
>> > I think we need replace_placeholders to keep a stack of objects, so
>> > that when we see a TARGET_EXPR we add it to the stack and therefore
>> > can properly replace a PLACEHOLDER_EXPR of its type.
>>
>> Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
>> it for later when we lower the TARGET_EXPR.
>
> Sorry, I don't really follow.  I have a patch that puts TARGET_EXPRs on
> a stack, but I don't know how that helps.  E.g. with nsdmi-aggr3.C
> we have
> B b = TARGET_EXPR <D1, {.a = TARGET_EXPR <D2, (struct A *) &<PLACEHOLDER_EXPR struct B>>}>
> so when we get to that PLACEHOLDER_EXPR, on the stack there's
> TARGET_EXPR with type struct A
> TARGET_EXPR with type struct B
> so the type of the PLACEHOLDER_EXPR doesn't match the type of the current
> TARGET_EXPR, but we still want to replace it in this case.
>
> So -- could you expand a bit on what you had in mind, please?

So then when we see a placeholder, we walk the stack to find the
object of the matching type.

But if the object we find was collected from walking through a
TARGET_EXPR, we should leave the PLACEHOLDER_EXPR alone, so that it
can be replaced later with the actual target of the initialization.

Jason

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Marek Polacek]

On Thu, Mar 23, 2017 at 05:09:58PM -0400, Jason Merrill wrote:
> On Thu, Mar 23, 2017 at 4:34 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote:
> >> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill <jason@redhat.com> wrote:
> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> >> In this testcase we have
> >> >> C c = bar (X{1});
> >> >> which store_init_value sees as
> >> >> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
> >> >> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
> >> >> that walks the whole tree to substitute the placeholders.  Eventually we find
> >> >> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
> >> >> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
> >> >> all; it has nothing to with "c", it's bar's argument.
> >> >>
> >> >> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
> >> >> placeholders in function arguments to cp_gimplify_init_expr which calls
> >> >> replace_placeholders for constructors.  Not sure if it's enough to handle
> >> >> CALL_EXPRs like this, anything else?
> >> >
> >> > Hmm, we might have a DMI containing a call with an argument referring
> >> > to *this, i.e.
> >> >
> >> > struct A
> >> > {
> >> >   int i;
> >> >   int j = frob (this->i);
> >> > };
> >> >
> >> > The TARGET_EXPR seems like a more likely barrier, but even there we
> >> > could have something like
> >> >
> >> > struct A { int i; };
> >> > struct B
> >> > {
> >> >   int i;
> >> >   A a = A{this->i};
> >> > };
> >> >
> >> > I think we need replace_placeholders to keep a stack of objects, so
> >> > that when we see a TARGET_EXPR we add it to the stack and therefore
> >> > can properly replace a PLACEHOLDER_EXPR of its type.
> >>
> >> Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
> >> it for later when we lower the TARGET_EXPR.
> >
> > Sorry, I don't really follow.  I have a patch that puts TARGET_EXPRs on
> > a stack, but I don't know how that helps.  E.g. with nsdmi-aggr3.C
> > we have
> > B b = TARGET_EXPR <D1, {.a = TARGET_EXPR <D2, (struct A *) &<PLACEHOLDER_EXPR struct B>>}>
> > so when we get to that PLACEHOLDER_EXPR, on the stack there's
> > TARGET_EXPR with type struct A
> > TARGET_EXPR with type struct B
> > so the type of the PLACEHOLDER_EXPR doesn't match the type of the current
> > TARGET_EXPR, but we still want to replace it in this case.
> >
> > So -- could you expand a bit on what you had in mind, please?
> 
> So then when we see a placeholder, we walk the stack to find the
> object of the matching type.
> 
> But if the object we find was collected from walking through a
> TARGET_EXPR, we should leave the PLACEHOLDER_EXPR alone, so that it
> can be replaced later with the actual target of the initialization.

Unfortunately, I still don't understand; guess I'll have to drop this PR.

With this we put TARGET_EXPRs on a stack, and then when we find a
PLACEHOLDER_EXPR we walk the stack to find a TARGET_EXPR of the same type as
the PLACEHOLDER_EXPR.  There are three simplified examples I've been playing
with:

  B b = T_E <D1, {.a = T_E <D2, ... &<P_E struct B>>}>

  - here we should replace the P_E; on the stack there are two
    TARGET_EXPRs of types B and A

  C c = T_E <D1, bar (T_E <D2, &<P_E struct X>>)>

  - here we shouldn't replace the P_E; on the stack there are two
    TARGET_EXPRs of types X and C

  B b = T_E <D1, {.a = {.b = &<P_E struct B>}}>

  - here we should replace the P_E; on the stack there's one TARGET_EXPR
    of type B

In each case we find a TARGET_EXPR of the type of the PLACEHOLDER_EXPR, but I
don't see how to decide which PLACEHOLDER_EXPR we should let slide.  Sorry for
being dense...

diff --git gcc/cp/tree.c gcc/cp/tree.c
index 2757af6..2439a00 100644
--- gcc/cp/tree.c
+++ gcc/cp/tree.c
@@ -2741,8 +2741,12 @@ build_ctor_subob_ref (tree index, tree type, tree obj)
 
 struct replace_placeholders_t
 {
-  tree obj;	    /* The object to be substituted for a PLACEHOLDER_EXPR.  */
-  bool seen;	    /* Whether we've encountered a PLACEHOLDER_EXPR.  */
+  /* The object to be substituted for a PLACEHOLDER_EXPR.  */
+  tree obj;
+  /* Whether we've encountered a PLACEHOLDER_EXPR.  */
+  bool seen;
+  /* A stack of TARGET_EXPRs we've found ourselves in.  */
+  vec<tree> target_expr_stack;
 };
 
 /* Like substitute_placeholder_in_expr, but handle C++ tree codes and
@@ -2762,14 +2766,35 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_)
 
   switch (TREE_CODE (*t))
     {
+    case TARGET_EXPR:
+      d->target_expr_stack.safe_push (*t);
+      cp_walk_tree (&TARGET_EXPR_INITIAL (*t), replace_placeholders_r, data_,
+		    NULL);
+      d->target_expr_stack.pop ();
+      *walk_subtrees = 0;
+      break;
+
     case PLACEHOLDER_EXPR:
       {
-	tree x = obj;
-	for (; !(same_type_ignoring_top_level_qualifiers_p
-		 (TREE_TYPE (*t), TREE_TYPE (x)));
-	     x = TREE_OPERAND (x, 0))
-	  gcc_assert (TREE_CODE (x) == COMPONENT_REF);
-	*t = x;
+	bool skip_it = false;
+	unsigned ix;
+	tree targ;
+	/* Walk the stack to find the object of the matching type.  */
+	FOR_EACH_VEC_ELT_REVERSE (d->target_expr_stack, ix, targ)
+	  if (same_type_ignoring_top_level_qualifiers_p
+	      (TREE_TYPE (*t), TREE_TYPE (targ)))
+	    {
+	      // ...
+	    }
+	if (!skip_it)
+	  {
+	    tree x = obj;
+	    for (; !(same_type_ignoring_top_level_qualifiers_p
+		     (TREE_TYPE (*t), TREE_TYPE (x)));
+		 x = TREE_OPERAND (x, 0))
+	      gcc_assert (TREE_CODE (x) == COMPONENT_REF);
+	    *t = x;
+	  }
 	*walk_subtrees = false;
 	d->seen = true;
       }
@@ -2813,14 +2838,23 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_)
   return NULL_TREE;
 }
 
+/* Replace PLACEHOLDER_EXPRs in EXP with object OBJ.  */
+
 tree
 replace_placeholders (tree exp, tree obj, bool *seen_p)
 {
   tree *tp = &exp;
-  replace_placeholders_t data = { obj, false };
+  replace_placeholders_t data;
+  data.obj = obj;
+  data.seen = false;
+  data.target_expr_stack.create (0);
   if (TREE_CODE (exp) == TARGET_EXPR)
-    tp = &TARGET_EXPR_INITIAL (exp);
+    {
+      tp = &TARGET_EXPR_INITIAL (exp);
+      data.target_expr_stack.safe_push (exp);
+    }
   cp_walk_tree (tp, replace_placeholders_r, &data, NULL);
+  data.target_expr_stack.release ();
   if (seen_p)
     *seen_p = data.seen;
   return exp;

	Marek

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Marek Polacek]

Ping.  Any ideas how to move this forward?

On Fri, Mar 24, 2017 at 05:22:00PM +0100, Marek Polacek wrote:
> On Thu, Mar 23, 2017 at 05:09:58PM -0400, Jason Merrill wrote:
> > On Thu, Mar 23, 2017 at 4:34 PM, Marek Polacek <polacek@redhat.com> wrote:
> > > On Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote:
> > >> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill <jason@redhat.com> wrote:
> > >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
> > >> >> In this testcase we have
> > >> >> C c = bar (X{1});
> > >> >> which store_init_value sees as
> > >> >> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
> > >> >> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
> > >> >> that walks the whole tree to substitute the placeholders.  Eventually we find
> > >> >> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
> > >> >> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
> > >> >> all; it has nothing to with "c", it's bar's argument.
> > >> >>
> > >> >> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
> > >> >> placeholders in function arguments to cp_gimplify_init_expr which calls
> > >> >> replace_placeholders for constructors.  Not sure if it's enough to handle
> > >> >> CALL_EXPRs like this, anything else?
> > >> >
> > >> > Hmm, we might have a DMI containing a call with an argument referring
> > >> > to *this, i.e.
> > >> >
> > >> > struct A
> > >> > {
> > >> >   int i;
> > >> >   int j = frob (this->i);
> > >> > };
> > >> >
> > >> > The TARGET_EXPR seems like a more likely barrier, but even there we
> > >> > could have something like
> > >> >
> > >> > struct A { int i; };
> > >> > struct B
> > >> > {
> > >> >   int i;
> > >> >   A a = A{this->i};
> > >> > };
> > >> >
> > >> > I think we need replace_placeholders to keep a stack of objects, so
> > >> > that when we see a TARGET_EXPR we add it to the stack and therefore
> > >> > can properly replace a PLACEHOLDER_EXPR of its type.
> > >>
> > >> Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
> > >> it for later when we lower the TARGET_EXPR.
> > >
> > > Sorry, I don't really follow.  I have a patch that puts TARGET_EXPRs on
> > > a stack, but I don't know how that helps.  E.g. with nsdmi-aggr3.C
> > > we have
> > > B b = TARGET_EXPR <D1, {.a = TARGET_EXPR <D2, (struct A *) &<PLACEHOLDER_EXPR struct B>>}>
> > > so when we get to that PLACEHOLDER_EXPR, on the stack there's
> > > TARGET_EXPR with type struct A
> > > TARGET_EXPR with type struct B
> > > so the type of the PLACEHOLDER_EXPR doesn't match the type of the current
> > > TARGET_EXPR, but we still want to replace it in this case.
> > >
> > > So -- could you expand a bit on what you had in mind, please?
> > 
> > So then when we see a placeholder, we walk the stack to find the
> > object of the matching type.
> > 
> > But if the object we find was collected from walking through a
> > TARGET_EXPR, we should leave the PLACEHOLDER_EXPR alone, so that it
> > can be replaced later with the actual target of the initialization.
> 
> Unfortunately, I still don't understand; guess I'll have to drop this PR.
> 
> With this we put TARGET_EXPRs on a stack, and then when we find a
> PLACEHOLDER_EXPR we walk the stack to find a TARGET_EXPR of the same type as
> the PLACEHOLDER_EXPR.  There are three simplified examples I've been playing
> with:
> 
>   B b = T_E <D1, {.a = T_E <D2, ... &<P_E struct B>>}>
> 
>   - here we should replace the P_E; on the stack there are two
>     TARGET_EXPRs of types B and A
> 
>   C c = T_E <D1, bar (T_E <D2, &<P_E struct X>>)>
> 
>   - here we shouldn't replace the P_E; on the stack there are two
>     TARGET_EXPRs of types X and C
> 
>   B b = T_E <D1, {.a = {.b = &<P_E struct B>}}>
> 
>   - here we should replace the P_E; on the stack there's one TARGET_EXPR
>     of type B
> 
> In each case we find a TARGET_EXPR of the type of the PLACEHOLDER_EXPR, but I
> don't see how to decide which PLACEHOLDER_EXPR we should let slide.  Sorry for
> being dense...
> 
> diff --git gcc/cp/tree.c gcc/cp/tree.c
> index 2757af6..2439a00 100644
> --- gcc/cp/tree.c
> +++ gcc/cp/tree.c
> @@ -2741,8 +2741,12 @@ build_ctor_subob_ref (tree index, tree type, tree obj)
>  
>  struct replace_placeholders_t
>  {
> -  tree obj;	    /* The object to be substituted for a PLACEHOLDER_EXPR.  */
> -  bool seen;	    /* Whether we've encountered a PLACEHOLDER_EXPR.  */
> +  /* The object to be substituted for a PLACEHOLDER_EXPR.  */
> +  tree obj;
> +  /* Whether we've encountered a PLACEHOLDER_EXPR.  */
> +  bool seen;
> +  /* A stack of TARGET_EXPRs we've found ourselves in.  */
> +  vec<tree> target_expr_stack;
>  };
>  
>  /* Like substitute_placeholder_in_expr, but handle C++ tree codes and
> @@ -2762,14 +2766,35 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_)
>  
>    switch (TREE_CODE (*t))
>      {
> +    case TARGET_EXPR:
> +      d->target_expr_stack.safe_push (*t);
> +      cp_walk_tree (&TARGET_EXPR_INITIAL (*t), replace_placeholders_r, data_,
> +		    NULL);
> +      d->target_expr_stack.pop ();
> +      *walk_subtrees = 0;
> +      break;
> +
>      case PLACEHOLDER_EXPR:
>        {
> -	tree x = obj;
> -	for (; !(same_type_ignoring_top_level_qualifiers_p
> -		 (TREE_TYPE (*t), TREE_TYPE (x)));
> -	     x = TREE_OPERAND (x, 0))
> -	  gcc_assert (TREE_CODE (x) == COMPONENT_REF);
> -	*t = x;
> +	bool skip_it = false;
> +	unsigned ix;
> +	tree targ;
> +	/* Walk the stack to find the object of the matching type.  */
> +	FOR_EACH_VEC_ELT_REVERSE (d->target_expr_stack, ix, targ)
> +	  if (same_type_ignoring_top_level_qualifiers_p
> +	      (TREE_TYPE (*t), TREE_TYPE (targ)))
> +	    {
> +	      // ...
> +	    }
> +	if (!skip_it)
> +	  {
> +	    tree x = obj;
> +	    for (; !(same_type_ignoring_top_level_qualifiers_p
> +		     (TREE_TYPE (*t), TREE_TYPE (x)));
> +		 x = TREE_OPERAND (x, 0))
> +	      gcc_assert (TREE_CODE (x) == COMPONENT_REF);
> +	    *t = x;
> +	  }
>  	*walk_subtrees = false;
>  	d->seen = true;
>        }
> @@ -2813,14 +2838,23 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_)
>    return NULL_TREE;
>  }
>  
> +/* Replace PLACEHOLDER_EXPRs in EXP with object OBJ.  */
> +
>  tree
>  replace_placeholders (tree exp, tree obj, bool *seen_p)
>  {
>    tree *tp = &exp;
> -  replace_placeholders_t data = { obj, false };
> +  replace_placeholders_t data;
> +  data.obj = obj;
> +  data.seen = false;
> +  data.target_expr_stack.create (0);
>    if (TREE_CODE (exp) == TARGET_EXPR)
> -    tp = &TARGET_EXPR_INITIAL (exp);
> +    {
> +      tp = &TARGET_EXPR_INITIAL (exp);
> +      data.target_expr_stack.safe_push (exp);
> +    }
>    cp_walk_tree (tp, replace_placeholders_r, &data, NULL);
> +  data.target_expr_stack.release ();
>    if (seen_p)
>      *seen_p = data.seen;
>    return exp;
> 
> 	Marek

	Marek

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Jason Merrill]

On Fri, Mar 24, 2017 at 12:22 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Thu, Mar 23, 2017 at 05:09:58PM -0400, Jason Merrill wrote:
>> On Thu, Mar 23, 2017 at 4:34 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote:
>> >> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill <jason@redhat.com> wrote:
>> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
>> >> >> In this testcase we have
>> >> >> C c = bar (X{1});
>> >> >> which store_init_value sees as
>> >> >> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
>> >> >> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
>> >> >> that walks the whole tree to substitute the placeholders.  Eventually we find
>> >> >> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
>> >> >> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
>> >> >> all; it has nothing to with "c", it's bar's argument.
>> >> >>
>> >> >> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
>> >> >> placeholders in function arguments to cp_gimplify_init_expr which calls
>> >> >> replace_placeholders for constructors.  Not sure if it's enough to handle
>> >> >> CALL_EXPRs like this, anything else?
>> >> >
>> >> > Hmm, we might have a DMI containing a call with an argument referring
>> >> > to *this, i.e.
>> >> >
>> >> > struct A
>> >> > {
>> >> >   int i;
>> >> >   int j = frob (this->i);
>> >> > };
>> >> >
>> >> > The TARGET_EXPR seems like a more likely barrier, but even there we
>> >> > could have something like
>> >> >
>> >> > struct A { int i; };
>> >> > struct B
>> >> > {
>> >> >   int i;
>> >> >   A a = A{this->i};
>> >> > };
>> >> >
>> >> > I think we need replace_placeholders to keep a stack of objects, so
>> >> > that when we see a TARGET_EXPR we add it to the stack and therefore
>> >> > can properly replace a PLACEHOLDER_EXPR of its type.
>> >>
>> >> Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
>> >> it for later when we lower the TARGET_EXPR.
>> >
>> > Sorry, I don't really follow.  I have a patch that puts TARGET_EXPRs on
>> > a stack, but I don't know how that helps.  E.g. with nsdmi-aggr3.C
>> > we have
>> > B b = TARGET_EXPR <D1, {.a = TARGET_EXPR <D2, (struct A *) &<PLACEHOLDER_EXPR struct B>>}>
>> > so when we get to that PLACEHOLDER_EXPR, on the stack there's
>> > TARGET_EXPR with type struct A
>> > TARGET_EXPR with type struct B
>> > so the type of the PLACEHOLDER_EXPR doesn't match the type of the current
>> > TARGET_EXPR, but we still want to replace it in this case.
>> >
>> > So -- could you expand a bit on what you had in mind, please?
>>
>> So then when we see a placeholder, we walk the stack to find the
>> object of the matching type.
>>
>> But if the object we find was collected from walking through a
>> TARGET_EXPR, we should leave the PLACEHOLDER_EXPR alone, so that it
>> can be replaced later with the actual target of the initialization.
>
> Unfortunately, I still don't understand; guess I'll have to drop this PR.
>
> With this we put TARGET_EXPRs on a stack, and then when we find a
> PLACEHOLDER_EXPR we walk the stack to find a TARGET_EXPR of the same type as
> the PLACEHOLDER_EXPR.  There are three simplified examples I've been playing
> with:
>
>   B b = T_E <D1, {.a = T_E <D2, ... &<P_E struct B>>}>
>
>   - here we should replace the P_E; on the stack there are two
>     TARGET_EXPRs of types B and A
>
>   C c = T_E <D1, bar (T_E <D2, &<P_E struct X>>)>
>
>   - here we shouldn't replace the P_E; on the stack there are two
>     TARGET_EXPRs of types X and C
>
>   B b = T_E <D1, {.a = {.b = &<P_E struct B>}}>
>
>   - here we should replace the P_E; on the stack there's one TARGET_EXPR
>     of type B
>
> In each case we find a TARGET_EXPR of the type of the PLACEHOLDER_EXPR, but I
> don't see how to decide which PLACEHOLDER_EXPR we should let slide.  Sorry for
> being dense...

I was thinking that we want to replace the type of the first entry in
the stack (B, C, B respectively), and leave others alone.

Jason

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Marek Polacek]

On Fri, Apr 07, 2017 at 03:27:36PM -0400, Jason Merrill wrote:
> On Fri, Mar 24, 2017 at 12:22 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Thu, Mar 23, 2017 at 05:09:58PM -0400, Jason Merrill wrote:
> >> On Thu, Mar 23, 2017 at 4:34 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> > On Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote:
> >> >> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill <jason@redhat.com> wrote:
> >> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> >> >> In this testcase we have
> >> >> >> C c = bar (X{1});
> >> >> >> which store_init_value sees as
> >> >> >> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
> >> >> >> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
> >> >> >> that walks the whole tree to substitute the placeholders.  Eventually we find
> >> >> >> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
> >> >> >> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
> >> >> >> all; it has nothing to with "c", it's bar's argument.
> >> >> >>
> >> >> >> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
> >> >> >> placeholders in function arguments to cp_gimplify_init_expr which calls
> >> >> >> replace_placeholders for constructors.  Not sure if it's enough to handle
> >> >> >> CALL_EXPRs like this, anything else?
> >> >> >
> >> >> > Hmm, we might have a DMI containing a call with an argument referring
> >> >> > to *this, i.e.
> >> >> >
> >> >> > struct A
> >> >> > {
> >> >> >   int i;
> >> >> >   int j = frob (this->i);
> >> >> > };
> >> >> >
> >> >> > The TARGET_EXPR seems like a more likely barrier, but even there we
> >> >> > could have something like
> >> >> >
> >> >> > struct A { int i; };
> >> >> > struct B
> >> >> > {
> >> >> >   int i;
> >> >> >   A a = A{this->i};
> >> >> > };
> >> >> >
> >> >> > I think we need replace_placeholders to keep a stack of objects, so
> >> >> > that when we see a TARGET_EXPR we add it to the stack and therefore
> >> >> > can properly replace a PLACEHOLDER_EXPR of its type.
> >> >>
> >> >> Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
> >> >> it for later when we lower the TARGET_EXPR.
> >> >
> >> > Sorry, I don't really follow.  I have a patch that puts TARGET_EXPRs on
> >> > a stack, but I don't know how that helps.  E.g. with nsdmi-aggr3.C
> >> > we have
> >> > B b = TARGET_EXPR <D1, {.a = TARGET_EXPR <D2, (struct A *) &<PLACEHOLDER_EXPR struct B>>}>
> >> > so when we get to that PLACEHOLDER_EXPR, on the stack there's
> >> > TARGET_EXPR with type struct A
> >> > TARGET_EXPR with type struct B
> >> > so the type of the PLACEHOLDER_EXPR doesn't match the type of the current
> >> > TARGET_EXPR, but we still want to replace it in this case.
> >> >
> >> > So -- could you expand a bit on what you had in mind, please?
> >>
> >> So then when we see a placeholder, we walk the stack to find the
> >> object of the matching type.
> >>
> >> But if the object we find was collected from walking through a
> >> TARGET_EXPR, we should leave the PLACEHOLDER_EXPR alone, so that it
> >> can be replaced later with the actual target of the initialization.
> >
> > Unfortunately, I still don't understand; guess I'll have to drop this PR.
> >
> > With this we put TARGET_EXPRs on a stack, and then when we find a
> > PLACEHOLDER_EXPR we walk the stack to find a TARGET_EXPR of the same type as
> > the PLACEHOLDER_EXPR.  There are three simplified examples I've been playing
> > with:
> >
> >   B b = T_E <D1, {.a = T_E <D2, ... &<P_E struct B>>}>
> >
> >   - here we should replace the P_E; on the stack there are two
> >     TARGET_EXPRs of types B and A
> >
> >   C c = T_E <D1, bar (T_E <D2, &<P_E struct X>>)>
> >
> >   - here we shouldn't replace the P_E; on the stack there are two
> >     TARGET_EXPRs of types X and C
> >
> >   B b = T_E <D1, {.a = {.b = &<P_E struct B>}}>
> >
> >   - here we should replace the P_E; on the stack there's one TARGET_EXPR
> >     of type B
> >
> > In each case we find a TARGET_EXPR of the type of the PLACEHOLDER_EXPR, but I
> > don't see how to decide which PLACEHOLDER_EXPR we should let slide.  Sorry for
> > being dense...
> 
> I was thinking that we want to replace the type of the first entry in
> the stack (B, C, B respectively), and leave others alone.

Even that didn't work for me, because we may end up with a case of

  C c = bar (T_E <D2, &<P_E struct X>>)

Oh well.  So I abandoned the idea of stacks and tried this straightforward
approach, which seems to work fine:

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-04-25  Marek Polacek  <polacek@redhat.com>

	PR c++/79937
	* tree.c (replace_placeholders_r): Don't substitute an object of
	unrelated type.

	* g++.dg/cpp1y/nsdmi-aggr10.C: New test.
	* g++.dg/cpp1y/nsdmi-aggr9.C: New test.

diff --git gcc/cp/tree.c gcc/cp/tree.c
index 15b3ad9..5391df8 100644
--- gcc/cp/tree.c
+++ gcc/cp/tree.c
@@ -2789,11 +2789,20 @@ replace_placeholders_r (tree* t, int* walk_subtrees, void* data_)
     case PLACEHOLDER_EXPR:
       {
 	tree x = obj;
-	for (; !(same_type_ignoring_top_level_qualifiers_p
-		 (TREE_TYPE (*t), TREE_TYPE (x)));
-	     x = TREE_OPERAND (x, 0))
-	  gcc_assert (TREE_CODE (x) == COMPONENT_REF);
-	*t = x;
+	bool skip = false;
+	while (!(same_type_ignoring_top_level_qualifiers_p
+		 (TREE_TYPE (*t), TREE_TYPE (x))))
+	  if (TREE_CODE (x) != COMPONENT_REF)
+	    {
+	      /* An object of unrelated type; don't substitute.  */
+	      skip = true;
+	      break;
+	    }
+	  else
+	    x = TREE_OPERAND (x, 0);
+
+	if (!skip)
+	  *t = x;
 	*walk_subtrees = false;
 	d->seen = true;
       }
diff --git gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr10.C gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr10.C
index e69de29..1e051d8 100644
--- gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr10.C
+++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr10.C
@@ -0,0 +1,16 @@
+// PR c++/79937
+// { dg-do compile { target c++14 } }
+
+extern int frob (int);
+
+struct A
+{
+  int i;
+  int j = frob (this->i);
+};
+
+void
+foo ()
+{
+  A a = { 1 };
+}
diff --git gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr9.C gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr9.C
index e69de29..c2fd404 100644
--- gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr9.C
+++ gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr9.C
@@ -0,0 +1,21 @@
+// PR c++/79937
+// { dg-do compile { target c++14 } }
+
+struct C {};
+
+struct X {
+  unsigned i;
+  unsigned n = i;
+};
+
+C
+bar (X)
+{
+  return {};
+}
+
+void
+foo ()
+{
+  C c = bar (X{1});
+}


	Marek

Re: C++ PATCH to fix ICE in replace_placeholders_r (PR c++/79937)

[Jason Merrill]

On Tue, Apr 25, 2017 at 12:17 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Fri, Apr 07, 2017 at 03:27:36PM -0400, Jason Merrill wrote:
>> On Fri, Mar 24, 2017 at 12:22 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > On Thu, Mar 23, 2017 at 05:09:58PM -0400, Jason Merrill wrote:
>> >> On Thu, Mar 23, 2017 at 4:34 PM, Marek Polacek <polacek@redhat.com> wrote:
>> >> > On Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote:
>> >> >> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill <jason@redhat.com> wrote:
>> >> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <polacek@redhat.com> wrote:
>> >> >> >> In this testcase we have
>> >> >> >> C c = bar (X{1});
>> >> >> >> which store_init_value sees as
>> >> >> >> c = TARGET_EXPR <D.2332, bar (TARGET_EXPR <D.2298, {.i=1, .n=(&<PLACEHOLDER_EXPR struct X>)->i}>)>
>> >> >> >> i.e. we're initializing "c" with a TARGET_EXPR.  We call replace_placeholders
>> >> >> >> that walks the whole tree to substitute the placeholders.  Eventually we find
>> >> >> >> the nested <PLACEHOLDER_EXPR struct X> but that's for another object, so we
>> >> >> >> crash.  Seems that we shouldn't have stepped into the second TARGET_EXPR at
>> >> >> >> all; it has nothing to with "c", it's bar's argument.
>> >> >> >>
>> >> >> >> It occurred to me that we shouldn't step into CALL_EXPRs and leave the
>> >> >> >> placeholders in function arguments to cp_gimplify_init_expr which calls
>> >> >> >> replace_placeholders for constructors.  Not sure if it's enough to handle
>> >> >> >> CALL_EXPRs like this, anything else?
>> >> >> >
>> >> >> > Hmm, we might have a DMI containing a call with an argument referring
>> >> >> > to *this, i.e.
>> >> >> >
>> >> >> > struct A
>> >> >> > {
>> >> >> >   int i;
>> >> >> >   int j = frob (this->i);
>> >> >> > };
>> >> >> >
>> >> >> > The TARGET_EXPR seems like a more likely barrier, but even there we
>> >> >> > could have something like
>> >> >> >
>> >> >> > struct A { int i; };
>> >> >> > struct B
>> >> >> > {
>> >> >> >   int i;
>> >> >> >   A a = A{this->i};
>> >> >> > };
>> >> >> >
>> >> >> > I think we need replace_placeholders to keep a stack of objects, so
>> >> >> > that when we see a TARGET_EXPR we add it to the stack and therefore
>> >> >> > can properly replace a PLACEHOLDER_EXPR of its type.
>> >> >>
>> >> >> Or actually, avoid replacing such a PLACEHOLDER_EXPR, but rather leave
>> >> >> it for later when we lower the TARGET_EXPR.
>> >> >
>> >> > Sorry, I don't really follow.  I have a patch that puts TARGET_EXPRs on
>> >> > a stack, but I don't know how that helps.  E.g. with nsdmi-aggr3.C
>> >> > we have
>> >> > B b = TARGET_EXPR <D1, {.a = TARGET_EXPR <D2, (struct A *) &<PLACEHOLDER_EXPR struct B>>}>
>> >> > so when we get to that PLACEHOLDER_EXPR, on the stack there's
>> >> > TARGET_EXPR with type struct A
>> >> > TARGET_EXPR with type struct B
>> >> > so the type of the PLACEHOLDER_EXPR doesn't match the type of the current
>> >> > TARGET_EXPR, but we still want to replace it in this case.
>> >> >
>> >> > So -- could you expand a bit on what you had in mind, please?
>> >>
>> >> So then when we see a placeholder, we walk the stack to find the
>> >> object of the matching type.
>> >>
>> >> But if the object we find was collected from walking through a
>> >> TARGET_EXPR, we should leave the PLACEHOLDER_EXPR alone, so that it
>> >> can be replaced later with the actual target of the initialization.
>> >
>> > Unfortunately, I still don't understand; guess I'll have to drop this PR.
>> >
>> > With this we put TARGET_EXPRs on a stack, and then when we find a
>> > PLACEHOLDER_EXPR we walk the stack to find a TARGET_EXPR of the same type as
>> > the PLACEHOLDER_EXPR.  There are three simplified examples I've been playing
>> > with:
>> >
>> >   B b = T_E <D1, {.a = T_E <D2, ... &<P_E struct B>>}>
>> >
>> >   - here we should replace the P_E; on the stack there are two
>> >     TARGET_EXPRs of types B and A
>> >
>> >   C c = T_E <D1, bar (T_E <D2, &<P_E struct X>>)>
>> >
>> >   - here we shouldn't replace the P_E; on the stack there are two
>> >     TARGET_EXPRs of types X and C
>> >
>> >   B b = T_E <D1, {.a = {.b = &<P_E struct B>}}>
>> >
>> >   - here we should replace the P_E; on the stack there's one TARGET_EXPR
>> >     of type B
>> >
>> > In each case we find a TARGET_EXPR of the type of the PLACEHOLDER_EXPR, but I
>> > don't see how to decide which PLACEHOLDER_EXPR we should let slide.  Sorry for
>> > being dense...
>>
>> I was thinking that we want to replace the type of the first entry in
>> the stack (B, C, B respectively), and leave others alone.
>
> Even that didn't work for me, because we may end up with a case of
>
>   C c = bar (T_E <D2, &<P_E struct X>>)

Why is that a problem?

Your patch doesn't fix this variant:

struct X {
  unsigned i;
  unsigned n = i;
};

unsigned int
bar (X x)
{
  return x.n;
}

int main()
{
  X x = { 2, bar (X{1}) };
  if (x.n != 1)
    __builtin_abort ();
}

Here we have two X objects, and we need to recognize them as distinct.

Jason