[PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Martin Sebor]

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

The attached patch avoids infinite recursion when traversing phi
nodes in maybe_warn_alloc_args_overflow by using a bitmap to keep
track of those already visited and breaking out.

Thanks
Martin

[-- Attachment #2: gcc-78775.diff --]
[-- Type: text/x-patch, Size: 2705 bytes --]

PR tree-optimization/78775 - ICE in maybe_warn_alloc_args_overflow

gcc/ChangeLog:

	PR tree-optimization/78775
	* calls.c (operand_signed_p): Add overload and avoid getting into
	infinite recursion when traversing phi nodes.

gcc/testsuite/ChangeLog:

	PR tree-optimization/78775
	* gcc.dg/pr78775.c: New test.

Index: gcc/calls.c
===================================================================
--- gcc/calls.c	(revision 243581)
+++ gcc/calls.c	(working copy)
@@ -1247,10 +1247,12 @@ alloc_max_size (void)
 }
 
 /* Return true if the type of OP is signed, looking through any casts
-   to an unsigned type.  */
+   to an unsigned type.  VISITED is expected to be initially null and
+   is used internally by recursive calls of the function.  Caller
+   must free *VISITED if non-null after the function returns.  */
 
 static bool
-operand_signed_p (tree op)
+operand_signed_p (tree op, bitmap *visited)
 {
   if (TREE_CODE (op) == SSA_NAME)
     {
@@ -1265,6 +1267,12 @@ static bool
 	}
       else if (gimple_code (def) == GIMPLE_PHI)
 	{
+	  if (!*visited)
+	    *visited = BITMAP_ALLOC (NULL);
+
+	  if (!bitmap_set_bit (*visited, SSA_NAME_VERSION (op)))
+	    return true;
+
 	  /* In a phi, a constant argument may be unsigned even
 	     if in the source it's signed and negative.  Ignore
 	     those and consider the result of a phi signed if
@@ -1274,7 +1282,7 @@ static bool
 	    {
 	      tree op = gimple_phi_arg_def (def, i);
 	      if (TREE_CODE (op) != INTEGER_CST
-		  && !operand_signed_p (op))
+		  && !operand_signed_p (op, visited))
 		return false;
 	    }
 
@@ -1285,6 +1293,21 @@ static bool
   return !TYPE_UNSIGNED (TREE_TYPE (op));
 }
 
+/* Return true if the type of OP is signed, looking through any casts
+   to an unsigned type.  */
+
+static bool
+operand_signed_p (tree op)
+{
+  bitmap visited = NULL;
+  bool ret = operand_signed_p (op, &visited);
+
+  if (visited)
+    BITMAP_FREE (visited);
+
+  return ret;
+}
+
 /* Diagnose a call EXP to function FN decorated with attribute alloc_size
    whose argument numbers given by IDX with values given by ARGS exceed
    the maximum object size or cause an unsigned oveflow (wrapping) when
Index: gcc/testsuite/gcc.dg/pr78775.c
===================================================================
--- gcc/testsuite/gcc.dg/pr78775.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr78775.c	(working copy)
@@ -0,0 +1,19 @@
+/* PR c/78775 - [7 Regression] ICE in maybe_warn_alloc_args_overflow
+   { dg-do compile }
+   { dg-options "-O2" } */
+
+int a, b, *c;
+
+int main (void)
+{
+  unsigned long d = 0;
+  while (1)
+    {
+      switch (b)
+      case 'S':
+	d = a;
+      c = __builtin_malloc (d);
+    }
+
+  return 0;
+}

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Richard Biener]

On Tue, Dec 13, 2016 at 2:36 AM, Martin Sebor <msebor@gmail.com> wrote:
> The attached patch avoids infinite recursion when traversing phi
> nodes in maybe_warn_alloc_args_overflow by using a bitmap to keep
> track of those already visited and breaking out.

It looks somewhat excessive (the whole PHI node walk looks exponential in the
number of alloca calls given a good enough testcase).

It also looks like operand_signed_p really returns only a wild guess, neither
conservatively true or false.  Is that correct?

Can you instead scrap the weird anti-range handling please?

Thanks,
Richard.

> Thanks
> Martin

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Marek Polacek]

On Mon, Dec 12, 2016 at 06:36:16PM -0700, Martin Sebor wrote:
> +/* Return true if the type of OP is signed, looking through any casts
> +   to an unsigned type.  */
> +
> +static bool
> +operand_signed_p (tree op)
> +{
> +  bitmap visited = NULL;
> +  bool ret = operand_signed_p (op, &visited);
> +
> +  if (visited)
> +    BITMAP_FREE (visited);

I think you can drop the if before BITMAP_FREE here.

	Marek

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Jeff Law]

On 12/13/2016 03:52 AM, Marek Polacek wrote:
> On Mon, Dec 12, 2016 at 06:36:16PM -0700, Martin Sebor wrote:
>> +/* Return true if the type of OP is signed, looking through any casts
>> +   to an unsigned type.  */
>> +
>> +static bool
>> +operand_signed_p (tree op)
>> +{
>> +  bitmap visited = NULL;
>> +  bool ret = operand_signed_p (op, &visited);
>> +
>> +  if (visited)
>> +    BITMAP_FREE (visited);
>
> I think you can drop the if before BITMAP_FREE here.
Correct.
jeff

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Jeff Law]

On 12/12/2016 06:36 PM, Martin Sebor wrote:
> The attached patch avoids infinite recursion when traversing phi
> nodes in maybe_warn_alloc_args_overflow by using a bitmap to keep
> track of those already visited and breaking out.
>
> Thanks
> Martin
>
> gcc-78775.diff
>
>
> PR tree-optimization/78775 - ICE in maybe_warn_alloc_args_overflow
>
> gcc/ChangeLog:
>
> 	PR tree-optimization/78775
> 	* calls.c (operand_signed_p): Add overload and avoid getting into
> 	infinite recursion when traversing phi nodes.
>
> gcc/testsuite/ChangeLog:
>
> 	PR tree-optimization/78775
> 	* gcc.dg/pr78775.c: New test.
So Richi asked for removal of the VR_ANTI_RANGE handling, which would 
imply removal of operand_signed_p.

What are the implications if we do that?

Jeff

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Martin Sebor]

On 01/05/2017 11:03 AM, Jeff Law wrote:
> On 12/12/2016 06:36 PM, Martin Sebor wrote:
>> The attached patch avoids infinite recursion when traversing phi
>> nodes in maybe_warn_alloc_args_overflow by using a bitmap to keep
>> track of those already visited and breaking out.
>>
>> Thanks
>> Martin
>>
>> gcc-78775.diff
>>
>>
>> PR tree-optimization/78775 - ICE in maybe_warn_alloc_args_overflow
>>
>> gcc/ChangeLog:
>>
>>     PR tree-optimization/78775
>>     * calls.c (operand_signed_p): Add overload and avoid getting into
>>     infinite recursion when traversing phi nodes.
>>
>> gcc/testsuite/ChangeLog:
>>
>>     PR tree-optimization/78775
>>     * gcc.dg/pr78775.c: New test.
> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
> imply removal of operand_signed_p.
>
> What are the implications if we do that?

