diff mbox series

[v3] c++: ICE with -Wtautological-compare in template [PR116534]

Message ID ZunndSEv6JEoFgEF@redhat.com
State New
Headers show
Series [v3] c++: ICE with -Wtautological-compare in template [PR116534] | expand

Commit Message

Marek Polacek Sept. 17, 2024, 8:32 p.m. UTC 
On Tue, Sep 17, 2024 at 12:53:25PM -0400, Jason Merrill wrote:
> On 9/16/24 7:08 PM, Marek Polacek wrote:
> > On Tue, Sep 10, 2024 at 10:37:31AM -0400, Jason Merrill wrote:
> > > On 8/29/24 12:23 PM, Marek Polacek wrote:
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/14?
> > > > 
> > > > -- >8 --
> > > > Pre r14-4793, we'd call warn_tautological_cmp -> operand_equal_p
> > > > with operands wrapped in NON_DEPENDENT_EXPR, which works, since
> > > > o_e_p bails for codes it doesn't know.  But now we pass operands
> > > > not encapsulated in NON_DEPENDENT_EXPR, and crash, because the
> > > > template tree for &a[x] has null DECL_FIELD_OFFSET.
> > > 
> > > Why are we trying to compare DECL_FIELD_OFFSET in C++, anyway?  I'd think we
> > > should limit that to C and in C++ rely on FIELD_DECL identity.
> > 
> > This goes back to r12-7797.  I suppose it's trying to handle C++ because we
> > can't use c_dialect_cxx () in fold-const.  I know there's lang_GNU_CXX but
> > I suggest we do the following.
> > > > I don't think I meant this warning to be called in a template,
> > > > so let's avoid the problem this easy way.
> > > 
> > > Hmm, it seems potentially useful in a template, especially since we suppress
> > > it in instantiations.
> > 
> > Ack.
> > 
> > How about this?
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > -- >8 --
> > Pre r14-4793, we'd call warn_tautological_cmp -> operand_equal_p
> > with operands wrapped in NON_DEPENDENT_EXPR, which works, since
> > o_e_p bails for codes it doesn't know.  But now we pass operands
> > not encapsulated in NON_DEPENDENT_EXPR, and crash, because the
> > template tree for &a[x] has null DECL_FIELD_OFFSET.
> > 
> > This patch extends r12-7797 to cover the case when DECL_FIELD_OFFSET
> > is null.
> > 
> > 	PR c++/116534
> > 
> > gcc/ChangeLog:
> > 
> > 	* fold-const.cc (operand_compare::operand_equal_p): Check
> > 	DECL_FIELD_OFFSET.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/warn/Wtautological-compare4.C: New test.
> > ---
> >   gcc/fold-const.cc                             | 16 ++++++++------
> >   .../g++.dg/warn/Wtautological-compare4.C      | 21 +++++++++++++++++++
> >   2 files changed, 31 insertions(+), 6 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare4.C
> > 
> > diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> > index 70db16759d0..3da09621584 100644
> > --- a/gcc/fold-const.cc
> > +++ b/gcc/fold-const.cc
> > @@ -3601,12 +3601,16 @@ operand_compare::operand_equal_p (const_tree arg0, const_tree arg1,
> >   		    /* Non-FIELD_DECL operands can appear in C++ templates.  */
> >   		    if (TREE_CODE (field0) != FIELD_DECL
> > -			|| TREE_CODE (field1) != FIELD_DECL
> > -			|| !operand_equal_p (DECL_FIELD_OFFSET (field0),
> > -					     DECL_FIELD_OFFSET (field1), flags)
> > -			|| !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
> > -					     DECL_FIELD_BIT_OFFSET (field1),
> > -					     flags))
> > +			|| TREE_CODE (field1) != FIELD_DECL)
> > +		      return false;
> > +
> > +		    if (DECL_FIELD_OFFSET (field0)
> > +			&& DECL_FIELD_OFFSET (field1)
> > +			&& (!operand_equal_p (DECL_FIELD_OFFSET (field0),
> > +					      DECL_FIELD_OFFSET (field1), flags)
> > +			    || !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
> > +						 DECL_FIELD_BIT_OFFSET (field1),
> > +						 flags)))
> 
> If at least one has null FIELD_OFFSET, I'd think we still want to compare
> them for being ==, this seems to assume that a field with null offset is
> equal to any other field.

Oop, you're right, that's no good.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/14?

-- >8 --
Pre r14-4793, we'd call warn_tautological_cmp -> operand_equal_p
with operands wrapped in NON_DEPENDENT_EXPR, which works, since
o_e_p bails for codes it doesn't know.  But now we pass operands
not encapsulated in NON_DEPENDENT_EXPR, and crash, because the
template tree for &a[x] has null DECL_FIELD_OFFSET.

This patch extends r12-7797 to cover the case when DECL_FIELD_OFFSET
is null.

	PR c++/116534

