| Message ID | 20170425161747.GV4255@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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}); +}