I just got back to this yesterday.  The implications of the removal
of the anti-range handling are a number of false negatives in the
test suite:

   FAIL: gcc.dg/attr-alloc_size-3.c  (test for warnings, line 448)
   FAIL: gcc.dg/attr-alloc_size-4.c  (test for warnings, line 137)
   FAIL: gcc.dg/attr-alloc_size-4.c  (test for warnings, line 139)
   FAIL: gcc.dg/attr-alloc_size-4.c  (test for warnings, line 175)

with the second one, for example, being:

   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
   p = malloc (n);

Passing signed integers as arguments to allocation functions is
common so I've been looking into how else to avoid the phi recursion
(which I assume is the concern here) without compromising this case.
Removing just the operand_signed_p() handling causes a number of
false positives in the test suite, such as for

   m = [-3, 7];
   n = [-5, 11];
   p = calloc (m, n);

which I suspect are common in the wild as well.

Martin

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Jeff Law]

On 01/05/2017 11:49 AM, Martin Sebor wrote:
> On 01/05/2017 11:03 AM, Jeff Law wrote:
>> On 12/12/2016 06:36 PM, Martin Sebor wrote:
>>> The attached patch avoids infinite recursion when traversing phi
>>> nodes in maybe_warn_alloc_args_overflow by using a bitmap to keep
>>> track of those already visited and breaking out.
>>>
>>> Thanks
>>> Martin
>>>
>>> gcc-78775.diff
>>>
>>>
>>> PR tree-optimization/78775 - ICE in maybe_warn_alloc_args_overflow
>>>
>>> gcc/ChangeLog:
>>>
>>>     PR tree-optimization/78775
>>>     * calls.c (operand_signed_p): Add overload and avoid getting into
>>>     infinite recursion when traversing phi nodes.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>     PR tree-optimization/78775
>>>     * gcc.dg/pr78775.c: New test.
>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>> imply removal of operand_signed_p.
>>
>> What are the implications if we do that?
>
> I just got back to this yesterday.  The implications of the removal
> of the anti-range handling are a number of false negatives in the
> test suite:
I was thinking more at a higher level.  ie, are the warnings still 
useful if we don't have the anti-range handling?  I suspect so, but 
would like to hear your opinion.


>
>   FAIL: gcc.dg/attr-alloc_size-3.c  (test for warnings, line 448)
>   FAIL: gcc.dg/attr-alloc_size-4.c  (test for warnings, line 137)
>   FAIL: gcc.dg/attr-alloc_size-4.c  (test for warnings, line 139)
>   FAIL: gcc.dg/attr-alloc_size-4.c  (test for warnings, line 175)
>
> with the second one, for example, being:
>
>   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
>   p = malloc (n);
Understood.  The low level question is do we get these kinds of ranges 
often enough in computations leading to allocation sizes?


>
> Passing signed integers as arguments to allocation functions is
> common so I've been looking into how else to avoid the phi recursion
> (which I assume is the concern here) without compromising this case.
> Removing just the operand_signed_p() handling causes a number of
> false positives in the test suite, such as for
Yes, passing signed integers as arguments is common.  But how often do 
we have one of these anti-ranges that allows us to do something useful?



>
>   m = [-3, 7];
>   n = [-5, 11];
>   p = calloc (m, n);
>
> which I suspect are common in the wild as well.
I must be missing something, given those ranges I wouldn't think we have 
a false positive.    The resulting size computation is going to have a 
range [-35, 88], right?  ISTM that we'd really want to warn for that.  I 
must be missing something.

Jeff

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Martin Sebor]

>>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>>> imply removal of operand_signed_p.
>>>
>>> What are the implications if we do that?
>>
>> I just got back to this yesterday.  The implications of the removal
>> of the anti-range handling are a number of false negatives in the
>> test suite:
> I was thinking more at a higher level.  ie, are the warnings still
> useful if we don't have the anti-range handling?  I suspect so, but
> would like to hear your opinion.
...
>>   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
>>   p = malloc (n);
> Understood.  The low level question is do we get these kinds of ranges
> often enough in computations leading to allocation sizes?

My intuition tells me that they are likely common enough not to
disregard but I don't have a lot of data to back it up with.  In
a Bash build a full 23% of all checked calls are of this kind (24
out of 106).  In a Binutils build only 4% are (9 out of 228).  In
Glibc, a little under 3%.  My guess is that the number will be
inversely proportional to the quality of the code.

>>   m = [-3, 7];
>>   n = [-5, 11];
>>   p = calloc (m, n);
>>
>> which I suspect are common in the wild as well.
> I must be missing something, given those ranges I wouldn't think we have
> a false positive.    The resulting size computation is going to have a
> range [-35, 88], right?  ISTM that we'd really want to warn for that.  I
> must be missing something.

The warning is meant to trigger only for cases of arguments that
are definitely too big (i.e., it's not a -Wmaybe-alloc-size-larger-
than type of warning).

Given a range with negative lower bound and a positive upper bound
the warning uses zero as the lower bound (it always ignores the upper
bound).  Doing otherwise would likely trigger lots of false positives.

(This is true for -Wstringop-overflow as well.)

The tradeoff, of course, is false negatives.  In the -Walloc-larger-
than case it can be mitigated by setting a lower threshold (I think
we might want to consider lowering the default to something less
liberal than PTRDIFF_MAX -- it seems very unlikely that a program
would try to allocate that much memory, especially in LP64).

Martin

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Jeff Law]

On 01/05/2017 08:52 PM, Martin Sebor wrote:
>>>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>>>> imply removal of operand_signed_p.
>>>>
>>>> What are the implications if we do that?
>>>
>>> I just got back to this yesterday.  The implications of the removal
>>> of the anti-range handling are a number of false negatives in the
>>> test suite:
>> I was thinking more at a higher level.  ie, are the warnings still
>> useful if we don't have the anti-range handling?  I suspect so, but
>> would like to hear your opinion.
> ...
>>>   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
>>>   p = malloc (n);
>> Understood.  The low level question is do we get these kinds of ranges
>> often enough in computations leading to allocation sizes?
>
> My intuition tells me that they are likely common enough not to
> disregard but I don't have a lot of data to back it up with.  In
> a Bash build a full 23% of all checked calls are of this kind (24
> out of 106).  In a Binutils build only 4% are (9 out of 228).  In
> Glibc, a little under 3%.  My guess is that the number will be
> inversely proportional to the quality of the code.
23% for bash is definitely concerning.

>
>>>   m = [-3, 7];
>>>   n = [-5, 11];
>>>   p = calloc (m, n);
>>>
>>> which I suspect are common in the wild as well.
>> I must be missing something, given those ranges I wouldn't think we have
>> a false positive.    The resulting size computation is going to have a
>> range [-35, 88], right?  ISTM that we'd really want to warn for that.  I
>> must be missing something.
>
> The warning is meant to trigger only for cases of arguments that
> are definitely too big (i.e., it's not a -Wmaybe-alloc-size-larger-
> than type of warning).
OK.  That's probably what I was missing.  I guess I should have gone 
back to the option documentation first.

So IIRC the range for any multiply is produced from the 4 cross 
products.  If you clamp the lower bound at 0, then 3 cross products drop 
to 0 and you get a range [0, u0 * u1]

And in that case you're not warning because we don't know it's 
definitely too big, right?

Let me ponder a bit too :-)

