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

[Marek Polacek <polacek at redhat dot com>]

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