gcc/ChangeLog:

	* fold-const.cc (operand_compare::operand_equal_p): If either
	field's DECL_FIELD_OFFSET is null, compare the fields with ==.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wtautological-compare4.C: New test.
---
 gcc/fold-const.cc                             | 12 ++++++++---
 .../g++.dg/warn/Wtautological-compare4.C      | 21 +++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare4.C


base-commit: dfe0d4389a3ce43179563a63046ad3e74d615a08

Comments

Jason Merrill Sept. 17, 2024, 8:56 p.m. UTC | #1 
On 9/17/24 10:32 PM, Marek Polacek wrote:
> On Tue, Sep 17, 2024 at 12:53:25PM -0400, Jason Merrill wrote:
>> On 9/16/24 7:08 PM, Marek Polacek wrote:
>>> On Tue, Sep 10, 2024 at 10:37:31AM -0400, Jason Merrill wrote:
>>>> On 8/29/24 12:23 PM, Marek Polacek wrote:
>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/14?
>>>>>
>>>>> -- >8 --
>>>>> Pre r14-4793, we'd call warn_tautological_cmp -> operand_equal_p
>>>>> with operands wrapped in NON_DEPENDENT_EXPR, which works, since
>>>>> o_e_p bails for codes it doesn't know.  But now we pass operands
>>>>> not encapsulated in NON_DEPENDENT_EXPR, and crash, because the
>>>>> template tree for &a[x] has null DECL_FIELD_OFFSET.
>>>>
>>>> Why are we trying to compare DECL_FIELD_OFFSET in C++, anyway?  I'd think we
>>>> should limit that to C and in C++ rely on FIELD_DECL identity.
>>>
>>> This goes back to r12-7797.  I suppose it's trying to handle C++ because we
>>> can't use c_dialect_cxx () in fold-const.  I know there's lang_GNU_CXX but
>>> I suggest we do the following.
>>>>> I don't think I meant this warning to be called in a template,
>>>>> so let's avoid the problem this easy way.
>>>>
>>>> Hmm, it seems potentially useful in a template, especially since we suppress
>>>> it in instantiations.
>>>
>>> Ack.
>>>
>>> How about this?
>>>
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>
>>> -- >8 --
>>> Pre r14-4793, we'd call warn_tautological_cmp -> operand_equal_p
>>> with operands wrapped in NON_DEPENDENT_EXPR, which works, since
>>> o_e_p bails for codes it doesn't know.  But now we pass operands
>>> not encapsulated in NON_DEPENDENT_EXPR, and crash, because the
>>> template tree for &a[x] has null DECL_FIELD_OFFSET.
>>>
>>> This patch extends r12-7797 to cover the case when DECL_FIELD_OFFSET
>>> is null.
>>>
>>> 	PR c++/116534
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* fold-const.cc (operand_compare::operand_equal_p): Check
>>> 	DECL_FIELD_OFFSET.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/warn/Wtautological-compare4.C: New test.
>>> ---
>>>    gcc/fold-const.cc                             | 16 ++++++++------
>>>    .../g++.dg/warn/Wtautological-compare4.C      | 21 +++++++++++++++++++
>>>    2 files changed, 31 insertions(+), 6 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare4.C
>>>
>>> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
>>> index 70db16759d0..3da09621584 100644
>>> --- a/gcc/fold-const.cc
>>> +++ b/gcc/fold-const.cc
>>> @@ -3601,12 +3601,16 @@ operand_compare::operand_equal_p (const_tree arg0, const_tree arg1,
>>>    		    /* Non-FIELD_DECL operands can appear in C++ templates.  */
>>>    		    if (TREE_CODE (field0) != FIELD_DECL
>>> -			|| TREE_CODE (field1) != FIELD_DECL
>>> -			|| !operand_equal_p (DECL_FIELD_OFFSET (field0),
>>> -					     DECL_FIELD_OFFSET (field1), flags)
>>> -			|| !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
>>> -					     DECL_FIELD_BIT_OFFSET (field1),
>>> -					     flags))
>>> +			|| TREE_CODE (field1) != FIELD_DECL)
>>> +		      return false;
>>> +
>>> +		    if (DECL_FIELD_OFFSET (field0)
>>> +			&& DECL_FIELD_OFFSET (field1)
>>> +			&& (!operand_equal_p (DECL_FIELD_OFFSET (field0),
>>> +					      DECL_FIELD_OFFSET (field1), flags)
>>> +			    || !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
>>> +						 DECL_FIELD_BIT_OFFSET (field1),
>>> +						 flags)))
>>
>> If at least one has null FIELD_OFFSET, I'd think we still want to compare
>> them for being ==, this seems to assume that a field with null offset is
>> equal to any other field.
> 
> Oop, you're right, that's no good.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/14?

OK.

