| Message ID | 20170324162200.GA3172@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <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... > > 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
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. Jason
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;