diff mbox series

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

Message ID 20230323203507.2960052-1-jason@redhat.com
State Accepted
Headers
Series [RFC] c-family: -Wsequence-point and COMPONENT_REF [PR107163] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Jason Merrill March 23, 2023, 8:35 p.m. UTC 
  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(-)


base-commit: 4872e46e080c6695dfe1f9dc9db26b4703bc348c

Comments

Jakub Jelinek March 23, 2023, 9:03 p.m. UTC | #1 
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
Jason Merrill March 24, 2023, 10:11 p.m. UTC | #2 
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?
Jakub Jelinek March 24, 2023, 10:25 p.m. UTC | #3 
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
Jason Merrill March 28, 2023, 3:31 p.m. UTC | #4 
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
diff mbox series

Patch

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;