On Sun, Apr 17, 2022 at 6:24 PM Jason Merrill <ja...@redhat.com> wrote:
>
> On 4/15/22 07:22, Jakub Jelinek wrote:
> > Hi!
> >
> > The CONSTRUCTOR_PLACEHOLDER_BOUNDARY bit is supposed to separate
> > PLACEHOLDER_EXPRs that should be replaced by one object or subobjects of it
> > (variable, TARGET_EXPR slot, ...) from other PLACEHOLDER_EXPRs that should
> > be replaced by different objects or subobjects.
> > The bit is set when finding PLACEHOLDER_EXPRs inside of a CONSTRUCTOR, not
> > looking into nested CONSTRUCTOR_PLACEHOLDER_BOUNDARY ctors, and we prevent
> > elision of TARGET_EXPRs (through TARGET_EXPR_NO_ELIDE) whose initializer
> > is a CONSTRUCTOR_PLACEHOLDER_BOUNDARY ctor. The following testcase ICEs
> > though, we don't replace the placeholders in there at all, because
> > CONSTRUCTOR_PLACEHOLDER_BOUNDARY isn't set on the TARGET_EXPR_INITIAL
> > ctor, but on a ctor nested in such a ctor. replace_placeholders should be
> > run on the whole TARGET_EXPR slot.
> >
> > So, the following patch fixes it by moving the
> > CONSTRUCTOR_PLACEHOLDER_BOUNDARY
> > bit from nested CONSTRUCTORs to the CONSTRUCTOR containing those (but only
> > if it is closely nested, if there is some other tree sandwiched in between,
> > it doesn't do it).
>
> Hmm, Patrick made a similar change and then reverted it for PR90996.
> But it makes sense to me; when we replace placeholders, it's appropriate
> to look at the whole aggregate initialization rather than the innermost
> CONSTRUCTOR that has DMIs. Patrick, was there a reason that change
> seemed wrong to you, or was it just unnecessary for the bug you were
> working on?
That change was just not strictly necessary for PR90996 I think. For
that testcase:
struct S {
int &&a = 2;
int b[1] {a};
};
S c[] {{2}};
we first call replace_placeholders from store_init_value for the
overall initiializer for c
{{.a=(int &) &_ZGR1c_, .b={*(&<PLACEHOLDER_EXPR struct S>)->a}}
but CONSTRUCTOR_PLACEHOLDER_BOUNDARY set on the middle pair of {}
prevents the placeholder from being resolved to c[0]. The reverted
change would have allowed us to resolve the placeholder at this point,
since the flag would have been moved to the outermost {} IIUC.
But fortunately split_nonconstant_init then factors out the
initialization of c[0].b into an INIT_EXPR:
c[0].b[0] = *(&<PLACEHOLDER_EXPR struct S>)->a;
during gimplification of which we attempt replace_placeholders again
which succeeds this time at resolving the placeholder, due to the
other change in r10-7587 that made replace_placeholders look through
all handled components, not just COMPONENT_REF (mirroring
replace_placeholders_r).
So I reverted the rest of r10-7587 after Martin noticed it had no
effect: https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543918.html
The reverted change and Jakub's more general patch seem right/safe to
me FWIW, I just couldn't come up with a testcase that demonstrated its
need at the time unfortunately.
>
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > 2022-04-15 Jakub Jelinek <ja...@redhat.com>
> >
> > PR c++/105256
> > * typeck2.cc (process_init_constructor_array,
> > process_init_constructor_record, process_init_constructor_union): Move
> > CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag from CONSTRUCTOR elements to the
> > containing CONSTRUCTOR.
> >
> > * g++.dg/cpp0x/pr105256.C: New test.
> >
> > --- gcc/cp/typeck2.cc.jj 2022-04-07 09:09:54.432995137 +0200
> > +++ gcc/cp/typeck2.cc 2022-04-14 16:02:12.438432494 +0200
> > @@ -1515,6 +1515,14 @@ process_init_constructor_array (tree typ
> > strip_array_types (TREE_TYPE (ce->value)))));
> >
> > picflags |= picflag_from_initializer (ce->value);
> > + /* Propagate CONSTRUCTOR_PLACEHOLDER_BOUNDARY to outer
> > + CONSTRUCTOR. */
> > + if (TREE_CODE (ce->value) == CONSTRUCTOR
> > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value))
> > + {
> > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1;
> > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value) = 0;
> > + }
> > }
> >
> > /* No more initializers. If the array is unbounded, we are done.
> > Otherwise,
> > @@ -1560,6 +1568,14 @@ process_init_constructor_array (tree typ
> > }
> >
> > picflags |= picflag_from_initializer (next);
> > + /* Propagate CONSTRUCTOR_PLACEHOLDER_BOUNDARY to outer
> > + CONSTRUCTOR. */
> > + if (TREE_CODE (next) == CONSTRUCTOR
> > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next))
> > + {
> > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1;
> > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0;
> > + }
> > if (len > i+1)
> > {
> > tree range = build2 (RANGE_EXPR, size_type_node,
> > @@ -1754,6 +1770,13 @@ process_init_constructor_record (tree ty
> > if (fldtype != TREE_TYPE (field))
> > next = cp_convert_and_check (TREE_TYPE (field), next, complain);
> > picflags |= picflag_from_initializer (next);
> > + /* Propagate CONSTRUCTOR_PLACEHOLDER_BOUNDARY to outer CONSTRUCTOR.
> > */
> > + if (TREE_CODE (next) == CONSTRUCTOR
> > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next))
> > + {
> > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1;
> > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0;
> > + }
> > CONSTRUCTOR_APPEND_ELT (v, field, next);
> > }
> >
> > @@ -1894,6 +1917,14 @@ process_init_constructor_union (tree typ
> > ce->value = massage_init_elt (TREE_TYPE (ce->index), ce->value,
> > nested,
> > flags, complain);
> >
> > + /* Propagate CONSTRUCTOR_PLACEHOLDER_BOUNDARY to outer CONSTRUCTOR. */
> > + if (ce->value
> > + && TREE_CODE (ce->value) == CONSTRUCTOR
> > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value))
> > + {
> > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1;
> > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value) = 0;
> > + }
> > return picflag_from_initializer (ce->value);
> > }
> >
> > --- gcc/testsuite/g++.dg/cpp0x/pr105256.C.jj 2022-04-14 16:04:30.518518875
> > +0200
> > +++ gcc/testsuite/g++.dg/cpp0x/pr105256.C 2022-04-14 16:03:53.264035175
> > +0200
> > @@ -0,0 +1,18 @@
> > +// PR c++/105256
> > +// { dg-do compile { target c++11 } }
> > +
> > +int bar (int &);
> > +
> > +struct S {
> > + struct T {
> > + struct U {
> > + int i = bar (i);
> > + } u;
> > + };
> > +};
> > +
> > +void
> > +foo (S::T *p)
> > +{
> > + *p = {};
> > +};
> >
> > Jakub
> >
>
