v2: Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)

Message ID 1550179249-4418-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • v2: Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)
Related show

Commit Message

David Malcolm Feb. 14, 2019, 9:20 p.m.
On Thu, 2019-02-14 at 17:32 +0100, Jakub Jelinek wrote:
> On Thu, Feb 14, 2019 at 11:26:15AM -0500, David Malcolm wrote:

> > There's an asymmetry in the warning; it's looking for a comparison

> > of a

> > LHS expression against an RHS constant 0, spelled as "0".

> > 

> > If we fold_for_warn on the RHS, then that folding introduces a

> > warning

> > for expressions that aren't spelled as "0" but can be folded to 0,

> > e.g., with:

> > 

> > enum { FOO, BAR };

> 

> So, shouldn't it be made symmetric?  Check if one argument is literal

> 0

> before folding, and only if it is, fold_for_warn the other argument?

> 

> 	Jakub


The reference to symmetry in my earlier email was somewhat
misleading, sorry.

The test happens after a canonicalization of the ordering happens
here, near the top of shorten_compare:

  /* If first arg is constant, swap the args (changing operation
     so value is preserved), for canonicalization.  Don't do this if
     the second arg is 0.  */

so this already gives us symmetry.

Here's an updated version of the patch which add the fold_for_warn in
a slightly later place, and adds a comment, and some more test cases.

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

OK for trunk?


Blurb from v1:

PR c++/88680 reports excess warnings from -Wtype-limits after the C++
FE's use of location wrappers was extended in r267272 for cases such as:

  const unsigned n = 8;
  static_assert (n >= 0 && n % 2 == 0, "");

t.C:3:18: warning: comparison of unsigned expression >= 0 is always true
  [-Wtype-limits]
    3 | static_assert (n >= 0 && n % 2 == 0, "");
      |                ~~^~~~

The root cause is that the location wrapper around "n" breaks the
suppression of the warning for the "if OP0 is a constant that is >= 0"
case.

This patch fixes it by calling fold_for_warn on OP0, extracting the
constant.

gcc/c-family/ChangeLog:
	PR c++/88680
	* c-common.c (shorten_compare): Call fold_for_warn on op0 when
	implementing -Wtype-limits.

gcc/testsuite/ChangeLog:
	PR c++/88680
	* g++.dg/wrappers/pr88680.C: New test.
---
 gcc/c-family/c-common.c                 | 13 ++++++--
 gcc/testsuite/g++.dg/wrappers/pr88680.C | 56 +++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/wrappers/pr88680.C

-- 
1.8.5.3

Comments

Jason Merrill Feb. 15, 2019, 4:29 p.m. | #1
On 2/14/19 4:20 PM, David Malcolm wrote:
> On Thu, 2019-02-14 at 17:32 +0100, Jakub Jelinek wrote:

>> On Thu, Feb 14, 2019 at 11:26:15AM -0500, David Malcolm wrote:

>>> There's an asymmetry in the warning; it's looking for a comparison

>>> of a

>>> LHS expression against an RHS constant 0, spelled as "0".

>>>

>>> If we fold_for_warn on the RHS, then that folding introduces a

>>> warning

>>> for expressions that aren't spelled as "0" but can be folded to 0,

>>> e.g., with:

>>>

>>> enum { FOO, BAR };

>>

>> So, shouldn't it be made symmetric?  Check if one argument is literal

>> 0

>> before folding, and only if it is, fold_for_warn the other argument?

>>

>> 	Jakub

> 

> The reference to symmetry in my earlier email was somewhat

> misleading, sorry.

> 

> The test happens after a canonicalization of the ordering happens

> here, near the top of shorten_compare:

> 

>    /* If first arg is constant, swap the args (changing operation

>       so value is preserved), for canonicalization.  Don't do this if

>       the second arg is 0.  */

> 

> so this already gives us symmetry.

> 

> Here's an updated version of the patch which add the fold_for_warn in

> a slightly later place, and adds a comment, and some more test cases.

> 

> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

> 

> OK for trunk?


OK.

Jason

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index ae23e59..c6856c9 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3117,6 +3117,12 @@  shorten_compare (location_t loc, tree *op0_ptr, tree *op1_ptr,
       primop0 = op0;
       primop1 = op1;
 
+      /* We want to fold unsigned comparisons of >= and < against zero.
+	 For these, we may also issue a warning if we have a non-constant
+	 compared against zero, where the zero was spelled as "0" (rather
+	 than merely folding to it).
+	 If we have at least one constant, then op1 is constant
+	 and we may have a non-constant expression as op0.  */
       if (!real1 && !real2 && integer_zerop (primop1)
 	  && TYPE_UNSIGNED (*restype_ptr))
 	{
@@ -3125,13 +3131,14 @@  shorten_compare (location_t loc, tree *op0_ptr, tree *op1_ptr,
 	     if OP0 is a constant that is >= 0, the signedness of
 	     the comparison isn't an issue, so suppress the
 	     warning.  */
+	  tree folded_op0 = fold_for_warn (op0);
 	  bool warn = 
 	    warn_type_limits && !in_system_header_at (loc)
-	    && !(TREE_CODE (primop0) == INTEGER_CST
+	    && !(TREE_CODE (folded_op0) == INTEGER_CST
 		 && !TREE_OVERFLOW (convert (c_common_signed_type (type),
-					     primop0)))
+					     folded_op0)))
 	    /* Do not warn for enumeration types.  */
-	    && (TREE_CODE (expr_original_type (primop0)) != ENUMERAL_TYPE);
+	    && (TREE_CODE (expr_original_type (folded_op0)) != ENUMERAL_TYPE);
 	  
 	  switch (code)
 	    {
diff --git a/gcc/testsuite/g++.dg/wrappers/pr88680.C b/gcc/testsuite/g++.dg/wrappers/pr88680.C
new file mode 100644
index 0000000..5497cda
--- /dev/null
+++ b/gcc/testsuite/g++.dg/wrappers/pr88680.C
@@ -0,0 +1,56 @@ 
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wtype-limits" }
+
+const unsigned N = 8;
+const unsigned P = 0;
+
+enum { FOO, BAR };
+
+static_assert (N >= 0 && N % 2 == 0, "");
+static_assert (FOO >= 0, "");
+static_assert (FOO >= FOO, "");
+static_assert (FOO >= P, "");
+static_assert (BAR >= P, "");
+static_assert (N >= FOO, "");
+
+void test(unsigned n)
+{
+  if (N >= 0 && N % 2 == 0)
+    return;
+  if (FOO >= 0)
+    return;
+  if (FOO >= FOO)
+    return;
+  if (FOO >= P)
+    return;
+  if (BAR >= P)
+    return;
+  if (N >= FOO)
+    return;
+  if (n >= 0) // { dg-warning ">= 0 is always true" }
+    return;
+  if (n < 0) // { dg-warning "< 0 is always false" }
+    return;
+  if (n >= FOO)
+    return;
+  if (n < FOO)
+    return;
+  if (N >= 0)
+    return;
+  if (N < 0)
+    return;
+  if (N >= FOO)
+    return;
+  if (N < FOO)
+    return;
+  if (0 <= FOO)
+    return;
+  if (0 <= n) // { dg-warning ">= 0 is always true" }
+    return;
+  if (0 > n) // { dg-warning "< 0 is always false" }
+    return;
+  if (N <= FOO)
+    return;
+  if (N <= n)
+    return;
+}