>
> The tradeoff, of course, is false negatives.  In the -Walloc-larger-
> than case it can be mitigated by setting a lower threshold (I think
> we might want to consider lowering the default to something less
> liberal than PTRDIFF_MAX -- it seems very unlikely that a program
> would try to allocate that much memory, especially in LP64).
Yea, the trick (of course) is finding a suitable value other than 
PTRDIFF_MAX.

jeff

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Jeff Law]

On 01/05/2017 08:52 PM, Martin Sebor wrote:
>>>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>>>> imply removal of operand_signed_p.
>>>>
>>>> What are the implications if we do that?
>>>
>>> I just got back to this yesterday.  The implications of the removal
>>> of the anti-range handling are a number of false negatives in the
>>> test suite:
>> I was thinking more at a higher level.  ie, are the warnings still
>> useful if we don't have the anti-range handling?  I suspect so, but
>> would like to hear your opinion.
> ...
>>>   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
>>>   p = malloc (n);
>> Understood.  The low level question is do we get these kinds of ranges
>> often enough in computations leading to allocation sizes?
>
> My intuition tells me that they are likely common enough not to
> disregard but I don't have a lot of data to back it up with.  In
> a Bash build a full 23% of all checked calls are of this kind (24
> out of 106).  In a Binutils build only 4% are (9 out of 228).  In
> Glibc, a little under 3%.  My guess is that the number will be
> inversely proportional to the quality of the code.
So I think you've made a case that we do want to handle this case.  So 
what's left is how best to avoid the infinite recursion and mitigate the 
pathological cases.

What you're computing seems to be "this object may have been derived 
from a signed type".  Right?  It's a property we can compute for any 
given SSA_NAME and it's not context/path specific beyond the 
context/path information encoded by the SSA graph.

So just thinking out load here, could we walk the IL once to identify 
call sites and build a worklist of SSA_NAMEs we care about.  Then we 
iterate on the worklist much like Aldy's code he's working on for the 
unswitching vs uninitialized variable issue?

Jeff

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Jeff Law]

Another approach would be to walk the SSA_NAME list and generate a 
bitmap of all the names which have a signed type or which were defined 
by a conversion to an unsigned type from a signed type.

At that point what's left is just the PHIs.  So we'd walk the dominator 
tree in RPO order to process the PHIs.  You don't have to recurse on the 
actual PHI arguments, just look at each one and see if it was already 
marked as being signed or derived from a signed type.  If all the args 
are marked as signed or derived from a signed type, then the PHI can be 
marked as well.

That may be more costly in the common case,  but should avoid the 
pathological cases Richi is worried about.

jeff

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Martin Sebor]

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

On 01/06/2017 09:45 AM, Jeff Law wrote:
> On 01/05/2017 08:52 PM, Martin Sebor wrote:
>>>>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>>>>> imply removal of operand_signed_p.
>>>>>
>>>>> What are the implications if we do that?
>>>>
>>>> I just got back to this yesterday.  The implications of the removal
>>>> of the anti-range handling are a number of false negatives in the
>>>> test suite:
>>> I was thinking more at a higher level.  ie, are the warnings still
>>> useful if we don't have the anti-range handling?  I suspect so, but
>>> would like to hear your opinion.
>> ...
>>>>   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
>>>>   p = malloc (n);
>>> Understood.  The low level question is do we get these kinds of ranges
>>> often enough in computations leading to allocation sizes?
>>
>> My intuition tells me that they are likely common enough not to
>> disregard but I don't have a lot of data to back it up with.  In
>> a Bash build a full 23% of all checked calls are of this kind (24
>> out of 106).  In a Binutils build only 4% are (9 out of 228).  In
>> Glibc, a little under 3%.  My guess is that the number will be
>> inversely proportional to the quality of the code.
> So I think you've made a case that we do want to handle this case.  So
> what's left is how best to avoid the infinite recursion and mitigate the
> pathological cases.
>
> What you're computing seems to be "this object may have been derived
> from a signed type".  Right?  It's a property we can compute for any
> given SSA_NAME and it's not context/path specific beyond the
> context/path information encoded by the SSA graph.
>
> So just thinking out load here, could we walk the IL once to identify
> call sites and build a worklist of SSA_NAMEs we care about.  Then we
> iterate on the worklist much like Aldy's code he's working on for the
> unswitching vs uninitialized variable issue?

Thanks for the suggestion.  It occurred to me while working on the fix
for 78973 (the non-bug) that size ranges should be handled the same by
-Wstringop-overflow as by -Walloc-size-larger-than, and that both have
the same problem: missing or incomplete support for anti-ranges.  The
attached patch moves get_size_range() from builtins.c to calls.c and
adds better support for anti-ranges.  That solves the problems also
lets it get rid of the objectionable operand_positive_p function.

Martin

PS The change to the alloc_max_size function is only needed to make
it possible to specify any argument to the -Walloc-size-larger-than
option, including 0 and -1, so that allocations of any size, including
zero can be flagged.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gcc-78775.diff --]
[-- Type: text/x-patch; name="gcc-78775.diff", Size: 19684 bytes --]

PR tree-optimization/78775 - [7 Regression] ICE in maybe_warn_alloc_args_overflow

gcc/ChangeLog:

	PR tree-optimization/78775
	* builtins.c (get_size_range): Move...
	* calls.c: ...to here.
	(alloc_max_size): Accept zero argument.
	(operand_signed_p): Remove.
	(maybe_warn_alloc_args_overflow): Call get_size_range.
	* calls.h (get_size_range): Declare.

gcc/testsuite/ChangeLog:

	PR tree-optimization/78775
	* gcc.dg/attr-alloc_size-4.c: Add test cases.
	* gcc.dg/pr78775.c: New test.
	* gcc.dg/pr78973-2.c: New test.
	* gcc.dg/pr78973.c: New test.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 5b76dfd..bf68e31 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -3031,42 +3031,6 @@ expand_builtin_memcpy_args (tree dest, tree src, tree len, rtx target, tree exp)
   return dest_addr;
 }
 
