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 <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... > > 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<tree> 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 (&TARGET_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 > - (TREE_TYPE (*t), TREE_TYPE (x))); > - x = TREE_OPERAND (x, 0)) > - gcc_assert (TREE_CODE (x) == COMPONENT_REF); > - *t = x; > + bool skip_it = false; > + unsigned ix; > + tree targ; > + /* Walk the stack to find the object of the matching type. */ > + FOR_EACH_VEC_ELT_REVERSE (d->target_expr_stack, ix, targ) > + if (same_type_ignoring_top_level_qualifiers_p > + (TREE_TYPE (*t), TREE_TYPE (targ))) > + { > + // ... > + } > + if (!skip_it) > + { > + 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; > + } > *walk_subtrees = false; > d->seen = true; > } > @@ -2813,14 +2838,23 @@ replace_placeholders_r (tree* t, int* walk_subtrees, > void* data_) > return NULL_TREE; > } > > +/* Replace PLACEHOLDER_EXPRs in EXP with object OBJ. */ > + > tree > replace_placeholders (tree exp, tree obj, bool *seen_p) > { > tree *tp = &exp; > - replace_placeholders_t data = { obj, false }; > + replace_placeholders_t data; > + data.obj = obj; > + data.seen = false; > + data.target_expr_stack.create (0); > if (TREE_CODE (exp) == TARGET_EXPR) > - tp = &TARGET_EXPR_INITIAL (exp); > + { > + tp = &TARGET_EXPR_INITIAL (exp); > + data.target_expr_stack.safe_push (exp); > + } > cp_walk_tree (tp, replace_placeholders_r, &data, NULL); > + data.target_expr_stack.release (); > if (seen_p) > *seen_p = data.seen; > return exp; > > Marek
Marek