* [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] @ 2023-03-23 20:35 Jason Merrill 2023-03-23 21:03 ` Jakub Jelinek 0 siblings, 1 reply; 5+ messages in thread From: Jason Merrill @ 2023-03-23 20:35 UTC (permalink / raw) To: gcc-patches, Jakub Jelinek Tested x86_64-pc-linux-gnu. Jakub, does this make sense to you? Do we have a way of testing for compile-hog regressions? -- 8< -- The patch for PR91415 fixed -Wsequence-point to treat shifts and ARRAY_REF as sequenced in C++17, and COMPONENT_REF as well. But this is unnecessary for COMPONENT_REF, since the RHS is just a FIELD_DECL with no actual evaluation, and in this testcase handling COMPONENT_REF as sequenced blows up fast in a deep inheritance tree. PR c++/107163 gcc/c-family/ChangeLog: * c-common.cc (verify_tree): Don't use sequenced handling for COMPONENT_REF. --- gcc/c-family/c-common.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc index bfb950e56db..a803cf94c68 100644 --- a/gcc/c-family/c-common.cc +++ b/gcc/c-family/c-common.cc @@ -2154,7 +2154,6 @@ verify_tree (tree x, struct tlist **pbefore_sp, struct tlist **pno_sp, case LSHIFT_EXPR: case RSHIFT_EXPR: - case COMPONENT_REF: case ARRAY_REF: if (cxx_dialect >= cxx17) goto sequenced_binary; base-commit: 4872e46e080c6695dfe1f9dc9db26b4703bc348c -- 2.31.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] 2023-03-23 20:35 [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] Jason Merrill @ 2023-03-23 21:03 ` Jakub Jelinek 2023-03-24 22:11 ` Jason Merrill 0 siblings, 1 reply; 5+ messages in thread From: Jakub Jelinek @ 2023-03-23 21:03 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches On Thu, Mar 23, 2023 at 04:35:07PM -0400, Jason Merrill wrote: > Tested x86_64-pc-linux-gnu. Jakub, does this make sense to you? Do we have a > way of testing for compile-hog regressions? > > -- 8< -- > > The patch for PR91415 fixed -Wsequence-point to treat shifts and ARRAY_REF > as sequenced in C++17, and COMPONENT_REF as well. But this is unnecessary > for COMPONENT_REF, since the RHS is just a FIELD_DECL with no actual > evaluation, and in this testcase handling COMPONENT_REF as sequenced blows > up fast in a deep inheritance tree. > > PR c++/107163 > > gcc/c-family/ChangeLog: > > * c-common.cc (verify_tree): Don't use sequenced handling > for COMPONENT_REF. When we touch this for COMPONENT_REF, shouldn't we then handle it as unary given that the second operand is FIELD_DECL and third/fourth will likely be NULL and even if not, aren't user expressions that should be inspected? So, instead of doing this do: case COMPONENT_REF: x = TREE_OPERAND (x, 0); writer = 0; goto restart; ? As for compile-hog, depends on how long it will take it to compile before fix/after fix. If before fix can be above the normal timeout on reasonably fast matchines and after fix can take a few seconds, great, if after fix would take longer but still not horribly long, one way to do it is guard the test with run_expensive_tests effective target. Or another way is have the test smaller in complexity normally and // { dg-additional-options "-DEXPENSIVE" { target run_expensive_tests } } and #ifdef EXPENSIVE make it more complex. Jakub ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] 2023-03-23 21:03 ` Jakub Jelinek @ 2023-03-24 22:11 ` Jason Merrill 2023-03-24 22:25 ` Jakub Jelinek 0 siblings, 1 reply; 5+ messages in thread From: Jason Merrill @ 2023-03-24 22:11 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 2082 bytes --] On 3/23/23 17:03, Jakub Jelinek wrote: > On Thu, Mar 23, 2023 at 04:35:07PM -0400, Jason Merrill wrote: >> Tested x86_64-pc-linux-gnu. Jakub, does this make sense to you? Do we have a >> way of testing for compile-hog regressions? >> >> -- 8< -- >> >> The patch for PR91415 fixed -Wsequence-point to treat shifts and ARRAY_REF >> as sequenced in C++17, and COMPONENT_REF as well. But this is unnecessary >> for COMPONENT_REF, since the RHS is just a FIELD_DECL with no actual >> evaluation, and in this testcase handling COMPONENT_REF as sequenced blows >> up fast in a deep inheritance tree. >> >> PR c++/107163 >> >> gcc/c-family/ChangeLog: >> >> * c-common.cc (verify_tree): Don't use sequenced handling >> for COMPONENT_REF. > > When we touch this for COMPONENT_REF, shouldn't we then handle it as > unary given that the second operand is FIELD_DECL and third/fourth > will likely be NULL and even if not, aren't user expressions that should be > inspected? > So, instead of doing this do: > case COMPONENT_REF: > x = TREE_OPERAND (x, 0); > writer = 0; > goto restart; > ? Is clearing 'writer' what we want, since an access to COMPONENT_REF is an access to (a subobject of) its op0? > As for compile-hog, depends on how long it will take it to compile before > fix/after fix. If before fix can be above the normal timeout on reasonably > fast matchines and after fix can take a few seconds, great Currently with the fix it takes <1s while gcc12 takes ~80s. > if after fix > would take longer but still not horribly long, one way to do it is > guard the test with run_expensive_tests effective target. Or another way > is have the test smaller in complexity normally and > // { dg-additional-options "-DEXPENSIVE" { target run_expensive_tests } } > and #ifdef EXPENSIVE make it more complex. Curiously, making the recursion much deeper doesn't work for that; I guess at some point the -Wsequence-point code decides the expression is too complex and gives up? But repeating the assignment brings it up over the timeout. How about this? [-- Attachment #2: 0001-c-family-Wsequence-point-and-COMPONENT_REF-PR107163.patch --] [-- Type: text/x-patch, Size: 2454 bytes --] From bb302f97929df9b854f7f929093441da60305254 Mon Sep 17 00:00:00 2001 From: Jason Merrill <jason@redhat.com> Date: Thu, 23 Mar 2023 15:57:39 -0400 Subject: [PATCH] c-family: -Wsequence-point and COMPONENT_REF [PR107163] To: gcc-patches@gcc.gnu.org The patch for PR91415 fixed -Wsequence-point to treat shifts and ARRAY_REF as sequenced in C++17, and COMPONENT_REF as well. But this is unnecessary for COMPONENT_REF, since the RHS is just a FIELD_DECL with no actual evaluation, and in this testcase handling COMPONENT_REF as sequenced blows up fast in a deep inheritance tree. Instead, look through it. PR c++/107163 gcc/c-family/ChangeLog: * c-common.cc (verify_tree): Don't use sequenced handling for COMPONENT_REF. gcc/testsuite/ChangeLog: * g++.dg/template/recurse5.C: New test. --- gcc/c-family/c-common.cc | 5 +++- gcc/testsuite/g++.dg/template/recurse5.C | 37 ++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/template/recurse5.C diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc index bfb950e56db..07f7beac8fd 100644 --- a/gcc/c-family/c-common.cc +++ b/gcc/c-family/c-common.cc @@ -2154,12 +2154,15 @@ verify_tree (tree x, struct tlist **pbefore_sp, struct tlist **pno_sp, case LSHIFT_EXPR: case RSHIFT_EXPR: - case COMPONENT_REF: case ARRAY_REF: if (cxx_dialect >= cxx17) goto sequenced_binary; goto do_default; + case COMPONENT_REF: + x = TREE_OPERAND (x, 0); + goto restart; + default: do_default: /* For other expressions, simply recurse on their operands. diff --git a/gcc/testsuite/g++.dg/template/recurse5.C b/gcc/testsuite/g++.dg/template/recurse5.C new file mode 100644 index 00000000000..0354ab09f53 --- /dev/null +++ b/gcc/testsuite/g++.dg/template/recurse5.C @@ -0,0 +1,37 @@ +// PR c++/107163 +// { dg-additional-options "-Wsequence-point" } + +struct BaseType { + int i; +}; + +template< int Seq > +class DerivedType : public DerivedType< Seq - 1 > { }; + +template<> +class DerivedType< -1 > : public BaseType { }; + +int main() { + DerivedType< 400 > d; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + d.i = 42; + return d.i; +} -- 2.31.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] 2023-03-24 22:11 ` Jason Merrill @ 2023-03-24 22:25 ` Jakub Jelinek 2023-03-28 15:31 ` Jason Merrill 0 siblings, 1 reply; 5+ messages in thread From: Jakub Jelinek @ 2023-03-24 22:25 UTC (permalink / raw) To: Jason Merrill; +Cc: gcc-patches On Fri, Mar 24, 2023 at 06:11:44PM -0400, Jason Merrill wrote: > > When we touch this for COMPONENT_REF, shouldn't we then handle it as > > unary given that the second operand is FIELD_DECL and third/fourth > > will likely be NULL and even if not, aren't user expressions that should be > > inspected? > > So, instead of doing this do: > > case COMPONENT_REF: > > x = TREE_OPERAND (x, 0); > > writer = 0; > > goto restart; > > ? > > Is clearing 'writer' what we want, since an access to COMPONENT_REF is an > access to (a subobject of) its op0? I've just mindlessly copied the unary op case. writer is set for pre/post increments and lhs of MODIFY_EXPR, and it is true that VIEW_CONVERT_EXPR doesn't clear it, but e.g. ARRAY_REF clears it for all operands. > Currently with the fix it takes <1s while gcc12 takes ~80s. Perfect. > PR c++/107163 > > gcc/c-family/ChangeLog: > > * c-common.cc (verify_tree): Don't use sequenced handling > for COMPONENT_REF. > > gcc/testsuite/ChangeLog: > > * g++.dg/template/recurse5.C: New test. LGTM, thanks. Maybe the testcase would be better as warn/Wsequence-point-5.C, dunno. Jakub ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] 2023-03-24 22:25 ` Jakub Jelinek @ 2023-03-28 15:31 ` Jason Merrill 0 siblings, 0 replies; 5+ messages in thread From: Jason Merrill @ 2023-03-28 15:31 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches On 3/24/23 18:25, Jakub Jelinek wrote: > On Fri, Mar 24, 2023 at 06:11:44PM -0400, Jason Merrill wrote: >>> When we touch this for COMPONENT_REF, shouldn't we then handle it as >>> unary given that the second operand is FIELD_DECL and third/fourth >>> will likely be NULL and even if not, aren't user expressions that should be >>> inspected? >>> So, instead of doing this do: >>> case COMPONENT_REF: >>> x = TREE_OPERAND (x, 0); >>> writer = 0; >>> goto restart; >>> ? >> >> Is clearing 'writer' what we want, since an access to COMPONENT_REF is an >> access to (a subobject of) its op0? > > I've just mindlessly copied the unary op case. > writer is set for pre/post increments and lhs of MODIFY_EXPR, and it is > true that VIEW_CONVERT_EXPR doesn't clear it, but e.g. ARRAY_REF clears it > for all operands. For whatever reason leaving writer set led to lots of false positives, so I've gone with your suggestion. >> Currently with the fix it takes <1s while gcc12 takes ~80s. > > Perfect. > >> PR c++/107163 >> >> gcc/c-family/ChangeLog: >> >> * c-common.cc (verify_tree): Don't use sequenced handling >> for COMPONENT_REF. >> >> gcc/testsuite/ChangeLog: >> >> * g++.dg/template/recurse5.C: New test. > > LGTM, thanks. Maybe the testcase would be better as > warn/Wsequence-point-5.C, dunno. Done. Jason ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-28 15:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-23 20:35 [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] Jason Merrill 2023-03-23 21:03 ` Jakub Jelinek 2023-03-24 22:11 ` Jason Merrill 2023-03-24 22:25 ` Jakub Jelinek 2023-03-28 15:31 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).