[PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163]

[PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163]

[Jason Merrill]

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

Re: [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163]

[Jakub Jelinek]

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

Re: [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163]

[Jason Merrill]

[-- 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

Re: [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163]

[Jakub Jelinek]

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

Re: [PATCH RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163]

[Jason Merrill]

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