Re: [PATCH v3] c++: ICE with temporary of class type in array DMI [PR109966]

[Jason Merrill <jason@redhat.com>]

On 3/14/24 17:26, Marek Polacek wrote:
> 
> In the following patch, I'm taking a different tack.  I believe
> we ought to use TARGET_EXPR_ELIDING_P.  The gimplify_arg bit I'm
> talking about below is this:
> 
>        /* Also strip a TARGET_EXPR that would force an extra copy.  */
>        if (TREE_CODE (*arg_p) == TARGET_EXPR)
>          {
>            tree init = TARGET_EXPR_INITIAL (*arg_p);
>            if (init
>                && !VOID_TYPE_P (TREE_TYPE (init)))
>              *arg_p = init;
>          }
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/13?
> 
> -- >8 --
> This ICE started with the fairly complicated r13-765.  We crash in
> gimplify_var_or_parm_decl because a stray VAR_DECL leaked there.
> The problem is ultimately that potential_prvalue_result_of wasn't
> correctly handling arrays and replace_placeholders_for_class_temp_r
> replaced a PLACEHOLDER_EXPR in a TARGET_EXPR which is used in the
> context of copy elision.  If I have
> 
>    M m[2] = { M{""}, M{""} };
> 
> then we don't invoke the M(const M&) copy-ctor.
> 
> One part of the fix is to use TARGET_EXPR_ELIDING_P rather than
> potential_prvalue_result_of.  That unfortunately doesn't handle the
> case like
> 
>    struct N { N(M); };
>    N arr[2] = { M{""}, M{""} };
> 
> because TARGET_EXPRs that initialize a function argument are not
> marked TARGET_EXPR_ELIDING_P even though gimplify_arg drops such
> TARGET_EXPRs on the floor.  We can use a pset to avoid replacing
> placeholders in them.
> 
> I made an attempt to use set_target_expr_eliding in
> convert_for_arg_passing but that regressed constexpr-diag1.C, and does
> not seem like a prudent change in stage 4 anyway.

I tried the same thing to see what you mean, and that doesn't look like 
a regression to me, just a different (and more accurate) diagnostic.

But you're right that this patch is safer, and the other approach can 
wait for stage 1.  Will you queue that up?  In the mean time, this patch 
is OK.

> I just realized this could also check !TARGET_EXPR_ELIDING_P; there's no point
> to adding an eliding TARGET_EXPR into the pset.

...with this change.

Jason