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

[Marek Polacek <polacek at redhat dot com>]

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