-/* Fill the 2-element RANGE array with the minimum and maximum values
-   EXP is known to have and return true, otherwise null and return
-   false.  */
-
-static bool
-get_size_range (tree exp, tree range[2])
-{
-  if (tree_fits_uhwi_p (exp))
-    {
-      range[0] = range[1] = exp;
-      return true;
-    }
-
-  if (TREE_CODE (exp) == SSA_NAME)
-    {
-      wide_int min, max;
-      enum value_range_type range_type = get_range_info (exp, &min, &max);
-
-      if (range_type == VR_RANGE)
-	{
-	  /* Interpret the bound in the variable's type.  */
-	  range[0] = wide_int_to_tree (TREE_TYPE (exp), min);
-	  range[1] = wide_int_to_tree (TREE_TYPE (exp), max);
-	  return true;
-	}
-      else if (range_type == VR_ANTI_RANGE)
-	{
-	  /* FIXME: Handle anti-ranges.  */
-	}
-    }
-
-  range[0] = NULL_TREE;
-  range[1] = NULL_TREE;
-  return false;
-}
-
 /* Try to verify that the sizes and lengths of the arguments to a string
    manipulation function given by EXP are within valid bounds and that
    the operation does not lead to buffer overflow.  Arguments other than
diff --git a/gcc/calls.c b/gcc/calls.c
index b7bbec5..c0d60bb 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1197,92 +1197,189 @@ alloc_max_size (void)
     {
       alloc_object_size_limit = TYPE_MAX_VALUE (ssizetype);
 
-      unsigned HOST_WIDE_INT unit = 1;
-
-      char *end;
-      errno = 0;
-      unsigned HOST_WIDE_INT limit
-	= warn_alloc_size_limit ? strtoull (warn_alloc_size_limit, &end, 10) : 0;
-
-      if (limit && !errno)
+      if (warn_alloc_size_limit)
 	{
-	  if (end && *end)
+	  char *end = NULL;
+	  errno = 0;
+	  unsigned HOST_WIDE_INT unit = 1;
+	  unsigned HOST_WIDE_INT limit
+	    = strtoull (warn_alloc_size_limit, &end, 10);
+
+	  if (!errno)
 	    {
-	      /* Numeric option arguments are at most INT_MAX.  Make it
-		 possible to specify a larger value by accepting common
-		 suffixes.  */
-	      if (!strcmp (end, "kB"))
-		unit = 1000;
-	      else if (!strcasecmp (end, "KiB") || strcmp (end, "KB"))
-		unit = 1024;
-	      else if (!strcmp (end, "MB"))
-		unit = 1000LU * 1000;
-	      else if (!strcasecmp (end, "MiB"))
-		unit = 1024LU * 1024;
-	      else if (!strcasecmp (end, "GB"))
-		unit = 1000LU * 1000 * 1000;
-	      else if (!strcasecmp (end, "GiB"))
-		unit = 1024LU * 1024 * 1024;
-	      else if (!strcasecmp (end, "TB"))
-		unit = 1000LU * 1000 * 1000 * 1000;
-	      else if (!strcasecmp (end, "TiB"))
-		unit = 1024LU * 1024 * 1024 * 1024;
-	      else if (!strcasecmp (end, "PB"))
-		unit = 1000LU * 1000 * 1000 * 1000 * 1000;
-	      else if (!strcasecmp (end, "PiB"))
-		unit = 1024LU * 1024 * 1024 * 1024 * 1024;
-	      else if (!strcasecmp (end, "EB"))
-		unit = 1000LU * 1000 * 1000 * 1000 * 1000 * 1000;
-	      else if (!strcasecmp (end, "EiB"))
-		unit = 1024LU * 1024 * 1024 * 1024 * 1024 * 1024;
-	      else
-		unit = 0;
-	    }
+	      if (end && *end)
+		{
+		  /* Numeric option arguments are at most INT_MAX.  Make it
+		     possible to specify a larger value by accepting common
+		     suffixes.  */
+		  if (!strcmp (end, "kB"))
+		    unit = 1000;
+		  else if (!strcasecmp (end, "KiB") || strcmp (end, "KB"))
+		    unit = 1024;
+		  else if (!strcmp (end, "MB"))
+		    unit = 1000LU * 1000;
+		  else if (!strcasecmp (end, "MiB"))
+		    unit = 1024LU * 1024;
+		  else if (!strcasecmp (end, "GB"))
+		    unit = 1000LU * 1000 * 1000;
+		  else if (!strcasecmp (end, "GiB"))
+		    unit = 1024LU * 1024 * 1024;
+		  else if (!strcasecmp (end, "TB"))
+		    unit = 1000LU * 1000 * 1000 * 1000;
+		  else if (!strcasecmp (end, "TiB"))
+		    unit = 1024LU * 1024 * 1024 * 1024;
+		  else if (!strcasecmp (end, "PB"))
+		    unit = 1000LU * 1000 * 1000 * 1000 * 1000;
+		  else if (!strcasecmp (end, "PiB"))
+		    unit = 1024LU * 1024 * 1024 * 1024 * 1024;
+		  else if (!strcasecmp (end, "EB"))
+		    unit = 1000LU * 1000 * 1000 * 1000 * 1000 * 1000;
+		  else if (!strcasecmp (end, "EiB"))
+		    unit = 1024LU * 1024 * 1024 * 1024 * 1024 * 1024;
+		  else
+		    unit = 0;
+		}
 
-	  if (unit)
-	    alloc_object_size_limit = build_int_cst (ssizetype, limit * unit);
+	      if (unit)
+		alloc_object_size_limit
+		  = build_int_cst (ssizetype, limit * unit);
+	    }
 	}
     }
   return alloc_object_size_limit;
 }
 
-/* Return true if the type of OP is signed, looking through any casts
-   to an unsigned type.  */
+/* Return true when EXP's range can be determined and set RANGE[] to it
+   after adjusting it if necessary to make EXP a valid size argument to
+   an allocation function declared with attribute alloc_size (whose
+   argument may be signed), or to a string manipulation function like
+   memset.  SIGNED_P is initially set to -1 and is used internally by
+   the function and should not be explicitly passed in by callers.  */
 
