On Fri, Apr 07, 2017 at 03:27:36PM -0400, Jason Merrill wrote:
> On Fri, Mar 24, 2017 at 12:22 PM, Marek Polacek <pola...@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 <pola...@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 <ja...@redhat.com> wrote:
> >> >> > On Tue, Mar 7, 2017 at 12:10 PM, Marek Polacek <pola...@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 <pola...@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
