[PATCH] Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)

[PATCH] Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)

[David Malcolm]

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.

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

OK for trunk?

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                 |  2 +-
 gcc/testsuite/g++.dg/wrappers/pr88680.C | 47 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/wrappers/pr88680.C

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index d3b5879..5813e0a 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3114,7 +3114,7 @@ shorten_compare (location_t loc, tree *op0_ptr, tree *op1_ptr,
       /* Here we must do the comparison on the nominal type
 	 using the args exactly as we received them.  */
       type = *restype_ptr;
-      primop0 = op0;
+      primop0 = fold_for_warn (op0);
       primop1 = op1;
 
       if (!real1 && !real2 && integer_zerop (primop1)
diff --git a/gcc/testsuite/g++.dg/wrappers/pr88680.C b/gcc/testsuite/g++.dg/wrappers/pr88680.C
new file mode 100644
index 0000000..86945db
--- /dev/null
+++ b/gcc/testsuite/g++.dg/wrappers/pr88680.C
@@ -0,0 +1,47 @@
+// { 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;
+
+}
-- 
1.8.5.3

PING Re: [PATCH] Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)

[David Malcolm]

Ping:
  https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00363.html

On Wed, 2019-02-06 at 21:23 -0500, David Malcolm wrote:
> 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.
> 
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> 
> OK for trunk?
> 
> 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                 |  2 +-
>  gcc/testsuite/g++.dg/wrappers/pr88680.C | 47
> +++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/wrappers/pr88680.C
> 
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index d3b5879..5813e0a 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -3114,7 +3114,7 @@ shorten_compare (location_t loc, tree *op0_ptr,
> tree *op1_ptr,
>        /* Here we must do the comparison on the nominal type
>  	 using the args exactly as we received them.  */
>        type = *restype_ptr;
> -      primop0 = op0;
> +      primop0 = fold_for_warn (op0);
>        primop1 = op1;
>  
>        if (!real1 && !real2 && integer_zerop (primop1)
> diff --git a/gcc/testsuite/g++.dg/wrappers/pr88680.C
> b/gcc/testsuite/g++.dg/wrappers/pr88680.C
> new file mode 100644
> index 0000000..86945db
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/wrappers/pr88680.C
> @@ -0,0 +1,47 @@
> +// { 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;
> +
> +}

Re: [PATCH] Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)

[Jason Merrill]

On 2/6/19 9:23 PM, David Malcolm wrote:
> 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.

Is there a reason not to do this for OP1 as well?

Jason

Re: [PATCH] Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)

[David Malcolm]

On Thu, 2019-02-14 at 10:38 -0500, Jason Merrill wrote:
> On 2/6/19 9:23 PM, David Malcolm wrote:
> > 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.
> 
> Is there a reason not to do this for OP1 as well?

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 };

pr88680.C:34:9: warning: comparison of unsigned expression >= 0 is
always true [-Wtype-limits]
   34 |   if (n >= FOO)
      |       ~~^~~~~~
pr88680.C:36:9: warning: comparison of unsigned expression < 0 is
always false [-Wtype-limits]
   36 |   if (n < FOO)
      |       ~~^~~~~

which we didn't warn for before.

We need to fold the LHS so that we can look through wrapper nodes, and
around wrappers around enum values, to suppress the warning if the
folded version of the LHS is a constant that fits in the data type (for
the example given in the original message above).

Dave

Re: [PATCH] Fix excess warnings from -Wtype-limits with location wrappers (PR c++/88680)

[Jakub Jelinek]

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

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

[David Malcolm]

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

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;
+}
-- 
1.8.5.3

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

[Jason Merrill]

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