-static bool
-operand_signed_p (tree op)
+bool
+get_size_range (tree exp, tree range[2], int signed_p /* = -1 */)
 {
-  if (TREE_CODE (op) == SSA_NAME)
+  if (tree_fits_uhwi_p (exp))
+    {
+      /* EXP is a constant.  */
+      range[0] = range[1] = exp;
+      return true;
+    }
+
+  if (TREE_CODE (exp) != SSA_NAME)
+    {
+      /* No range information available. */
+      range[0] = NULL_TREE;
+      range[1] = NULL_TREE;
+      return false;
+    }
+
+  wide_int min, max;
+  enum value_range_type range_type = get_range_info (exp, &min, &max);
+
+  tree exptype = TREE_TYPE (exp);
+  unsigned expprec = TYPE_PRECISION (exptype);
+  wide_int wzero = wi::zero (expprec);
+
+  /* Set SIGNED_P once (will be used by recursive calls).  */
+  if (signed_p < 0)
+    signed_p = !TYPE_UNSIGNED (exptype);
+
+  if (range_type == VR_VARYING)
     {
-      gimple *def = SSA_NAME_DEF_STMT (op);
-      if (is_gimple_assign (def))
+      /* No range information available. */
+      range[0] = NULL_TREE;
+      range[1] = NULL_TREE;
+      return false;
+    }
+
+  if (range_type == VR_ANTI_RANGE)
+    {
+      if (TYPE_UNSIGNED (exptype) || wi::les_p (wzero, min))
 	{
-	  /* In an assignment involving a cast, ignore the type
-	     of the cast and consider the type of its  operand.  */
-	  tree_code code = gimple_assign_rhs_code (def);
-	  if (code == NOP_EXPR)
-	    op = gimple_assign_rhs1 (def);
+	  /* EXP is either unsigned or strictly not in an unsigned
+	     range.  Set the result range using zero as the lower
+	     bound if that's still a range or the upper bound if
+	     using zero would result in [0, 0].  */
+	  if (wi::lts_p (wzero, min - 1))
+	    {
+	      max = min - 1;
+	      min = wzero;
+	    }
+	  else
+	    {
+	      min = max + 1;
+	      max = wide_int (TYPE_MAX_VALUE (exptype));
+	    }
 	}
-      else if (gimple_code (def) == GIMPLE_PHI)
+      else if (wi::les_p (max, wzero))
 	{
-	  /* In a phi, a constant argument may be unsigned even
-	     if in the source it's signed and negative.  Ignore
-	     those and consider the result of a phi signed if
-	     all its non-constant operands are.  */
-	  unsigned nargs = gimple_phi_num_args (def);
-	  for (unsigned i = 0; i != nargs; ++i)
+	  /* EXP is not in a strictly negative range.  That means
+	     it must be in some (not necessarily strictly) positive
+	     range which includes zero.  Since in signed to unsigned
+	     conversions negative values end up converted to large
+	     positive values, and otherwise they are not valid sizes,
+	     the resulting range is in both cases [0, TYPE_MAX].  */
+	  min = wzero;
+	  max = wide_int (TYPE_MAX_VALUE (exptype));
+	}
+      else if (signed_p)
+	{
+	  /* EXP is not in a negative-positive range and no conversion
+	     is being performed.  That means EXP is either negative,
+	     or greater than max.  Since negative sizes are invalid
+	     make the range [MAX + 1, TYPE_MAX].  */
+	  min = max + 1;
+	  max = wide_int (TYPE_MAX_VALUE (exptype));
+	}
+      else
+	{
+	  /* EXP is not in negative-positive range and it's being
+	     converted to an unsigned.  That means the negative
+	     lower bound is inverted and the resulting range is
+	     either [~(MIN - 1), MAX + 1] or [MAX + 1, ~(MIN - 1)],
+	     whichever results in the lower bound being less than
+	     the upper bound.  */
+	  --min;
+	  ++max;
+
+	  /* For a signed to unsigned conversion invert
+	     the minimum.  */
+	  min = ~min;
+
+	  if (wi::ltu_p (max, min))
 	    {
-	      tree op = gimple_phi_arg_def (def, i);
-	      if (TREE_CODE (op) != INTEGER_CST
-		  && !operand_signed_p (op))
-		return false;
+	      wide_int tmp = max;
+	      max = min;
+	      min = tmp;
 	    }
-
-	  return true;
 	}
     }
 
-  return !TYPE_UNSIGNED (TREE_TYPE (op));
+  if (TYPE_UNSIGNED (exptype) || !wi::neg_p (min) || signed_p)
+    {
+      /* EXP is either unsigned or in a non-negative range,
+	 or the result is not being converted to unsigned.
+	 Use its range as is.  */
+      range[0] = wide_int_to_tree (exptype, min);
+      range[1] = wide_int_to_tree (exptype, max);
+    }
+  else if (wi::neg_p (min) && wi::les_p (max, wzero))
+    {
+      /* EXP is in a strictly negative range (or zero) and is
+	 being converted to unsigned.  Also use its range as is.  */
+      range[0] = wide_int_to_tree (exptype, min);
+      range[1] = wide_int_to_tree (exptype, max);
+    }
+  else
+    {
+      /* Otherwise, clear the lower bound if it's negative and
+	 the upper bound is positive.  */
+      range[0] = integer_zero_node;
+      range[1] = wide_int_to_tree (exptype, max);
+    }
+
+  return true;
 }
 
 /* Diagnose a call EXP to function FN decorated with attribute alloc_size
@@ -1316,8 +1413,8 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp, tree args[2], int idx[2])
 	  if (tree_int_cst_lt (args[i], integer_zero_node))
 	    {
 	      warned = warning_at (loc, OPT_Walloc_size_larger_than_,
-				   "argument %i value %qE is negative",
-				   idx[i] + 1, args[i]);
+				   "%Kargument %i value %qE is negative",
+				   exp, idx[i] + 1, args[i]);
 	    }
 	  else if (integer_zerop (args[i]))
 	    {
@@ -1334,8 +1431,8 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp, tree args[2], int idx[2])
 		      && !lookup_attribute ("returns_nonnull",
 					    TYPE_ATTRIBUTES (TREE_TYPE (fn)))))
 		warned = warning_at (loc, OPT_Walloc_zero,
-				     "argument %i value is zero",
-				     idx[i] + 1);
+				     "%Kargument %i value is zero",
+				     exp, idx[i] + 1);
 	    }
 	  else if (tree_int_cst_lt (maxobjsize, args[i]))
 	    {
@@ -1351,79 +1448,31 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp, tree args[2], int idx[2])
 		continue;
 
 	      warned = warning_at (loc, OPT_Walloc_size_larger_than_,
-				   "argument %i value %qE exceeds "
+				   "%Kargument %i value %qE exceeds "
 				   "maximum object size %E",
-				   idx[i] + 1, args[i], maxobjsize);
+				   exp, idx[i] + 1, args[i], maxobjsize);
 	    }
 	}
-      else if (TREE_CODE (args[i]) == SSA_NAME)
+      else if (TREE_CODE (args[i]) == SSA_NAME
+	       && get_size_range (args[i], argrange[i]))
 	{
-	  tree type = TREE_TYPE (args[i]);
-
-	  wide_int min, max;
-	  value_range_type range_type = get_range_info (args[i], &min, &max);
-	  if (range_type == VR_RANGE)
-	    {
-	      argrange[i][0] = wide_int_to_tree (type, min);
-	      argrange[i][1] = wide_int_to_tree (type, max);
-	    }
-	  else if (range_type == VR_ANTI_RANGE)
-	    {
-	      /* For an anti-range, if the type of the formal argument
-		 is unsigned and the bounds of the range are of opposite
-		 signs when interpreted as signed, check to see if the
-		 type of the actual argument is signed.  If so, the lower
-		 bound must be taken to be zero (rather than a large
-		 positive value corresonding to the actual lower bound
-		 interpreted as unsigned) and there is nothing else that
-		 can be inferred from it.  */
-	      --min;
-	      ++max;
-	      wide_int zero = wi::uhwi (0, TYPE_PRECISION (type));
-	      if (TYPE_UNSIGNED (type)
-		  && wi::lts_p (zero, min) && wi::lts_p (max, zero)
-		  && operand_signed_p (args[i]))
-		continue;
-
-	      argrange[i][0] = wide_int_to_tree (type, max);
-	      argrange[i][1] = wide_int_to_tree (type, min);
-
-	      /* Verify that the anti-range doesn't make all arguments
-		 invalid (treat the anti-range ~[0, 0] as invalid).  */
-	      if (tree_int_cst_lt (maxobjsize, argrange[i][0])
-		  && tree_int_cst_le (argrange[i][1], integer_zero_node))
-		{
-		  warned
-		    = warning_at (loc, OPT_Walloc_size_larger_than_,
-				  (TYPE_UNSIGNED (type)
-				   ? G_("argument %i range [%E, %E] exceeds "
-					"maximum object size %E")
-				   : G_("argument %i range [%E, %E] is both "
-					"negative and exceeds maximum object "
-					"size %E")),
-				  idx[i] + 1, argrange[i][0],
-				  argrange[i][1], maxobjsize);
-		}
-	      continue;
-	    }
-	  else
-	    continue;
-
 	  /* Verify that the argument's range is not negative (including
 	     upper bound of zero).  */
 	  if (tree_int_cst_lt (argrange[i][0], integer_zero_node)
 	      && tree_int_cst_le (argrange[i][1], integer_zero_node))
 	    {
 	      warned = warning_at (loc, OPT_Walloc_size_larger_than_,
-				   "argument %i range [%E, %E] is negative",
-				   idx[i] + 1, argrange[i][0], argrange[i][1]);
+				   "%Kargument %i range [%E, %E] is negative",
+				   exp, idx[i] + 1,
+				   argrange[i][0], argrange[i][1]);
 	    }
 	  else if (tree_int_cst_lt (maxobjsize, argrange[i][0]))
 	    {
 	      warned = warning_at (loc, OPT_Walloc_size_larger_than_,
-				   "argument %i range [%E, %E] exceeds "
+				   "%Kargument %i range [%E, %E] exceeds "
 				   "maximum object size %E",
-				   idx[i] + 1, argrange[i][0], argrange[i][1],
+				   exp, idx[i] + 1,
+				   argrange[i][0], argrange[i][1],
 				   maxobjsize);
 	    }
 	}
