Re: [PATCH v2] c++: ICE with temporary of class type in DMI [PR100252]

[Patrick Palka <ppalka@redhat.com>]

On Wed, 27 Apr 2022, Marek Polacek wrote:

> On Wed, Apr 27, 2022 at 11:00:46AM -0400, Patrick Palka wrote:
> > On Tue, 26 Apr 2022, Marek Polacek wrote:
> > 
> > > Consider
> > > 
> > >   struct A {
> > >     int x;
> > >     int y = x;
> > >   };
> > > 
> > >   struct B {
> > >     int x = 0;
> > >     int y = A{x}.y; // #1
> > >   };
> > > 
> > > where for #1 we end up with
> > > 
> > >   {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
> > > 
> > > that is, two PLACEHOLDER_EXPRs for different types on the same level in
> > > a {}.  This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to
> > > avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it.
> > > 
> > > Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing
> > > cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal
> > > on type=A, compound_literal={((struct B *) this)->x}.  When digesting this
> > > initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't
> > > have any object to refer to yet.  After digesting, we have
> > > 
> > >   {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
> > > 
> > > and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor
> > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY.  f_c_l creates a TARGET_EXPR and returns
> > > 
> > >   TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>
> > > 
> > > Then we get to
> > > 
> > >   B b = {};
> > > 
> > > and call store_init_value, which digest the {}, which produces
> > > 
> > >   {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y}
> > > 
> > > The call to replace_placeholders in store_init_value will not do anything:
> > > we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only
> > > a sub-expression, so replace_placeholders does nothing, so the <P_E struct B>
> > > stays even though now is the perfect time to replace it because we have an
> > > object for it: 'b'.
> > > 
> > > Later, in cp_gimplify_init_expr the *expr_p is
> > > 
> > >   D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
> > > 
> > > where D.2395 is of type A, but we crash because we hit <P_E struct B>, which
> > > has a different type.
> > > 
> > > My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the
> > > TARGET_EXPR because that means we have an object we can refer to.  Then clear
> > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR
> > > in the {}.  Then store_init_value will be able to replace <P_E struct B> with
> > > 'b', and we should be good to go.
> > 
> > Makes sense to me.  It seems all was well until break_out_target_exprs,
> > called from get_nsdmi for B::y, replaced the 'this' in the initializer
> > 
> >   (TARGET_EXPR <D.2131, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y;
> > 
> > with a PLACEHOLDER_EXPR;
> > 
> >   (TARGET_EXPR <D.2142, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y;
> > 
> > This seems to be the wrong thing to do when the 'this' appears inside a
> > CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructor because the new
> > PLACEHOLDER_EXPR then can't be resolved correctly.
> 
> Exactly.
> 
> > So in light of this I wonder if we should instead perform this handling
> > you added to finish_compound_literal in break_out_target_exprs /
> > bot_manip instead?
> 
> Unfortunately that causes an ICE in gimplify_var_or_parm_decl on the new
> testcase I've added here.  bot_manip is a different context and so I can't
> use parsing_nsdmi anymore, and it seems we'd replace the placeholders too
> aggressively in bot_manip.  So I'm not sure if that's the best place.

Ah :/ good catch...

FWIW the transformation itself (doing replace_placeholders followed by
clearing CONSTRUCTOR_PLACEHOLDER_BOUNDARY) makes sense to me, and I'm
happy with doing it in finish_compound_literal when parsing_nsdmi, so
the patch LGTM.

(I guess we could also consider doing the transformation in get_nsdmi or
digest_nsdmi_init via cp_walk_tree, but I don't have a preference either way..)

> 
> -- >8 --
> Consider
> 
>   struct A {
>     int x;
>     int y = x;
>   };
> 
>   struct B {
>     int x = 0;
>     int y = A{x}.y; // #1
>   };
> 
> where for #1 we end up with
> 
>   {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
> 
> that is, two PLACEHOLDER_EXPRs for different types on the same level in
> a {}.  This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to
> avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it.
> 
> Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing
> cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal
> on type=A, compound_literal={((struct B *) this)->x}.  When digesting this
> initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't
> have any object to refer to yet.  After digesting, we have
> 
>   {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
> 
> and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor
> CONSTRUCTOR_PLACEHOLDER_BOUNDARY.  f_c_l creates a TARGET_EXPR and returns
> 
>   TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>
> 
> Then we get to
> 
>   B b = {};
> 
> and call store_init_value, which digest the {}, which produces
> 
>   {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y}
> 
> The call to replace_placeholders in store_init_value will not do anything:
> we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only
> a sub-expression, so replace_placeholders does nothing, so the <P_E struct B>
> stays even though now is the perfect time to replace it because we have an
> object for it: 'b'.
> 
> Later, in cp_gimplify_init_expr the *expr_p is
> 
>   D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
> 
> where D.2395 is of type A, but we crash because we hit <P_E struct B>, which
> has a different type.
> 
> My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the
> TARGET_EXPR because that means we have an object we can refer to.  Then clear
> CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR
> in the {}.  Then store_init_value will be able to replace <P_E struct B> with
> 'b', and we should be good to go.
> 
> 	PR c++/100252
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.cc (finish_compound_literal): replace_placeholders after
> 	creating the TARGET_EXPR.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp1y/nsdmi-aggr14.C: New test.
> 	* g++.dg/cpp1y/nsdmi-aggr15.C: New test.
> ---
>  gcc/cp/semantics.cc                       | 31 +++++++++++++++
>  gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++++++
>  gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++++
>  3 files changed, 106 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
>  create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index ab48f11c9be..770369458bb 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -3296,6 +3296,37 @@ finish_compound_literal (tree type, tree compound_literal,
>        if (TREE_CODE (compound_literal) == CONSTRUCTOR)
>  	TREE_HAS_CONSTRUCTOR (compound_literal) = false;
>        compound_literal = get_target_expr_sfinae (compound_literal, complain);
> +      /* We may have A{} in a NSDMI.  */
> +      if (parsing_nsdmi ())
> +	{
> +	  /* Digesting the {} could have introduced a PLACEHOLDER_EXPR
> +	     referring to A.  Now that we've built up a TARGET_EXPR, we
> +	     have an object we can refer to.  The reason we bother doing
> +	     this here is for code like
> +
> +	       struct A {
> +		 int x;
> +		 int y = x;
> +	       };
> +
> +	       struct B {
> +		 int x = 0;
> +		 int y = A{x}.y; // #1
> +	       };
> +
> +	     where in #1 we don't want to end up with two PLACEHOLDER_EXPRs
> +	     for different types on the same level in a {} as in 100252.  */
> +	  tree init = TARGET_EXPR_INITIAL (compound_literal);
> +	  if (TREE_CODE (init) == CONSTRUCTOR
> +	      && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init))
> +	    {
> +	      tree obj = TARGET_EXPR_SLOT (compound_literal);
> +	      replace_placeholders (compound_literal, obj);
> +	      /* We should have dealt with the PLACEHOLDER_EXPRs.  */
> +	      CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false;
> +	      gcc_checking_assert (!find_placeholders (init));
> +	    }
> +	}
>      }
>    else
>      /* For e.g. int{42} just make sure it's a prvalue.  */
> diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
> new file mode 100644
> index 00000000000..7d508f52b48
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
> @@ -0,0 +1,46 @@
> +// PR c++/100252
> +// { dg-do run { target c++14 } }
> +
> +#define SA(X) static_assert ((X),#X)
> +
> +struct A {
> +  int x;
> +  int y = x;
> +};
> +
> +struct B {
> +  int x = 0;
> +  int y = A{x}.y;
> +};
> +
> +constexpr B csb1 = { };
> +SA(csb1.x == 0 && csb1.y == csb1.x);
> +constexpr B csb2 = { 1 };
> +SA(csb2.x == 1 && csb2.y == csb2.x);
> +constexpr B csb3 = { 1, 2 };
> +SA(csb3.x == 1 && csb3.y == 2);
> +
> +B sb1 = { };
> +B sb2 = { 1 };
> +B sb3 = { 1, 2};
> +
> +int
> +main ()
> +{
> +  if (sb1.x != 0 || sb1.x != sb1.y)
> +    __builtin_abort();
> +  if (sb2.x != 1 || sb2.x != sb2.y)
> +    __builtin_abort();
> +  if (sb3.x != 1 || sb3.y != 2)
> +    __builtin_abort();
> +
> +  B b1 = { };
> +  B b2 = { 1 };
> +  B b3 = { 1, 2};
> +  if (b1.x != 0 || b1.x != b1.y)
> +    __builtin_abort();
> +  if (b2.x != 1 || b2.x != b2.y)
> +    __builtin_abort();
> +  if (b3.x != 1 || b3.y != 2)
> +    __builtin_abort();
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C
> new file mode 100644
> index 00000000000..bc997bb5e1d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C
> @@ -0,0 +1,29 @@
> +// PR c++/100252
> +// { dg-do run { target c++14 } }
> +
> +struct A {
> +  int x;
> +  int y = x;
> +};
> +
> +struct B {
> +  int x = 0;
> +  int y = A{x}.y;
> +};
> +
> +void
> +g (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2})
> +{
> +  if (b1.x != 0 || b1.y != b1.x)
> +    __builtin_abort();
> +  if (b2.x != 1 || b2.y != b2.x)
> +    __builtin_abort();
> +  if (b3.x != 1 || b3.y != 2)
> +    __builtin_abort();
> +}
> +
> +int
> +main ()
> +{
> +  g ();
> +}
> 
> base-commit: 409edcca331296b53842c50d3b789e1b1ccc05e5
> -- 
> 2.35.1
> 
>