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