@@ -1450,15 +1499,15 @@ maybe_warn_alloc_args_overflow (tree fn, tree exp, tree args[2], int idx[2])
 
       if (vflow)
 	warned = warning_at (loc, OPT_Walloc_size_larger_than_,
-			     "product %<%E * %E%> of arguments %i and %i "
+			     "%Kproduct %<%E * %E%> of arguments %i and %i "
 			     "exceeds %<SIZE_MAX%>",
-			     argrange[0][0], argrange[1][0],
+			     exp, argrange[0][0], argrange[1][0],
 			     idx[0] + 1, idx[1] + 1);
       else if (wi::ltu_p (wi::to_wide (maxobjsize, szprec), prod))
 	warned = warning_at (loc, OPT_Walloc_size_larger_than_,
-			     "product %<%E * %E%> of arguments %i and %i "
+			     "%Kproduct %<%E * %E%> of arguments %i and %i "
 			     "exceeds maximum object size %E",
-			     argrange[0][0], argrange[1][0],
+			     exp, argrange[0][0], argrange[1][0],
 			     idx[0] + 1, idx[1] + 1,
 			     maxobjsize);
 
diff --git a/gcc/calls.h b/gcc/calls.h
index e87fbda..ed4b75b 100644
--- a/gcc/calls.h
+++ b/gcc/calls.h
@@ -38,5 +38,6 @@ extern bool pass_by_reference (CUMULATIVE_ARGS *, machine_mode,
 extern bool reference_callee_copied (CUMULATIVE_ARGS *, machine_mode,
 				     tree, bool);
 extern void maybe_warn_alloc_args_overflow (tree, tree, tree[2], int[2]);
+extern bool get_size_range (tree, tree[2], int = -1);
 
 #endif // GCC_CALLS_H
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size-4.c b/gcc/testsuite/gcc.dg/attr-alloc_size-4.c
index 6b70a85..5ce593e 100644
--- a/gcc/testsuite/gcc.dg/attr-alloc_size-4.c
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size-4.c
@@ -128,15 +128,22 @@ test_int_range (int n)
 
   sink (f_int_1 (SR (min, 1234)));
   sink (f_int_1 (SR (-2, -1)));   /* { dg-warning "argument 1 range \\\[-2, -1\\\] is negative" } */
+
   sink (f_int_1 (SR (1235, 2345)));  /* { dg-warning "argument 1 range \\\[1235, 2345\\\] exceeds maximum object size 1234" } */
   sink (f_int_1 (SR (max - 1, max)));   /* { dg-warning "argument 1 range \\\[\[0-9\]+, \[0-9\]+\\\] exceeds maximum object size 1234" } */
 
   sink (f_int_1 (SAR (-1, 1)));
   sink (f_int_1 (SAR (-2, 12)));
   sink (f_int_1 (SAR (-3, 123)));
-  sink (f_int_1 (SAR (-4, 1234)));   /* { dg-warning "argument 1 range \\\[1235, -5\\\] is both negative and exceeds maximum object size 1234" } */
+  sink (f_int_1 (SAR (-4, 1234)));   /* { dg-warning "argument 1 range \\\[1235, \[0-9\]+\\\] exceeds maximum object size 1234" } */
   sink (f_int_1 (SAR (min + 1, 1233)));
-  sink (f_int_1 (SAR (min + 2, 1235)));   /* { dg-warning "argument 1 range \\\[1236, -\[0-9\]+\\\] is both negative and exceeds maximum object size 1234" } */
+  sink (f_int_1 (SAR (min + 2, 1235)));   /* { dg-warning "argument 1 range \\\[1236, \[0-9\]+\\\] exceeds maximum object size 1234" } */
+  sink (f_int_1 (SAR (0, max)));   /* { dg-warning "argument 1 range \\\[-\[0-9\]*, -1\\\] is negative" } */
+  /* The range below includes zero which would be diagnosed by
+     -Walloc-size-zero but since all other values are negative it
+     is diagnosed by -Walloc-size-larger-than.  */
+  sink (f_int_1 (SAR (1, max)));   /* { dg-warning "argument 1 range \\\[-\[0-9\]*, 0\\\] is negative" } */
+  sink (f_int_1 (SAR (2, max)));
 }
 
 void
diff --git a/gcc/testsuite/gcc.dg/pr78775.c b/gcc/testsuite/gcc.dg/pr78775.c
new file mode 100644
index 0000000..120c252
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr78775.c
@@ -0,0 +1,19 @@
+/* PR c/78775 - [7 Regression] ICE in maybe_warn_alloc_args_overflow
+   { dg-do compile }
+   { dg-options "-O2" } */
+
+int a, b, *c;
+
+int main (void)
+{
+  unsigned long d = 0;
+  while (1)
+    {
+      switch (b)
+      case 'S':
+	d = a;
+      c = __builtin_malloc (d);
+    }
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/pr78973-2.c b/gcc/testsuite/gcc.dg/pr78973-2.c
new file mode 100644
index 0000000..dadec16
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr78973-2.c
@@ -0,0 +1,25 @@
+/* PR c/78973 - warning: ‘memcpy’: specified size exceeds maximum object
+   size [-Wstringop-overflow=]
+
+   This is a companion test for the bug above that verifies that the correct
+   range of the int variable is detected.
+
+   { dg-do compile }
+   { dg-require-effective-target int32plus }
+   { dg-options "-O2 -Walloc-size-larger-than=4" }  */
+
+void *p;
+
+void f (int n)
+{
+  if (n <= 4)
+    p = __builtin_malloc (n);
+  /* { dg-warning "argument 1 range \\\[5, 2147483647\\\] exceeds maximum object size 4" "ilp32" { xfail { ! lp64 } } .-1 } */
+}
+
+void g (unsigned n)
+{
+  if (n < 5)
+    n = 5;
+  f (n);
+}
diff --git a/gcc/testsuite/gcc.dg/pr78973.c b/gcc/testsuite/gcc.dg/pr78973.c
new file mode 100644
index 0000000..a6195f0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr78973.c
@@ -0,0 +1,20 @@
+/* PR c/78973 - warning: ‘memcpy’: specified size exceeds maximum object size
+
+   Test case for what was initially thought to be a false positive but after
+   deeper investigation turned out to be a true positive.
+
+   { dg-do compile }
+   { dg-options "-O2 -Wall" }  */
+
+void f (void *p, int n)
+{
+  if (n <= 4)
+    __builtin_memset (p, 0, n);   /* { dg-warning "exceeds maximum object size" } */
+}
+
+void g (void *d, unsigned n)
+{
+  if (n < 5)
+    n = 5;
+  f (d, n);
+}

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Jeff Law]

