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  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  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  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  
>> >> >> wrote:
>> >> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  
>> >> >> > wrote:
>> >> >> >> In this testcase we have
>> >> >> >> C c = bar (X{1});
>> >> >> >> which store_init_value sees as
>> >> >> >> c = TARGET_EXPR > >> >> >> .n=(&)->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  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 > >> > &>}>
>> >> > 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 }>
>> >
>> >   - here we should replace the P_E; on the stack there are two
>> > TARGET_EXPRs of types B and A
>> >
>> >   C c = T_E )>
>> >
>> >   - 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 
>> >
>> >   - 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 )

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 

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  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  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  wrote:
> >> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  
> >> >> > wrote:
> >> >> >> In this testcase we have
> >> >> >> C c = bar (X{1});
> >> >> >> which store_init_value sees as
> >> >> >> c = TARGET_EXPR  >> >> >> .n=(&)->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  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  >> > &>}>
> >> > 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 }>
> >
> >   - here we should replace the P_E; on the stack there are two
> > TARGET_EXPRs of types B and A
> >
> >   C c = T_E )>
> >
> >   - 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 
> >
> >   - 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 )

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  

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 

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  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  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  wrote:
>> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  
>> >> > wrote:
>> >> >> In this testcase we have
>> >> >> C c = bar (X{1});
>> >> >> which store_init_value sees as
>> >> >> c = TARGET_EXPR > >> >> .n=(&)->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  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 > > &>}>
>> > 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 }>
>
>   - here we should replace the P_E; on the stack there are two
> TARGET_EXPRs of types B and A
>
>   C c = T_E )>
>
>   - 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 
>
>   - 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]

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  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  wrote:
> > >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  
> > >> > wrote:
> > >> >> In this testcase we have
> > >> >> C c = bar (X{1});
> > >> >> which store_init_value sees as
> > >> >> c = TARGET_EXPR  > >> >> .n=(&)->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  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  > > &>}>
> > > 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 }>
> 
>   - here we should replace the P_E; on the stack there are two
> TARGET_EXPRs of types B and A
> 
>   C c = T_E )>
> 
>   - 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 
> 
>   - 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 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 

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  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  wrote:
> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  
> >> > wrote:
> >> >> In this testcase we have
> >> >> C c = bar (X{1});
> >> >> which store_init_value sees as
> >> >> c = TARGET_EXPR  >> >> .n=(&)->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  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  > &>}>
> > 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 }>

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

  C c = T_E )>

  - 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 

  - 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 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 (_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
-  

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  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  wrote:
>> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  wrote:
>> >> In this testcase we have
>> >> C c = bar (X{1});
>> >> which store_init_value sees as
>> >> c = TARGET_EXPR > >> .n=(&)->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  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 >}>
> 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 Tue, Mar 14, 2017 at 02:34:30PM -0400, Jason Merrill wrote:
> On Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill  wrote:
> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  wrote:
> >> In this testcase we have
> >> C c = bar (X{1});
> >> which store_init_value sees as
> >> c = TARGET_EXPR  >> .n=(&)->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  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 }>
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 Tue, Mar 14, 2017 at 2:33 PM, Jason Merrill  wrote:
> On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  wrote:
>> In this testcase we have
>> C c = bar (X{1});
>> which store_init_value sees as
>> c = TARGET_EXPR > .n=(&)->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  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)

[Jason Merrill]

On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek  wrote:
> In this testcase we have
> C c = bar (X{1});
> which store_init_value sees as
> c = TARGET_EXPR  .n=(&)->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  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)

[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  .n=(&)->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  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  
> 
>   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

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 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  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  

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