> -- >8 --
> Pre r14-4793, we'd call warn_tautological_cmp -> operand_equal_p
> with operands wrapped in NON_DEPENDENT_EXPR, which works, since
> o_e_p bails for codes it doesn't know.  But now we pass operands
> not encapsulated in NON_DEPENDENT_EXPR, and crash, because the
> template tree for &a[x] has null DECL_FIELD_OFFSET.
> 
> This patch extends r12-7797 to cover the case when DECL_FIELD_OFFSET
> is null.
> 
> 	PR c++/116534
> 
> gcc/ChangeLog:
> 
> 	* fold-const.cc (operand_compare::operand_equal_p): If either
> 	field's DECL_FIELD_OFFSET is null, compare the fields with ==.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Wtautological-compare4.C: New test.
> ---
>   gcc/fold-const.cc                             | 12 ++++++++---
>   .../g++.dg/warn/Wtautological-compare4.C      | 21 +++++++++++++++++++
>   2 files changed, 30 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wtautological-compare4.C
> 
> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> index 70db16759d0..0578f42ac0c 100644
> --- a/gcc/fold-const.cc
> +++ b/gcc/fold-const.cc
> @@ -3601,9 +3601,15 @@ operand_compare::operand_equal_p (const_tree arg0, const_tree arg1,
>   
>   		    /* Non-FIELD_DECL operands can appear in C++ templates.  */
>   		    if (TREE_CODE (field0) != FIELD_DECL
> -			|| TREE_CODE (field1) != FIELD_DECL
> -			|| !operand_equal_p (DECL_FIELD_OFFSET (field0),
> -					     DECL_FIELD_OFFSET (field1), flags)
> +			|| TREE_CODE (field1) != FIELD_DECL)
> +		      return false;
> +
> +		    if (!DECL_FIELD_OFFSET (field0)
> +			|| !DECL_FIELD_OFFSET (field1))
> +		      return field0 == field1;
> +
> +		    if (!operand_equal_p (DECL_FIELD_OFFSET (field0),
> +					  DECL_FIELD_OFFSET (field1), flags)
>   			|| !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
>   					     DECL_FIELD_BIT_OFFSET (field1),
>   					     flags))
> diff --git a/gcc/testsuite/g++.dg/warn/Wtautological-compare4.C b/gcc/testsuite/g++.dg/warn/Wtautological-compare4.C
> new file mode 100644
> index 00000000000..96308f49a42
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wtautological-compare4.C
> @@ -0,0 +1,21 @@
> +// PR c++/116534
> +// { dg-do compile }
> +// { dg-options "-Wall" }
> +
> +template <class A>
> +struct Test {
> +    bool foo(unsigned x, unsigned y) {
> +        bool test = &a[x] == &b[y];
> +	return test;
> +    }
> +    unsigned *a;
> +    unsigned *b;
> +};
> +
> +void
> +g ()
> +{
> +  Test<int> t;
> +  t.foo (0u, 1u);
> +  t.foo (0u, 0u);
> +}
> 
> base-commit: dfe0d4389a3ce43179563a63046ad3e74d615a08
diff mbox series

Patch

diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
index 70db16759d0..0578f42ac0c 100644
--- a/gcc/fold-const.cc
+++ b/gcc/fold-const.cc
@@ -3601,9 +3601,15 @@  operand_compare::operand_equal_p (const_tree arg0, const_tree arg1,
 
 		    /* Non-FIELD_DECL operands can appear in C++ templates.  */
 		    if (TREE_CODE (field0) != FIELD_DECL
-			|| TREE_CODE (field1) != FIELD_DECL
-			|| !operand_equal_p (DECL_FIELD_OFFSET (field0),
-					     DECL_FIELD_OFFSET (field1), flags)
+			|| TREE_CODE (field1) != FIELD_DECL)
+		      return false;
+
+		    if (!DECL_FIELD_OFFSET (field0)
+			|| !DECL_FIELD_OFFSET (field1))
+		      return field0 == field1;
+
+		    if (!operand_equal_p (DECL_FIELD_OFFSET (field0),
+					  DECL_FIELD_OFFSET (field1), flags)
 			|| !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
 					     DECL_FIELD_BIT_OFFSET (field1),
 					     flags))
diff --git a/gcc/testsuite/g++.dg/warn/Wtautological-compare4.C b/gcc/testsuite/g++.dg/warn/Wtautological-compare4.C
new file mode 100644
index 00000000000..96308f49a42
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wtautological-compare4.C
@@ -0,0 +1,21 @@ 
+// PR c++/116534
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+template <class A>
+struct Test {
+    bool foo(unsigned x, unsigned y) {
+        bool test = &a[x] == &b[y];
+	return test;
+    }
+    unsigned *a;
+    unsigned *b;
+};
+
+void
+g ()
+{
+  Test<int> t;
+  t.foo (0u, 1u);
+  t.foo (0u, 0u);
+}