On 01/08/2017 02:04 PM, Martin Sebor wrote:
> On 01/06/2017 09:45 AM, Jeff Law wrote:
>> On 01/05/2017 08:52 PM, Martin Sebor wrote:
>>>>>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>>>>>> imply removal of operand_signed_p.
>>>>>>
>>>>>> What are the implications if we do that?
>>>>>
>>>>> I just got back to this yesterday.  The implications of the removal
>>>>> of the anti-range handling are a number of false negatives in the
>>>>> test suite:
>>>> I was thinking more at a higher level.  ie, are the warnings still
>>>> useful if we don't have the anti-range handling?  I suspect so, but
>>>> would like to hear your opinion.
>>> ...
>>>>>   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
>>>>>   p = malloc (n);
>>>> Understood.  The low level question is do we get these kinds of ranges
>>>> often enough in computations leading to allocation sizes?
>>>
>>> My intuition tells me that they are likely common enough not to
>>> disregard but I don't have a lot of data to back it up with.  In
>>> a Bash build a full 23% of all checked calls are of this kind (24
>>> out of 106).  In a Binutils build only 4% are (9 out of 228).  In
>>> Glibc, a little under 3%.  My guess is that the number will be
>>> inversely proportional to the quality of the code.
>> So I think you've made a case that we do want to handle this case.  So
>> what's left is how best to avoid the infinite recursion and mitigate the
>> pathological cases.
>>
>> What you're computing seems to be "this object may have been derived
>> from a signed type".  Right?  It's a property we can compute for any
>> given SSA_NAME and it's not context/path specific beyond the
>> context/path information encoded by the SSA graph.
>>
>> So just thinking out load here, could we walk the IL once to identify
>> call sites and build a worklist of SSA_NAMEs we care about.  Then we
>> iterate on the worklist much like Aldy's code he's working on for the
>> unswitching vs uninitialized variable issue?
>
> Thanks for the suggestion.  It occurred to me while working on the fix
> for 78973 (the non-bug) that size ranges should be handled the same by
> -Wstringop-overflow as by -Walloc-size-larger-than, and that both have
> the same problem: missing or incomplete support for anti-ranges.  The
> attached patch moves get_size_range() from builtins.c to calls.c and
> adds better support for anti-ranges.  That solves the problems also
> lets it get rid of the objectionable operand_positive_p function.
>
> Martin
>
> PS The change to the alloc_max_size function is only needed to make
> it possible to specify any argument to the -Walloc-size-larger-than
> option, including 0 and -1, so that allocations of any size, including
> zero can be flagged.
>
> gcc-78775.diff
>
>
> PR tree-optimization/78775 - [7 Regression] ICE in maybe_warn_alloc_args_overflow
>
> gcc/ChangeLog:
>
> 	PR tree-optimization/78775
> 	* builtins.c (get_size_range): Move...
> 	* calls.c: ...to here.
> 	(alloc_max_size): Accept zero argument.
> 	(operand_signed_p): Remove.
> 	(maybe_warn_alloc_args_overflow): Call get_size_range.
> 	* calls.h (get_size_range): Declare.
>
> gcc/testsuite/ChangeLog:
>
> 	PR tree-optimization/78775
> 	* gcc.dg/attr-alloc_size-4.c: Add test cases.
> 	* gcc.dg/pr78775.c: New test.
> 	* gcc.dg/pr78973-2.c: New test.
> 	* gcc.dg/pr78973.c: New test.
>


> +
> +  wide_int min, max;
> +  enum value_range_type range_type = get_range_info (exp, &min, &max);
> +
> +  tree exptype = TREE_TYPE (exp);
> +  unsigned expprec = TYPE_PRECISION (exptype);
> +  wide_int wzero = wi::zero (expprec);
> +
> +  /* Set SIGNED_P once (will be used by recursive calls).  */
> +  if (signed_p < 0)
> +    signed_p = !TYPE_UNSIGNED (exptype);
> +
> +  if (range_type == VR_VARYING)
>      {
> -      gimple *def = SSA_NAME_DEF_STMT (op);
> -      if (is_gimple_assign (def))
> +      /* No range information available. */
> +      range[0] = NULL_TREE;
> +      range[1] = NULL_TREE;
> +      return false;
> +    }
Might as well move this range_type test earlier since it doesn't use 
exptype, expprec, wzero or signed_p.

OK with that change.

jeff

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Christophe Lyon]

Hi Martin,

On 9 January 2017 at 04:14, Jeff Law <law@redhat.com> wrote:
> On 01/08/2017 02:04 PM, Martin Sebor wrote:
>>
>> On 01/06/2017 09:45 AM, Jeff Law wrote:
>>>
>>> On 01/05/2017 08:52 PM, Martin Sebor wrote:
>>>>>>>
>>>>>>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>>>>>>> imply removal of operand_signed_p.
>>>>>>>
>>>>>>> What are the implications if we do that?
>>>>>>
>>>>>>
>>>>>> I just got back to this yesterday.  The implications of the removal
>>>>>> of the anti-range handling are a number of false negatives in the
>>>>>> test suite:
>>>>>
>>>>> I was thinking more at a higher level.  ie, are the warnings still
>>>>> useful if we don't have the anti-range handling?  I suspect so, but
>>>>> would like to hear your opinion.
>>>>
>>>> ...
>>>>>>
>>>>>>   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
>>>>>>   p = malloc (n);
>>>>>
>>>>> Understood.  The low level question is do we get these kinds of ranges
>>>>> often enough in computations leading to allocation sizes?
>>>>
>>>>
>>>> My intuition tells me that they are likely common enough not to
>>>> disregard but I don't have a lot of data to back it up with.  In
>>>> a Bash build a full 23% of all checked calls are of this kind (24
>>>> out of 106).  In a Binutils build only 4% are (9 out of 228).  In
>>>> Glibc, a little under 3%.  My guess is that the number will be
>>>> inversely proportional to the quality of the code.
>>>
>>> So I think you've made a case that we do want to handle this case.  So
>>> what's left is how best to avoid the infinite recursion and mitigate the
>>> pathological cases.
>>>
>>> What you're computing seems to be "this object may have been derived
>>> from a signed type".  Right?  It's a property we can compute for any
>>> given SSA_NAME and it's not context/path specific beyond the
>>> context/path information encoded by the SSA graph.
>>>
>>> So just thinking out load here, could we walk the IL once to identify
>>> call sites and build a worklist of SSA_NAMEs we care about.  Then we
>>> iterate on the worklist much like Aldy's code he's working on for the
>>> unswitching vs uninitialized variable issue?
>>
>>
>> Thanks for the suggestion.  It occurred to me while working on the fix
>> for 78973 (the non-bug) that size ranges should be handled the same by
>> -Wstringop-overflow as by -Walloc-size-larger-than, and that both have
>> the same problem: missing or incomplete support for anti-ranges.  The
>> attached patch moves get_size_range() from builtins.c to calls.c and
>> adds better support for anti-ranges.  That solves the problems also
>> lets it get rid of the objectionable operand_positive_p function.
>>
>> Martin
>>
>> PS The change to the alloc_max_size function is only needed to make
>> it possible to specify any argument to the -Walloc-size-larger-than
>> option, including 0 and -1, so that allocations of any size, including
>> zero can be flagged.
>>
>> gcc-78775.diff
>>
>>
>> PR tree-optimization/78775 - [7 Regression] ICE in
>> maybe_warn_alloc_args_overflow
>>
>> gcc/ChangeLog:
>>
>>         PR tree-optimization/78775
>>         * builtins.c (get_size_range): Move...
>>         * calls.c: ...to here.
>>         (alloc_max_size): Accept zero argument.
>>         (operand_signed_p): Remove.
>>         (maybe_warn_alloc_args_overflow): Call get_size_range.
>>         * calls.h (get_size_range): Declare.
>>
>> gcc/testsuite/ChangeLog:
>>
>>         PR tree-optimization/78775
>>         * gcc.dg/attr-alloc_size-4.c: Add test cases.
>>         * gcc.dg/pr78775.c: New test.
>>         * gcc.dg/pr78973-2.c: New test.
>>         * gcc.dg/pr78973.c: New test.
>>
>

