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