On Thu, Apr 28, 2022 at 10:12:04AM -0400, Patrick Palka wrote:
> 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.
Thanks for taking a look.
> (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..)
I actually considered doing it in get_nsdmi, but it's either too early (don't
have a TARGET_EXPR yet, therefore no object to refer to), or too late -- we're
just about to introduce <PLACEHOLDER_EXPR struct B>. I felt like it may be
best to replace the <PLACEHOLDER_EXPR struct A> at the earliest opportunity,
which if f_c_l.
Marek
