PATCH to shorten_compare -Wtype-limits handling

PATCH to shorten_compare -Wtype-limits handling

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

The rs6000 target was hitting a bootstrap failure due to 
-Werror=type-limits.  Since warn_tautological_cmp and other warnings 
avoid warning if one of the operands comes from a macro, I thought it 
would make sense to do that here as well.

Tested that this allows rs6000 bootstrap to proceed, regression tested 
on x86_64-pc-linux-gnu, applying to trunk.  David, do you want to revert 
the #pragma GCC diagnostic change?

[-- Attachment #2: type-lim.patch --]
[-- Type: text/x-patch, Size: 1206 bytes --]

commit 3d382500ccb766eb1c6dea69a32348d62c86b950
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Nov 18 16:30:06 2015 -0500

    	* c-common.c (shorten_compare): Don't -Wtype-limits if the
    	non-constant operand comes from a macro.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index f50ca48..068a0bc 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -4650,7 +4650,9 @@ shorten_compare (location_t loc, tree *op0_ptr, tree *op1_ptr,
 	  type = c_common_unsigned_type (type);
 	}
 
-      if (TREE_CODE (primop0) != INTEGER_CST)
+      if (TREE_CODE (primop0) != INTEGER_CST
+	  /* Don't warn if it's from a macro.  */
+	  && !from_macro_expansion_at (EXPR_LOCATION (primop0)))
 	{
 	  if (val == truthvalue_false_node)
 	    warning_at (loc, OPT_Wtype_limits,
diff --git a/gcc/testsuite/g++.dg/warn/Wtype-limits2.C b/gcc/testsuite/g++.dg/warn/Wtype-limits2.C
new file mode 100644
index 0000000..a46baad
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wtype-limits2.C
@@ -0,0 +1,11 @@
+// { dg-options -Wtype-limits }
+
+unsigned char array[4];
+bool b;
+#define VAL (b ? array[0] : (unsigned char)0)
+
+int main()
+{
+  if (VAL > 1000)
+    __builtin_abort();
+}

Re: PATCH to shorten_compare -Wtype-limits handling

[David Edelsohn]

On Wed, Nov 18, 2015 at 11:26 PM, Jason Merrill <jason@redhat.com> wrote:
> The rs6000 target was hitting a bootstrap failure due to
> -Werror=type-limits.  Since warn_tautological_cmp and other warnings avoid
> warning if one of the operands comes from a macro, I thought it would make
> sense to do that here as well.
>
> Tested that this allows rs6000 bootstrap to proceed, regression tested on
> x86_64-pc-linux-gnu, applying to trunk.  David, do you want to revert the
> #pragma GCC diagnostic change?

Thanks for the patch.

Yes, I will revert the #pragma GCC diagnostic patch.  It was meant as
a temporary hack to fix bootstrap while a long-term solution was
developed.

Thanks, David

Re: PATCH to shorten_compare -Wtype-limits handling

[Jeff Law]

On 11/18/2015 09:33 PM, David Edelsohn wrote:
> On Wed, Nov 18, 2015 at 11:26 PM, Jason Merrill <jason@redhat.com> wrote:
>> The rs6000 target was hitting a bootstrap failure due to
>> -Werror=type-limits.  Since warn_tautological_cmp and other warnings avoid
>> warning if one of the operands comes from a macro, I thought it would make
>> sense to do that here as well.
>>
>> Tested that this allows rs6000 bootstrap to proceed, regression tested on
>> x86_64-pc-linux-gnu, applying to trunk.  David, do you want to revert the
>> #pragma GCC diagnostic change?
>
> Thanks for the patch.
>
> Yes, I will revert the #pragma GCC diagnostic patch.  It was meant as
> a temporary hack to fix bootstrap while a long-term solution was
> developed.
The even longer term direction for this code is to separate out the 
type-limits warning from the canonicalization and shortening.  I've got 
a blob of code form Kai that goes in that direction, but it needs more 
engineering around it.

Ideally the canonicalization/shortening moves into match.pd.  The 
warning, in theory, moves out of the front-ends as well.



jeff

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 19 November 2015 at 17:09, Jeff Law <law@redhat.com> wrote:
> The even longer term direction for this code is to separate out the
> type-limits warning from the canonicalization and shortening.  I've got a
> blob of code form Kai that goes in that direction, but it needs more
> engineering around it.
>
> Ideally the canonicalization/shortening moves into match.pd.  The warning,
> in theory, moves out of the front-ends as well.

The last attempt by Kai was here:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01265.html

But there were a couple of patches from you some time ago, for
example: http://permalink.gmane.org/gmane.comp.gcc.patches/343476

What happened with those?

There is also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51712 It
would be great if that was eventually fixed in the new delay-folding
world.

Cheers,

Manuel.

Re: PATCH to shorten_compare -Wtype-limits handling

[Jeff Law]

On 11/19/2015 10:48 AM, Manuel López-Ibáñez wrote:
> On 19 November 2015 at 17:09, Jeff Law <law@redhat.com> wrote:
>> The even longer term direction for this code is to separate out the
>> type-limits warning from the canonicalization and shortening.  I've got a
>> blob of code form Kai that goes in that direction, but it needs more
>> engineering around it.
>>
>> Ideally the canonicalization/shortening moves into match.pd.  The warning,
>> in theory, moves out of the front-ends as well.
>
> The last attempt by Kai was here:
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01265.html
Right.  THe problem is there wasn't any real information on how that 
affected code generation.


>
> But there were a couple of patches from you some time ago, for
> example: http://permalink.gmane.org/gmane.comp.gcc.patches/343476
>
> What happened with those?
On hold pending fixing the type-limits warning placement.  Essentially 
that has to be untangled first.

>
> There is also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51712 It
> would be great if that was eventually fixed in the new delay-folding
> world.
Noted.

jeff

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 19 November 2015 at 17:54, Jeff Law <law@redhat.com> wrote:
>> But there were a couple of patches from you some time ago, for
>> example: http://permalink.gmane.org/gmane.comp.gcc.patches/343476
>>
>> What happened with those?
>
> On hold pending fixing the type-limits warning placement.  Essentially that
> has to be untangled first.

Could you elaborate on this? Or point me to some previous email
thread? (I don't have enough free time to follow the mailing list
anymore, sorry).

Cheers,

Manuel.

Re: PATCH to shorten_compare -Wtype-limits handling

[Jeff Law]

On 11/19/2015 12:02 PM, Manuel López-Ibáñez wrote:
> On 19 November 2015 at 17:54, Jeff Law <law@redhat.com> wrote:
>>> But there were a couple of patches from you some time ago, for
>>> example: http://permalink.gmane.org/gmane.comp.gcc.patches/343476
>>>
>>> What happened with those?
>>
>> On hold pending fixing the type-limits warning placement.  Essentially that
>> has to be untangled first.
>
> Could you elaborate on this? Or point me to some previous email
> thread? (I don't have enough free time to follow the mailing list
> anymore, sorry).
I don't have the thread handy, but essentially the operand shortening 
code has warning bits inside it.  As a result pulling out functionality 
(such as operand canonicalization) and putting into match.pd results in 
missing a warning -- ie a regression.

So we have to detangle the operand shortening from warning detection. 
Kai's idea was to first make the shortening code "pure" in the sense 
that it would have no side effects other than to generate the warnings. 
  Canonicalization and other transformations would still occur 
internally, but not be reflected in the IL.

That was the overall plan and he posted a patch for that.  But that 
patch didn't do the due diligence to verify that once the shortening 
code was made "pure" that we didn't regress on the quality of the code 
we generated.

jeff

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 20 November 2015 at 17:42, Jeff Law <law@redhat.com> wrote:
> So we have to detangle the operand shortening from warning detection. Kai's
> idea was to first make the shortening code "pure" in the sense that it would
> have no side effects other than to generate the warnings.  Canonicalization
> and other transformations would still occur internally, but not be reflected
> in the IL.
>
> That was the overall plan and he posted a patch for that.  But that patch
> didn't do the due diligence to verify that once the shortening code was made
> "pure" that we didn't regress on the quality of the code we generated.

I thought that the original plan was to make the warning code also use
match.pd. That is, that all folding, including FE folding, will be
match.pd-based. Has this changed?

Cheers,

Manuel.

Re: PATCH to shorten_compare -Wtype-limits handling

[Jeff Law]

On 11/20/2015 11:04 AM, Manuel López-Ibáñez wrote:
> On 20 November 2015 at 17:42, Jeff Law <law@redhat.com> wrote:
>> So we have to detangle the operand shortening from warning detection. Kai's
>> idea was to first make the shortening code "pure" in the sense that it would
>> have no side effects other than to generate the warnings.  Canonicalization
>> and other transformations would still occur internally, but not be reflected
>> in the IL.
>>
>> That was the overall plan and he posted a patch for that.  But that patch
>> didn't do the due diligence to verify that once the shortening code was made
>> "pure" that we didn't regress on the quality of the code we generated.
>
> I thought that the original plan was to make the warning code also use
> match.pd. That is, that all folding, including FE folding, will be
> match.pd-based. Has this changed?
I don't think that's changed.

Detangling the two is the first step. Once detangled we can then look to 
move the warning to a more suitable location -- right now it's in the 
C/C++ front-ends, firing way too early.  Instead it ought to be checked 
in gimple form, after match.pd canonicalization and simplifications.

Jeff

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

On 20 November 2015 at 22:28, Jeff Law <law@redhat.com> wrote:
>>> That was the overall plan and he posted a patch for that.  But that patch
>>> didn't do the due diligence to verify that once the shortening code was
>>> made
>>> "pure" that we didn't regress on the quality of the code we generated.
>>
>>
>> I thought that the original plan was to make the warning code also use
>> match.pd. That is, that all folding, including FE folding, will be
>> match.pd-based. Has this changed?
>
> I don't think that's changed.
>
> Detangling the two is the first step. Once detangled we can then look to
> move the warning to a more suitable location -- right now it's in the C/C++
> front-ends, firing way too early.  Instead it ought to be checked in gimple
> form, after match.pd canonicalization and simplifications.

Perhaps worth noting is that the warnings in shorten_compare are
basically trying to figure out if X CMP_OP Y can be folded to
true/false. They do not care about shortening/optimizing the types and
conversions. Perhaps an intermediate step would be to duplicate the
function into a warning version and an optimization version, making
the warning version pure as you say, call the warning version first,
then the optimizing one. Note that this is not exactly what you want
to have at the end and it adds a bit of duplication, but it gives a
clear separation between what is needed for warning and what is needed
for optimizing.This is step one.

Step two makes the warning version fold using match.pd, fix any
warning regressions. Note that the warning version does not really
need to be pure. If the comparison can be folded to true/false, there
is no much more optimization to be done. But I'm not sure how the FEs
are handling results from folding for warnings. Otherwise, one is
probably duplicating a bit of analysis between the warning and
optimizing functions. Not sure if the overhead will be measurable at
all once the warning version uses match.pd.

Step three moves the optimizing version to match.pd. Identifying
regressions here may be harder, but no harder than any other move of
optimizations to match.pd.

The attached patch implements step 1. Feel free to do as you please with it.

Cheers,

Manuel.

[-- Attachment #2: shorten-compare.diff --]
[-- Type: text/plain, Size: 10648 bytes --]

Index: c-common.c
===================================================================
--- c-common.c	(revision 230753)
+++ c-common.c	(working copy)
@@ -4419,10 +4419,251 @@ expr_original_type (tree expr)
 {
   STRIP_SIGN_NOPS (expr);
   return TREE_TYPE (expr);
 }
 
+static void
+warn_shorten_compare (location_t loc, tree op0, tree op1,
+		      tree restype, enum tree_code rescode)
+{
+  if (!warn_type_limits)
+    return;
+
+  switch (rescode)
+    {
+    case NE_EXPR:
+    case EQ_EXPR:
+    case LT_EXPR:
+    case GT_EXPR:
+    case LE_EXPR:
+    case GE_EXPR:
+      break;
+
+    default:
+      return;
+    }
+
+  /* Throw away any conversions to wider types
+     already present in the operands.  */
+  int unsignedp0, unsignedp1;
+  tree primop0 = c_common_get_narrower (op0, &unsignedp0);
+  tree primop1 = c_common_get_narrower (op1, &unsignedp1);
+
+  /* If primopN is first sign-extended from primopN's precision to opN's
+     precision, then zero-extended from opN's precision to
+     restype precision, shortenings might be invalid.  */
+  if (TYPE_PRECISION (TREE_TYPE (primop0)) < TYPE_PRECISION (TREE_TYPE (op0))
+      && TYPE_PRECISION (TREE_TYPE (op0)) < TYPE_PRECISION (restype)
+      && !unsignedp0
+      && TYPE_UNSIGNED (TREE_TYPE (op0)))
+    primop0 = op0;
+  if (TYPE_PRECISION (TREE_TYPE (primop1)) < TYPE_PRECISION (TREE_TYPE (op1))
+      && TYPE_PRECISION (TREE_TYPE (op1)) < TYPE_PRECISION (restype)
+      && !unsignedp1
+      && TYPE_UNSIGNED (TREE_TYPE (op1)))
+    primop1 = op1;
+
+  /* Handle the case that OP0 does not *contain* a conversion
+     but it *requires* conversion to FINAL_TYPE.  */
+  if (op0 == primop0 && TREE_TYPE (op0) != restype)
+    unsignedp0 = TYPE_UNSIGNED (TREE_TYPE (op0));
+  if (op1 == primop1 && TREE_TYPE (op1) != restype)
+    unsignedp1 = TYPE_UNSIGNED (TREE_TYPE (op1));
+
+  /* If one of the operands must be float, we do not warn.  */
+  if (TREE_CODE (TREE_TYPE (primop0)) == REAL_TYPE
+      || TREE_CODE (TREE_TYPE (primop1)) == REAL_TYPE)
+    return;
+
+  /* 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.  */
+
+  if (TREE_CONSTANT (primop0)
+      && !integer_zerop (primop1) && !real_zerop (primop1)
+      && !fixed_zerop (primop1))
+    {
+      std::swap (primop0, primop1);
+      std::swap (op0, op1);
+      std::swap (unsignedp0, unsignedp1);
+
+      switch (rescode)
+	{
+	case LT_EXPR:
+	  rescode = GT_EXPR;
+	  break;
+	case GT_EXPR:
+	  rescode = LT_EXPR;
+	  break;
+	case LE_EXPR:
+	  rescode = GE_EXPR;
+	  break;
+	case GE_EXPR:
+	  rescode = LE_EXPR;
+	  break;
+	default:
+	  break;
+	}
+    }
+
+  /* If comparing an integer against a constant more bits wide, maybe we can
+     deduce a value of 1 or 0 independent of the data.  Or else truncate the
+     constant now rather than extend the variable at run time.
+
+     This is only interesting if the constant is the wider arg.
+     Also, it is not safe if the constant is unsigned and the
+     variable arg is signed, since in this case the variable
+     would be sign-extended and then regarded as unsigned.
+     Our technique fails in this case because the lowest/highest
+     possible unsigned results don't follow naturally from the
+     lowest/highest possible values of the variable operand.
+     For just EQ_EXPR and NE_EXPR there is another technique that
+     could be used: see if the constant can be faithfully represented
+     in the other operand's type, by truncating it and reextending it
+     and see if that preserves the constant's value.  */
+
+  if (TREE_CODE (primop0) != INTEGER_CST
+      /* Don't warn if it's from a (non-system) macro.  */
+      && TREE_CODE (TREE_TYPE (primop0)) != FIXED_POINT_TYPE
+      && TREE_CODE (primop1) == INTEGER_CST
+      && TYPE_PRECISION (TREE_TYPE (primop0)) < TYPE_PRECISION (restype)
+      && !(from_macro_expansion_at (expansion_point_location_if_in_system_header
+				    (EXPR_LOCATION (primop0)))))
+    {
+      /* true if comparison is nominally unsigned.  */
+      bool unsignedp = TYPE_UNSIGNED (restype);
+
+      /* If primop0 was sign-extended and unsigned comparison specd,
+	 we do a signed comparison using the signed type bounds.
+	 But the comparison we output must be unsigned.
+
+	 Also, for inequalities, VAL is no good; but if the signed
+	 comparison had *any* fixed result, it follows that the
+	 unsigned comparison just tests the sign in reverse
+	 (positive values are LE, negative ones GE).
+	 So we can generate an unsigned comparison
+	 against an extreme value of the signed type.  */
+
+      if (unsignedp && !unsignedp0)
+	switch (rescode)
+	  {
+	  case LT_EXPR:
+	  case GE_EXPR:
+	  case LE_EXPR:
+	  case GT_EXPR:
+	    return;
+	    
+	  default:
+	    restype = c_common_signed_type (restype);
+	    break;
+	  }
+
+      if (TREE_TYPE (primop1) != restype)
+	{
+	  /* Convert primop1 to target type, but do not introduce
+	     additional overflow.  We know primop1 is an int_cst.  */
+	  primop1 = force_fit_type (restype,
+				    wide_int::from
+				      (primop1,
+				       TYPE_PRECISION (restype),
+				       TYPE_SIGN (TREE_TYPE (primop1))),
+				    0, TREE_OVERFLOW (primop1));
+	}
+
+      tree type = c_common_signed_or_unsigned_type (unsignedp0,
+						    TREE_TYPE (primop0));
+      tree maxval = TYPE_MAX_VALUE (type);
+      tree minval = TYPE_MIN_VALUE (type);
+      if (type != restype)
+	{
+	  minval = convert (restype, minval);
+	  maxval = convert (restype, maxval);
+	}
+
+      bool min_gt = tree_int_cst_lt (primop1, minval);
+      bool max_gt = tree_int_cst_lt (primop1, maxval);
+      bool min_lt = tree_int_cst_lt (minval, primop1);
+      bool max_lt = tree_int_cst_lt (maxval, primop1);
+
+      tree val = 0;
+      switch (rescode)
+	{
+	case NE_EXPR:
+	  if (max_lt || min_gt)
+	    val = truthvalue_true_node;
+	  break;
+	case EQ_EXPR:
+	  if (max_lt || min_gt)
+	    val = truthvalue_false_node;
+	  break;
+	case LT_EXPR:
+	  if (max_lt)
+	    val = truthvalue_true_node;
+	  if (!min_lt)
+	    val = truthvalue_false_node;
+	  break;
+	case GT_EXPR:
+	  if (min_gt)
+	    val = truthvalue_true_node;
+	  if (!max_gt)
+	    val = truthvalue_false_node;
+	  break;
+	case LE_EXPR:
+	  if (!max_gt)
+	    val = truthvalue_true_node;
+	  if (min_gt)
+	    val = truthvalue_false_node;
+	  break;
+	case GE_EXPR:
+	  if (!min_lt)
+	    val = truthvalue_true_node;
+	  if (max_lt)
+	    val = truthvalue_false_node;
+	  break;
+
+	default:
+	  gcc_unreachable ();
+	}
+
+      if (val == truthvalue_false_node)
+	warning_at (loc, OPT_Wtype_limits,
+		    "comparison is always false due to limited range of data type");
+      else if (val == truthvalue_true_node)
+	warning_at (loc, OPT_Wtype_limits,
+		    "comparison is always true due to limited range of data type");
+    }
+  /* Here we must do the comparison on the nominal type
+     using the args exactly as we received them.  */
+  /* All unsigned values are >= 0, so we warn.  However,
+     if OP0 is a constant that is >= 0, the signedness of
+     the comparison isn't an issue, so suppress the
+     warning.  */
+  else if (integer_zerop (op1) && TYPE_UNSIGNED (restype)
+	   && !(TREE_CODE (op0) == INTEGER_CST
+		&& !TREE_OVERFLOW (convert (c_common_signed_type (restype),
+					    op0)))
+	   /* Do not warn for enumeration types.  */
+	   && (TREE_CODE (expr_original_type (op0)) != ENUMERAL_TYPE))
+    {      
+      switch (rescode)
+	{
+	case GE_EXPR:
+	  warning_at (loc, OPT_Wtype_limits,
+		      "comparison of unsigned expression >= 0 is always true");
+	  break;
+	  
+	case LT_EXPR:
+	  warning_at (loc, OPT_Wtype_limits,
+		      "comparison of unsigned expression < 0 is always false");
+	  break;
+	  
+	default:
+	  break;
+	}
+    }
+}
+
 /* Subroutine of build_binary_op, used for comparison operations.
    See if the operands have both been converted from subword integer types
    and, if so, perhaps change them both back to their original type.
    This function is also responsible for converting the two operands
    to the proper common type for comparison.
@@ -4447,10 +4688,12 @@ shorten_compare (location_t loc, tree *o
   int unsignedp0, unsignedp1;
   int real1, real2;
   tree primop0, primop1;
   enum tree_code code = *rescode_ptr;
 
+  warn_shorten_compare (loc, op0, op1, *restype_ptr, code);
+
   /* Throw away any conversions to wider types
      already present in the operands.  */
 
   primop0 = c_common_get_narrower (op0, &unsignedp0);
   primop1 = c_common_get_narrower (op1, &unsignedp1);
@@ -4648,24 +4891,10 @@ shorten_compare (location_t loc, tree *o
 		break;
 	      }
 	  type = c_common_unsigned_type (type);
 	}
 
-      if (TREE_CODE (primop0) != INTEGER_CST
-	  /* Don't warn if it's from a (non-system) macro.  */
-	  && !(from_macro_expansion_at
-	       (expansion_point_location_if_in_system_header
-		(EXPR_LOCATION (primop0)))))
-	{
-	  if (val == truthvalue_false_node)
-	    warning_at (loc, OPT_Wtype_limits,
-			"comparison is always false due to limited range of data type");
-	  if (val == truthvalue_true_node)
-	    warning_at (loc, OPT_Wtype_limits,
-			"comparison is always true due to limited range of data type");
-	}
-
       if (val != 0)
 	{
 	  /* Don't forget to evaluate PRIMOP0 if it has side effects.  */
 	  if (TREE_SIDE_EFFECTS (primop0))
 	    return build2 (COMPOUND_EXPR, TREE_TYPE (val), primop0, val);
@@ -4732,35 +4961,17 @@ shorten_compare (location_t loc, tree *o
 
       if (!real1 && !real2 && integer_zerop (primop1)
 	  && TYPE_UNSIGNED (*restype_ptr))
 	{
 	  tree value = 0;
-	  /* All unsigned values are >= 0, so we warn.  However,
-	     if OP0 is a constant that is >= 0, the signedness of
-	     the comparison isn't an issue, so suppress the
-	     warning.  */
-	  bool warn = 
-	    warn_type_limits && !in_system_header_at (loc)
-	    && !(TREE_CODE (primop0) == INTEGER_CST
-		 && !TREE_OVERFLOW (convert (c_common_signed_type (type),
-					     primop0)))
-	    /* Do not warn for enumeration types.  */
-	    && (TREE_CODE (expr_original_type (primop0)) != ENUMERAL_TYPE);
-	  
 	  switch (code)
 	    {
 	    case GE_EXPR:
-	      if (warn)
-		warning_at (loc, OPT_Wtype_limits,
-			    "comparison of unsigned expression >= 0 is always true");
 	      value = truthvalue_true_node;
 	      break;
 
 	    case LT_EXPR:
-	      if (warn)
-		warning_at (loc, OPT_Wtype_limits,
-			    "comparison of unsigned expression < 0 is always false");
 	      value = truthvalue_false_node;
 	      break;
 
 	    default:
 	      break;

Re: PATCH to shorten_compare -Wtype-limits handling

[Martin Sebor]

On 11/18/2015 09:26 PM, Jason Merrill wrote:
> The rs6000 target was hitting a bootstrap failure due to
> -Werror=type-limits.  Since warn_tautological_cmp and other warnings
> avoid warning if one of the operands comes from a macro, I thought it
> would make sense to do that here as well.

The also disables the warning for functions that are shadowed by
macros such as C atomic_load et al. For example, in the program
below. Is that an acceptable compromise or is there a way to avoid
this?

   #include <stdatomic.h>

   unsigned foo (unsigned char *x)
   {
     if (atomic_load (x) > 1000)
       return 0;
     return 1;
   }

At the same time, the change doesn't suppress the warning in other
cases where I would have expected it to suppress it based on your
description. For instance here:

   unsigned short bar (unsigned short x)
   {
   #define X x

     if (x > 0xffff)
       return 0;
     return 1;
   }

I noticed there is code elsewhere in c-common.c that avoids
issuing the same warning for system headers (that's the code
that responsible for issuing the warning for the second test
case above).

There is also code in tree-vrp.c that issues it unconditionally
regardless of macros or system headers.

Martin

Re: PATCH to shorten_compare -Wtype-limits handling

[Jason Merrill]

On 11/19/2015 02:44 PM, Martin Sebor wrote:
> On 11/18/2015 09:26 PM, Jason Merrill wrote:
>> The rs6000 target was hitting a bootstrap failure due to
>> -Werror=type-limits.  Since warn_tautological_cmp and other warnings
>> avoid warning if one of the operands comes from a macro, I thought it
>> would make sense to do that here as well.
>
> The also disables the warning for functions that are shadowed by
> macros such as C atomic_load et al. For example, in the program
> below. Is that an acceptable compromise or is there a way to avoid
> this?

I think it's an acceptable compromise, but see below.

> At the same time, the change doesn't suppress the warning in other
> cases where I would have expected it to suppress it based on your
> description. For instance here:
>
>    unsigned short bar (unsigned short x)
>    {
>    #define X x
>
>      if (x > 0xffff)

Yes, this is missed because the front end doesn't remember the location 
of the use of x; that's one of many location tracking issues.  David 
Malcolm is working on this stuff.

> I noticed there is code elsewhere in c-common.c that avoids
> issuing the same warning for system headers (that's the code
> that responsible for issuing the warning for the second test
> case above).

Hmm, it looks like using expansion_point_if_in_system_header might avoid 
the first issue you mention.

> There is also code in tree-vrp.c that issues it unconditionally
> regardless of macros or system headers.

Good to know.

Jason

Re: PATCH to shorten_compare -Wtype-limits handling

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On 11/19/2015 05:16 PM, Jason Merrill wrote:
> On 11/19/2015 02:44 PM, Martin Sebor wrote:
>> On 11/18/2015 09:26 PM, Jason Merrill wrote:
>>> The rs6000 target was hitting a bootstrap failure due to
>>> -Werror=type-limits.  Since warn_tautological_cmp and other warnings
>>> avoid warning if one of the operands comes from a macro, I thought it
>>> would make sense to do that here as well.
>>
>> The also disables the warning for functions that are shadowed by
>> macros such as C atomic_load et al. For example, in the program
>> below. Is that an acceptable compromise or is there a way to avoid
>> this?
>
> I think it's an acceptable compromise, but see below.
>
>> At the same time, the change doesn't suppress the warning in other
>> cases where I would have expected it to suppress it based on your
>> description. For instance here:
>>
>>    unsigned short bar (unsigned short x)
>>    {
>>    #define X x
>>
>>      if (x > 0xffff)
>
> Yes, this is missed because the front end doesn't remember the location
> of the use of x; that's one of many location tracking issues.  David
> Malcolm is working on this stuff.
>
>> I noticed there is code elsewhere in c-common.c that avoids
>> issuing the same warning for system headers (that's the code
>> that responsible for issuing the warning for the second test
>> case above).
>
> Hmm, it looks like using expansion_point_if_in_system_header might avoid
> the first issue you mention.

Thus.



[-- Attachment #2: type-lim2.patch --]
[-- Type: text/x-patch, Size: 1406 bytes --]

commit fb71bf4de520cc4bd11eefb57e50748b4679996f
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Nov 19 15:21:47 2015 -0500

    	* c-common.c (shorten_compare): But look through macros from
    	system headers.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 068a0bc..fe0a235 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -4651,8 +4651,10 @@ shorten_compare (location_t loc, tree *op0_ptr, tree *op1_ptr,
 	}
 
       if (TREE_CODE (primop0) != INTEGER_CST
-	  /* Don't warn if it's from a macro.  */
-	  && !from_macro_expansion_at (EXPR_LOCATION (primop0)))
+	  /* Don't warn if it's from a (non-system) macro.  */
+	  && !(from_macro_expansion_at
+	       (expansion_point_location_if_in_system_header
+		(EXPR_LOCATION (primop0)))))
 	{
 	  if (val == truthvalue_false_node)
 	    warning_at (loc, OPT_Wtype_limits,
diff --git a/gcc/testsuite/gcc.dg/Wtype-limits2.c b/gcc/testsuite/gcc.dg/Wtype-limits2.c
new file mode 100644
index 0000000..92151aa
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wtype-limits2.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-Wtype-limits" } */
+/* { dg-require-effective-target sync_char_short } */
+
+#include <stdatomic.h>
+
+unsigned foo (unsigned char *x)
+{
+  if (atomic_load (x) > 1000) /* { dg-warning "comparison is always false due to limited range of data type" } */
+    return 0;
+  return 1;
+}

Re: PATCH to shorten_compare -Wtype-limits handling

[Martin Sebor]

>> Hmm, it looks like using expansion_point_if_in_system_header might avoid
>> the first issue you mention.
>
> Thus.

Great, thanks! (I'll have to remember the trick for my own use!)

Martin

Re: PATCH to shorten_compare -Wtype-limits handling

[Manuel López-Ibáñez]

On 20 November 2015 at 16:10, Martin Sebor <msebor@gmail.com> wrote:
>>> Hmm, it looks like using expansion_point_if_in_system_header might avoid
>>> the first issue you mention.
>>
>>
>> Thus.
>
>
> Great, thanks! (I'll have to remember the trick for my own use!)

I added this to  https://gcc.gnu.org/wiki/DiagnosticsGuidelines under
"Locations" for future reference. I hope others would do the same in
the future, so the info is kept up-to-date.

Cheers,

Manuel.