Message ID | YllValsRLUdLWaFx@tucnak |
---|---|
State | New |
Headers | show |
Series | c++: Fix up CONSTRUCTOR_PLACEHOLDER_BOUNDARY handling [PR105256] | expand |
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? > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2022-04-15 Jakub Jelinek <jakub@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 >
On Sun, Apr 17, 2022 at 6:24 PM Jason Merrill <jason@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 <jakub@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 > > >
On Mon, Apr 18, 2022 at 09:57:12AM -0400, Patrick Palka wrote: > > 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? > > 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. So is the patch ok for trunk then? Apparently it is also a recent regression on 11 branch (since Marek's r11-9711) when compiling firefox, ok for 11.3 as well? > > > 2022-04-15 Jakub Jelinek <jakub@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. Jakub
On Tue, Apr 19, 2022, 6:53 AM Jakub Jelinek <jakub@redhat.com> wrote: > On Mon, Apr 18, 2022 at 09:57:12AM -0400, Patrick Palka wrote: > > > 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? > > > > 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. > > So is the patch ok for trunk then? > Apparently it is also a recent regression on 11 branch (since Marek's > r11-9711) when compiling firefox, ok for 11.3 as well? > Ok for both. > > > 2022-04-15 Jakub Jelinek <jakub@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. > > Jakub > >
--- 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 = {}; +};