[C++ PATCH] Fix PLACEHOLDER_EXPR handling (PR c++/79937, PR c++/82410)

[Jason Merrill]

On Thu, Mar 15, 2018 at 1:59 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 15, 2018 at 01:15:53PM -0400, Jason Merrill wrote:
>> > g++.dg/cpp0x/nsdmi13.C ICEs without that, we have there:
>> > a = A({});
>> > and build_over_call does:
>> > 8163          else if (tree_int_cst_equal (TYPE_SIZE (type), TYPE_SIZE (as_base)))
>> > 8164            {
>> > 8165              arg = cp_build_fold_indirect_ref (arg);
>> > 8166              val = build2 (MODIFY_EXPR, TREE_TYPE (to), to, arg);
>> > 8167              /* Handle NSDMI that refer to the object being initialized.  */
>> > 8168              replace_placeholders (arg, to);
>> > 8169            }
>> > where arg is after the cp_build_fold_indirect_ref
>> > *(struct A &) &TARGET_EXPR <D.2403, {.p=&<PLACEHOLDER_EXPR struct A>}>
>> > This is in MODIFY_EXPR rather than INIT_EXPR and the gimplifier through
>> > gimple_fold_indirect_ref_rhs folds the *(struct A &) & away and so there is
>> > no further temporary and thus cp_gimplify_init_expr isn't called to
>> > replace_placeholders, so if we don't replace it here (with to being the a
>> > VAR_DECL), we don't replace it ever.
>>
>> Ah.  That's a problem: the language says there's a temporary, so after
>> the assignment a.p should not point to a.
>
> Seems the gimplify.c stuff:
>         case INDIRECT_REF:
>           {
>             /* If we have code like
>
>              *(const A*)(A*)&x
>
>              where the type of "x" is a (possibly cv-qualified variant
>              of "A"), treat the entire expression as identical to "x".
>              This kind of code arises in C++ when an object is bound
>              to a const reference, and if "x" is a TARGET_EXPR we want
>              to take advantage of the optimization below.  */
> ...
> has been added in r92539 as part of PR16405 fix.  So, do we want to stop
> doing that unconditionally if t is a TARGET_EXPR, or for selected kinds of

Folding away the INDIRECT_REF is fine.  It's the TARGET_EXPR handling
that is wrong.

> types of TARGET_EXPR, or ask some langhook whether it is ok to do so
> (say not ok if find_placeholders (t))?  Or contains_placeholder_p?
> Though the last one could also affect Ada and could return true even if
> the PLACEHOLDER_EXPRs are for some nested TARGET_EXPR in it.

The existing test for whether to collapse a TARGET_EXPR is

            if (init
                && !VOID_TYPE_P (TREE_TYPE (init)))

so we need this test to fail somehow when the constructor contains
placeholders, either by adding an additional test (like the ones you
mention) or by making the initialization void in a way that other
gimplification knows how to handle.

>> So the existing call to replace_placeholders in build_over_call is wrong.
>
> Shall it be just dropped if we tweak the gimplifier somehow?

It must be dropped.

>> Seems like if the initializer for a TARGET_EXPR involves
>> PLACEHOLDER_EXPR, the gimple_fold_modify_expr_rhs treatment of
>> TARGET_EXPR isn't safe for MODIFY_EXPR, for the same reason that
>> cp_gimplify_expr treats INIT_EXPR and MODIFY_EXPR differently.