The new test (gcc.dg/pr78973.c) fails on arm targets (there's no warning).

In addition, I have noticed a new failure:
  gcc.dg/attr-alloc_size-4.c  (test for warnings, line 140)
on target arm-none-linux-gnueabihf --with-cpu=cortex-a5
(works fine --with-cpu=cortex-a9)


Christophe

>
>> +
>> +  wide_int min, max;
>> +  enum value_range_type range_type = get_range_info (exp, &min, &max);
>> +
>> +  tree exptype = TREE_TYPE (exp);
>> +  unsigned expprec = TYPE_PRECISION (exptype);
>> +  wide_int wzero = wi::zero (expprec);
>> +
>> +  /* Set SIGNED_P once (will be used by recursive calls).  */
>> +  if (signed_p < 0)
>> +    signed_p = !TYPE_UNSIGNED (exptype);
>> +
>> +  if (range_type == VR_VARYING)
>>      {
>> -      gimple *def = SSA_NAME_DEF_STMT (op);
>> -      if (is_gimple_assign (def))
>> +      /* No range information available. */
>> +      range[0] = NULL_TREE;
>> +      range[1] = NULL_TREE;
>> +      return false;
>> +    }
>
> Might as well move this range_type test earlier since it doesn't use
> exptype, expprec, wzero or signed_p.
>
> OK with that change.
>
> jeff
>
>

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Andreas Schwab]

On Jan 11 2017, Christophe Lyon <christophe.lyon@linaro.org> wrote:

> The new test (gcc.dg/pr78973.c) fails on arm targets (there's no warning).

Also fails on m68k.

> In addition, I have noticed a new failure:
>   gcc.dg/attr-alloc_size-4.c  (test for warnings, line 140)
> on target arm-none-linux-gnueabihf --with-cpu=cortex-a5
> (works fine --with-cpu=cortex-a9)

Dito.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] avoid infinite recursion in maybe_warn_alloc_args_overflow (pr 78775)

[Martin Sebor]

On 01/11/2017 02:05 AM, Christophe Lyon wrote:
> Hi Martin,
>
> On 9 January 2017 at 04:14, Jeff Law <law@redhat.com> wrote:
>> On 01/08/2017 02:04 PM, Martin Sebor wrote:
>>>
>>> On 01/06/2017 09:45 AM, Jeff Law wrote:
>>>>
>>>> On 01/05/2017 08:52 PM, Martin Sebor wrote:
>>>>>>>>
>>>>>>>> So Richi asked for removal of the VR_ANTI_RANGE handling, which would
>>>>>>>> imply removal of operand_signed_p.
>>>>>>>>
>>>>>>>> What are the implications if we do that?
>>>>>>>
>>>>>>>
>>>>>>> I just got back to this yesterday.  The implications of the removal
>>>>>>> of the anti-range handling are a number of false negatives in the
>>>>>>> test suite:
>>>>>>
>>>>>> I was thinking more at a higher level.  ie, are the warnings still
>>>>>> useful if we don't have the anti-range handling?  I suspect so, but
>>>>>> would like to hear your opinion.
>>>>>
>>>>> ...
>>>>>>>
>>>>>>>   n = ~[-4, MAX];   (I.e., n is either negative or too big.)
>>>>>>>   p = malloc (n);
>>>>>>
>>>>>> Understood.  The low level question is do we get these kinds of ranges
>>>>>> often enough in computations leading to allocation sizes?
>>>>>
>>>>>
>>>>> My intuition tells me that they are likely common enough not to
>>>>> disregard but I don't have a lot of data to back it up with.  In
>>>>> a Bash build a full 23% of all checked calls are of this kind (24
>>>>> out of 106).  In a Binutils build only 4% are (9 out of 228).  In
>>>>> Glibc, a little under 3%.  My guess is that the number will be
>>>>> inversely proportional to the quality of the code.
>>>>
>>>> So I think you've made a case that we do want to handle this case.  So
>>>> what's left is how best to avoid the infinite recursion and mitigate the
>>>> pathological cases.
>>>>
>>>> What you're computing seems to be "this object may have been derived
>>>> from a signed type".  Right?  It's a property we can compute for any
>>>> given SSA_NAME and it's not context/path specific beyond the
>>>> context/path information encoded by the SSA graph.
>>>>
>>>> So just thinking out load here, could we walk the IL once to identify
>>>> call sites and build a worklist of SSA_NAMEs we care about.  Then we
>>>> iterate on the worklist much like Aldy's code he's working on for the
>>>> unswitching vs uninitialized variable issue?
>>>
>>>
>>> Thanks for the suggestion.  It occurred to me while working on the fix
>>> for 78973 (the non-bug) that size ranges should be handled the same by
>>> -Wstringop-overflow as by -Walloc-size-larger-than, and that both have
>>> the same problem: missing or incomplete support for anti-ranges.  The
>>> attached patch moves get_size_range() from builtins.c to calls.c and
>>> adds better support for anti-ranges.  That solves the problems also
>>> lets it get rid of the objectionable operand_positive_p function.
>>>
>>> Martin
>>>
>>> PS The change to the alloc_max_size function is only needed to make
>>> it possible to specify any argument to the -Walloc-size-larger-than
>>> option, including 0 and -1, so that allocations of any size, including
>>> zero can be flagged.
>>>
>>> gcc-78775.diff
>>>
>>>
>>> PR tree-optimization/78775 - [7 Regression] ICE in
>>> maybe_warn_alloc_args_overflow
>>>
>>> gcc/ChangeLog:
>>>
>>>         PR tree-optimization/78775
>>>         * builtins.c (get_size_range): Move...
>>>         * calls.c: ...to here.
>>>         (alloc_max_size): Accept zero argument.
>>>         (operand_signed_p): Remove.
>>>         (maybe_warn_alloc_args_overflow): Call get_size_range.
>>>         * calls.h (get_size_range): Declare.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>         PR tree-optimization/78775
>>>         * gcc.dg/attr-alloc_size-4.c: Add test cases.
>>>         * gcc.dg/pr78775.c: New test.
>>>         * gcc.dg/pr78973-2.c: New test.
>>>         * gcc.dg/pr78973.c: New test.
>>>
>>
>
> The new test (gcc.dg/pr78973.c) fails on arm targets (there's no warning).
>
> In addition, I have noticed a new failure:
>   gcc.dg/attr-alloc_size-4.c  (test for warnings, line 140)
> on target arm-none-linux-gnueabihf --with-cpu=cortex-a5
> (works fine --with-cpu=cortex-a9)

Thanks.  I'm tracking the test failure on powerpc64* in bug 79051.
It's caused by a VRP bug/limitation described in bug 79054.  Let
me add arm and m68k to the list of targets.

Martin