Message ID | 20200406190756.1798784-1-ppalka@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: Fix usage of CONSTRUCTOR_PLACEHOLDER_BOUNDARY inside array initializers [90996] | expand |
On 4/6/20 3:07 PM, Patrick Palka wrote: > This PR reports that since the introduction of the > CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag, we are sometimes failing to resolve > PLACEHOLDER_EXPRs inside array initializers that refer to some inner > constructor. In the testcase in the PR, we have as the initializer for "S c[];" > the following > > {{.a=(int &) &_ZGR1c_, .b={*(&<PLACEHOLDER_EXPR struct S>)->a}}} > > where CONSTRUCTOR_PLACEHOLDER_BOUNDARY is set on the second outermost > constructor. However, we pass the whole initializer to replace_placeholders in > store_init_value, and so due to the flag being set on the second outermost ctor > it avoids recursing into the innermost constructor and we fail to resolve the > PLACEHOLDER_EXPR within. > > To fix this, we could perhaps either call replace_placeholders in more places, > or we could change where we set CONSTRUCTOR_PLACEHOLDER_BOUNDARY. This patch > takes the latter approach -- when building up an array initializer, it bubbles > any CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag set on the element initializers up to > the array initializer. Doing so shouldn't create any new PLACEHOLDER_EXPR > resolution ambiguities because we don't deal with PLACEHOLDER_EXPRs of array > type in the frontend, as far as I can tell. Interesting. Yes, that sounds like it should work. > Does this look OK to comit after testing? Yes. Though I'm seeing "after testing" a lot; what testing are you doing before sending patches? > gcc/cp/ChangeLog: > > PR c++/90996 > * tree.c (replace_placeholders): Look through all handled components, > not just COMPONENT_REFs. > * typeck2.c (process_init_constructor_array): Propagate > CONSTRUCTOR_PLACEHOLDER_BOUNDARY up from each element initializer to > the array initializer. > > gcc/testsuite/ChangeLog: > > PR c++/90996 > * g++.dg/cpp1y/pr90996.C: New test. > --- > gcc/cp/tree.c | 2 +- > gcc/cp/typeck2.c | 18 ++++++++++++++++++ > gcc/testsuite/g++.dg/cpp1y/pr90996.C | 17 +++++++++++++++++ > 3 files changed, 36 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr90996.C > > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > index 5eb0dcd717a..d1192b7e094 100644 > --- a/gcc/cp/tree.c > +++ b/gcc/cp/tree.c > @@ -3247,7 +3247,7 @@ replace_placeholders (tree exp, tree obj, bool *seen_p /*= NULL*/) > > /* If the object isn't a (member of a) class, do nothing. */ > tree op0 = obj; > - while (TREE_CODE (op0) == COMPONENT_REF) > + while (handled_component_p (op0)) > op0 = TREE_OPERAND (op0, 0); > if (!CLASS_TYPE_P (strip_array_types (TREE_TYPE (op0)))) > return exp; > diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c > index cf1cb5ace66..fe844bc08bb 100644 > --- a/gcc/cp/typeck2.c > +++ b/gcc/cp/typeck2.c > @@ -1488,6 +1488,17 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, > = massage_init_elt (TREE_TYPE (type), ce->value, nested, flags, > complain); > > + if (TREE_CODE (ce->value) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value)) > + { > + /* Shift CONSTRUCTOR_PLACEHOLDER_BOUNDARY from the element initializer > + up to the array initializer, so that the call to > + replace_placeholders from store_init_value can resolve any > + PLACEHOLDER_EXPRs within this element initializer. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value) = 0; > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; > + } > + > gcc_checking_assert > (ce->value == error_mark_node > || (same_type_ignoring_top_level_qualifiers_p > @@ -1516,6 +1527,13 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, > /* The default zero-initialization is fine for us; don't > add anything to the CONSTRUCTOR. */ > next = NULL_TREE; > + else if (TREE_CODE (next) == CONSTRUCTOR > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) > + { > + /* As above. */ > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; > + } > } > else if (!zero_init_p (TREE_TYPE (type))) > next = build_zero_init (TREE_TYPE (type), > diff --git a/gcc/testsuite/g++.dg/cpp1y/pr90996.C b/gcc/testsuite/g++.dg/cpp1y/pr90996.C > new file mode 100644 > index 00000000000..780cbb4e3ac > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/pr90996.C > @@ -0,0 +1,17 @@ > +// PR c++/90996 > +// { dg-do compile { target c++14 } } > + > +struct S > +{ > + int &&a = 2; > + int b[1] {a}; > +}; > + > +S c[2][2] {{{5}}}; > + > +struct T > +{ > + S c[2][2] {{{7}}}; > +}; > + > +T d {}; >
On Mon, 6 Apr 2020, Jason Merrill wrote: > On 4/6/20 3:07 PM, Patrick Palka wrote: > > This PR reports that since the introduction of the > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag, we are sometimes failing to resolve > > PLACEHOLDER_EXPRs inside array initializers that refer to some inner > > constructor. In the testcase in the PR, we have as the initializer for "S > > c[];" > > the following > > > > {{.a=(int &) &_ZGR1c_, .b={*(&<PLACEHOLDER_EXPR struct S>)->a}}} > > > > where CONSTRUCTOR_PLACEHOLDER_BOUNDARY is set on the second outermost > > constructor. However, we pass the whole initializer to replace_placeholders > > in > > store_init_value, and so due to the flag being set on the second outermost > > ctor > > it avoids recursing into the innermost constructor and we fail to resolve > > the > > PLACEHOLDER_EXPR within. > > > > To fix this, we could perhaps either call replace_placeholders in more > > places, > > or we could change where we set CONSTRUCTOR_PLACEHOLDER_BOUNDARY. This > > patch > > takes the latter approach -- when building up an array initializer, it > > bubbles > > any CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag set on the element initializers up > > to > > the array initializer. Doing so shouldn't create any new PLACEHOLDER_EXPR > > resolution ambiguities because we don't deal with PLACEHOLDER_EXPRs of array > > type in the frontend, as far as I can tell. > > Interesting. Yes, that sounds like it should work. > > > Does this look OK to comit after testing? > > Yes. > > Though I'm seeing "after testing" a lot; what testing are you doing before > sending patches? Sorry for the sloppiness -- I should be writing "after a full bootstrap/regtest" instead of "after testing" because I do indeed do some testing before sending a patch. In particular, I usually run and inspect the outputs of make check RUNTESTFLAGS="dg.exp=*.C" make check RUNTESTFLAGS="old-deja.exp=*.C" make check RUNTESTFLAGS="conformance.exp=*ranges*" in a build tree that is configured with --disable-bootstrap, as a quick smoke test for the patch. Is this a sufficient amount of testing before sending a patch for review, or would you prefer that I do a full bootstrap/regtest beforehand? In any case, I'll make sure to clearly convey the amount of testing that was done and is remaining in future patch submissions. > > > gcc/cp/ChangeLog: > > > > PR c++/90996 > > * tree.c (replace_placeholders): Look through all handled components, > > not just COMPONENT_REFs. > > * typeck2.c (process_init_constructor_array): Propagate > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY up from each element initializer to > > the array initializer. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/90996 > > * g++.dg/cpp1y/pr90996.C: New test. > > --- > > gcc/cp/tree.c | 2 +- > > gcc/cp/typeck2.c | 18 ++++++++++++++++++ > > gcc/testsuite/g++.dg/cpp1y/pr90996.C | 17 +++++++++++++++++ > > 3 files changed, 36 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/pr90996.C > > > > diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c > > index 5eb0dcd717a..d1192b7e094 100644 > > --- a/gcc/cp/tree.c > > +++ b/gcc/cp/tree.c > > @@ -3247,7 +3247,7 @@ replace_placeholders (tree exp, tree obj, bool *seen_p > > /*= NULL*/) > > /* If the object isn't a (member of a) class, do nothing. */ > > tree op0 = obj; > > - while (TREE_CODE (op0) == COMPONENT_REF) > > + while (handled_component_p (op0)) > > op0 = TREE_OPERAND (op0, 0); > > if (!CLASS_TYPE_P (strip_array_types (TREE_TYPE (op0)))) > > return exp; > > diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c > > index cf1cb5ace66..fe844bc08bb 100644 > > --- a/gcc/cp/typeck2.c > > +++ b/gcc/cp/typeck2.c > > @@ -1488,6 +1488,17 @@ process_init_constructor_array (tree type, tree init, > > int nested, int flags, > > = massage_init_elt (TREE_TYPE (type), ce->value, nested, flags, > > complain); > > + if (TREE_CODE (ce->value) == CONSTRUCTOR > > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value)) > > + { > > + /* Shift CONSTRUCTOR_PLACEHOLDER_BOUNDARY from the element > > initializer > > + up to the array initializer, so that the call to > > + replace_placeholders from store_init_value can resolve any > > + PLACEHOLDER_EXPRs within this element initializer. */ > > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value) = 0; > > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; > > + } > > + > > gcc_checking_assert > > (ce->value == error_mark_node > > || (same_type_ignoring_top_level_qualifiers_p > > @@ -1516,6 +1527,13 @@ process_init_constructor_array (tree type, tree init, > > int nested, int flags, > > /* The default zero-initialization is fine for us; don't > > add anything to the CONSTRUCTOR. */ > > next = NULL_TREE; > > + else if (TREE_CODE (next) == CONSTRUCTOR > > + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) > > + { > > + /* As above. */ > > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; > > + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; > > + } > > } > > else if (!zero_init_p (TREE_TYPE (type))) > > next = build_zero_init (TREE_TYPE (type), > > diff --git a/gcc/testsuite/g++.dg/cpp1y/pr90996.C > > b/gcc/testsuite/g++.dg/cpp1y/pr90996.C > > new file mode 100644 > > index 00000000000..780cbb4e3ac > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/pr90996.C > > @@ -0,0 +1,17 @@ > > +// PR c++/90996 > > +// { dg-do compile { target c++14 } } > > + > > +struct S > > +{ > > + int &&a = 2; > > + int b[1] {a}; > > +}; > > + > > +S c[2][2] {{{5}}}; > > + > > +struct T > > +{ > > + S c[2][2] {{{7}}}; > > +}; > > + > > +T d {}; > > > >
On 4/6/20 6:22 PM, Patrick Palka wrote: > On Mon, 6 Apr 2020, Jason Merrill wrote: > >> On 4/6/20 3:07 PM, Patrick Palka wrote: >>> This PR reports that since the introduction of the >>> CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag, we are sometimes failing to resolve >>> PLACEHOLDER_EXPRs inside array initializers that refer to some inner >>> constructor. In the testcase in the PR, we have as the initializer for "S >>> c[];" >>> the following >>> >>> {{.a=(int &) &_ZGR1c_, .b={*(&<PLACEHOLDER_EXPR struct S>)->a}}} >>> >>> where CONSTRUCTOR_PLACEHOLDER_BOUNDARY is set on the second outermost >>> constructor. However, we pass the whole initializer to replace_placeholders >>> in >>> store_init_value, and so due to the flag being set on the second outermost >>> ctor >>> it avoids recursing into the innermost constructor and we fail to resolve >>> the >>> PLACEHOLDER_EXPR within. >>> >>> To fix this, we could perhaps either call replace_placeholders in more >>> places, >>> or we could change where we set CONSTRUCTOR_PLACEHOLDER_BOUNDARY. This >>> patch >>> takes the latter approach -- when building up an array initializer, it >>> bubbles >>> any CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag set on the element initializers up >>> to >>> the array initializer. Doing so shouldn't create any new PLACEHOLDER_EXPR >>> resolution ambiguities because we don't deal with PLACEHOLDER_EXPRs of array >>> type in the frontend, as far as I can tell. >> >> Interesting. Yes, that sounds like it should work. >> >>> Does this look OK to comit after testing? >> >> Yes. >> >> Though I'm seeing "after testing" a lot; what testing are you doing before >> sending patches? > > Sorry for the sloppiness -- I should be writing "after a full > bootstrap/regtest" instead of "after testing" because I do indeed do > some testing before sending a patch. In particular, I usually run and > inspect the outputs of > > make check RUNTESTFLAGS="dg.exp=*.C" > make check RUNTESTFLAGS="old-deja.exp=*.C" > make check RUNTESTFLAGS="conformance.exp=*ranges*" > > in a build tree that is configured with --disable-bootstrap, as a quick > smoke test for the patch. Is this a sufficient amount of testing before > sending a patch for review, or would you prefer that I do a full > bootstrap/regtest beforehand? You don't need to do a full bootstrap and run non-C++ testsuites, but please run the full libstdc++ testsuite. Is there a reason you aren't using 'make check-c++'? > In any case, I'll make sure to clearly convey the amount of testing that > was done and is remaining in future patch submissions. Thanks. Jason
On Wed, 8 Apr 2020, Jason Merrill wrote: > On 4/6/20 6:22 PM, Patrick Palka wrote: > > On Mon, 6 Apr 2020, Jason Merrill wrote: > > > > > On 4/6/20 3:07 PM, Patrick Palka wrote: > > > > This PR reports that since the introduction of the > > > > CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag, we are sometimes failing to > > > > resolve > > > > PLACEHOLDER_EXPRs inside array initializers that refer to some inner > > > > constructor. In the testcase in the PR, we have as the initializer for > > > > "S > > > > c[];" > > > > the following > > > > > > > > {{.a=(int &) &_ZGR1c_, .b={*(&<PLACEHOLDER_EXPR struct S>)->a}}} > > > > > > > > where CONSTRUCTOR_PLACEHOLDER_BOUNDARY is set on the second outermost > > > > constructor. However, we pass the whole initializer to > > > > replace_placeholders > > > > in > > > > store_init_value, and so due to the flag being set on the second > > > > outermost > > > > ctor > > > > it avoids recursing into the innermost constructor and we fail to > > > > resolve > > > > the > > > > PLACEHOLDER_EXPR within. > > > > > > > > To fix this, we could perhaps either call replace_placeholders in more > > > > places, > > > > or we could change where we set CONSTRUCTOR_PLACEHOLDER_BOUNDARY. This > > > > patch > > > > takes the latter approach -- when building up an array initializer, it > > > > bubbles > > > > any CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag set on the element > > > > initializers up > > > > to > > > > the array initializer. Doing so shouldn't create any new > > > > PLACEHOLDER_EXPR > > > > resolution ambiguities because we don't deal with PLACEHOLDER_EXPRs of > > > > array > > > > type in the frontend, as far as I can tell. > > > > > > Interesting. Yes, that sounds like it should work. > > > > > > > Does this look OK to comit after testing? > > > > > > Yes. > > > > > > Though I'm seeing "after testing" a lot; what testing are you doing before > > > sending patches? > > > > Sorry for the sloppiness -- I should be writing "after a full > > bootstrap/regtest" instead of "after testing" because I do indeed do > > some testing before sending a patch. In particular, I usually run and > > inspect the outputs of > > > > make check RUNTESTFLAGS="dg.exp=*.C" > > make check RUNTESTFLAGS="old-deja.exp=*.C" > > make check RUNTESTFLAGS="conformance.exp=*ranges*" > > > > in a build tree that is configured with --disable-bootstrap, as a quick > > smoke test for the patch. Is this a sufficient amount of testing before > > sending a patch for review, or would you prefer that I do a full > > bootstrap/regtest beforehand? > > You don't need to do a full bootstrap and run non-C++ testsuites, but please > run the full libstdc++ testsuite. > > Is there a reason you aren't using 'make check-c++'? No good reason, I didn't know about "make check-c++" :) Good to know, thanks! > > > In any case, I'll make sure to clearly convey the amount of testing that > > was done and is remaining in future patch submissions. > > Thanks. > > Jason > >
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index 5eb0dcd717a..d1192b7e094 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -3247,7 +3247,7 @@ replace_placeholders (tree exp, tree obj, bool *seen_p /*= NULL*/) /* If the object isn't a (member of a) class, do nothing. */ tree op0 = obj; - while (TREE_CODE (op0) == COMPONENT_REF) + while (handled_component_p (op0)) op0 = TREE_OPERAND (op0, 0); if (!CLASS_TYPE_P (strip_array_types (TREE_TYPE (op0)))) return exp; diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c index cf1cb5ace66..fe844bc08bb 100644 --- a/gcc/cp/typeck2.c +++ b/gcc/cp/typeck2.c @@ -1488,6 +1488,17 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, = massage_init_elt (TREE_TYPE (type), ce->value, nested, flags, complain); + if (TREE_CODE (ce->value) == CONSTRUCTOR + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value)) + { + /* Shift CONSTRUCTOR_PLACEHOLDER_BOUNDARY from the element initializer + up to the array initializer, so that the call to + replace_placeholders from store_init_value can resolve any + PLACEHOLDER_EXPRs within this element initializer. */ + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (ce->value) = 0; + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; + } + gcc_checking_assert (ce->value == error_mark_node || (same_type_ignoring_top_level_qualifiers_p @@ -1516,6 +1527,13 @@ process_init_constructor_array (tree type, tree init, int nested, int flags, /* The default zero-initialization is fine for us; don't add anything to the CONSTRUCTOR. */ next = NULL_TREE; + else if (TREE_CODE (next) == CONSTRUCTOR + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next)) + { + /* As above. */ + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (next) = 0; + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = 1; + } } else if (!zero_init_p (TREE_TYPE (type))) next = build_zero_init (TREE_TYPE (type), diff --git a/gcc/testsuite/g++.dg/cpp1y/pr90996.C b/gcc/testsuite/g++.dg/cpp1y/pr90996.C new file mode 100644 index 00000000000..780cbb4e3ac --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp1y/pr90996.C @@ -0,0 +1,17 @@ +// PR c++/90996 +// { dg-do compile { target c++14 } } + +struct S +{ + int &&a = 2; + int b[1] {a}; +}; + +S c[2][2] {{{5}}}; + +struct T +{ + S c[2][2] {{{7}}}; +}; + +T d {};