Don't issue array bound warnings on zero-length arrays

Hi All,

This patch fixes a minor issue that can occur when issuing array bounds
warnings.  In GNU C mode we allow empty lists and their upper bound is
initialized to -1.  This confuses the array bound analysis in VRP and
in some cases we end up issuing false positives.  This patch fixes
the issue by bailing out when a zero-length array is encountered.

OK for trunk?

gcc/

2013-08-30  Meador Inge  <meadori@codesourcery.com>

	* tree-vrp.c (check_array_ref): Bail out no emtpy arrays.

gcc/testsuite/

2013-08-30  Meador Inge  <meadori@codesourcery.com>

	* gcc.dg/Warray-bounds-11.c: New testcase.

On 08/30/2013 09:13 AM, Meador Inge wrote:
> Hi All,
>
> This patch fixes a minor issue that can occur when issuing array bounds
> warnings.  In GNU C mode we allow empty lists and their upper bound is
> initialized to -1.  This confuses the array bound analysis in VRP and
> in some cases we end up issuing false positives.  This patch fixes
> the issue by bailing out when a zero-length array is encountered.
>
> OK for trunk?
>
> gcc/
>
> 2013-08-30  Meador Inge  <meadori@codesourcery.com>
>
> 	* tree-vrp.c (check_array_ref): Bail out no emtpy arrays.
>
> gcc/testsuite/
>
> 2013-08-30  Meador Inge  <meadori@codesourcery.com>
>
> 	* gcc.dg/Warray-bounds-11.c: New testcase.
OK for the trunk.  Thanks,
Jeff

On Fri, Aug 30, 2013 at 5:13 PM, Meador Inge <meadori@codesourcery.com> wrote:
> Hi All,
>
> This patch fixes a minor issue that can occur when issuing array bounds
> warnings.  In GNU C mode we allow empty lists and their upper bound is
> initialized to -1.  This confuses the array bound analysis in VRP and
> in some cases we end up issuing false positives.  This patch fixes
> the issue by bailing out when a zero-length array is encountered.
>
> OK for trunk?
>
> gcc/
>
> 2013-08-30  Meador Inge  <meadori@codesourcery.com>
>
>         * tree-vrp.c (check_array_ref): Bail out no emtpy arrays.
>
> gcc/testsuite/
>
> 2013-08-30  Meador Inge  <meadori@codesourcery.com>
>
>         * gcc.dg/Warray-bounds-11.c: New testcase.
>
> Index: gcc/testsuite/gcc.dg/Warray-bounds-11.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/Warray-bounds-11.c     (revision 0)
> +++ gcc/testsuite/gcc.dg/Warray-bounds-11.c     (revision 0)
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Warray-bounds -std=gnu99" } */
> +/* Test zero-length arrays for GNU C.  */
> +
> +unsigned int a[] = { };
> +unsigned int size_a;
> +
> +int test(void)
> +{
> +  /* This should not warn.  */
> +  return size_a ? a[0] : 0;
> +}
> Index: gcc/tree-vrp.c
> ===================================================================
> --- gcc/tree-vrp.c      (revision 202088)
> +++ gcc/tree-vrp.c      (working copy)
> @@ -6137,9 +6137,10 @@ check_array_ref (location_t location, tr
>    low_sub = up_sub = TREE_OPERAND (ref, 1);
>    up_bound = array_ref_up_bound (ref);
>
> -  /* Can not check flexible arrays.  */
> +  /* Can not check flexible arrays or zero-length arrays.  */
>    if (!up_bound
> -      || TREE_CODE (up_bound) != INTEGER_CST)
> +      || TREE_CODE (up_bound) != INTEGER_CST
> +      || tree_int_cst_equal (up_bound, integer_minus_one_node))

That doesn't look correct - what if the lower bound is -10?  That can
easily happen
for Ada, so please revert the patch.  And I fail to see why the testcase should
not warn.  Clearly you have a definition of a here and it doesn't have
an element
so the access is out of bounds.

Richard.

>      return;
>
>    /* Accesses to trailing arrays via pointers may access storage

On 09/02/2013 04:27 AM, Richard Biener wrote:
> On Fri, Aug 30, 2013 at 5:13 PM, Meador Inge <meadori@codesourcery.com> wrote:
>> Hi All,
>>
>> This patch fixes a minor issue that can occur when issuing array bounds
>> warnings.  In GNU C mode we allow empty lists and their upper bound is
>> initialized to -1.  This confuses the array bound analysis in VRP and
>> in some cases we end up issuing false positives.  This patch fixes
>> the issue by bailing out when a zero-length array is encountered.
>>
>> OK for trunk?
>>
>> gcc/
>>
>> 2013-08-30  Meador Inge  <meadori@codesourcery.com>
>>
>>         * tree-vrp.c (check_array_ref): Bail out no emtpy arrays.
>>
>> gcc/testsuite/
>>
>> 2013-08-30  Meador Inge  <meadori@codesourcery.com>
>>
>>         * gcc.dg/Warray-bounds-11.c: New testcase.
>>
>> Index: gcc/testsuite/gcc.dg/Warray-bounds-11.c
>> ===================================================================
>> --- gcc/testsuite/gcc.dg/Warray-bounds-11.c     (revision 0)
>> +++ gcc/testsuite/gcc.dg/Warray-bounds-11.c     (revision 0)
>> @@ -0,0 +1,12 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -Warray-bounds -std=gnu99" } */
>> +/* Test zero-length arrays for GNU C.  */
>> +
>> +unsigned int a[] = { };
>> +unsigned int size_a;
>> +
>> +int test(void)
>> +{
>> +  /* This should not warn.  */
>> +  return size_a ? a[0] : 0;
>> +}
>> Index: gcc/tree-vrp.c
>> ===================================================================
>> --- gcc/tree-vrp.c      (revision 202088)
>> +++ gcc/tree-vrp.c      (working copy)
>> @@ -6137,9 +6137,10 @@ check_array_ref (location_t location, tr
>>    low_sub = up_sub = TREE_OPERAND (ref, 1);
>>    up_bound = array_ref_up_bound (ref);
>>
>> -  /* Can not check flexible arrays.  */
>> +  /* Can not check flexible arrays or zero-length arrays.  */
>>    if (!up_bound
>> -      || TREE_CODE (up_bound) != INTEGER_CST)
>> +      || TREE_CODE (up_bound) != INTEGER_CST
>> +      || tree_int_cst_equal (up_bound, integer_minus_one_node))
> 
> That doesn't look correct - what if the lower bound is -10?  That can
> easily happen
> for Ada, so please revert the patch.

OK.

> And I fail to see why the testcase should
> not warn.  Clearly you have a definition of a here and it doesn't have
> an element
> so the access is out of bounds.

Not always, 'size_a' can be zero and the warning is worded such that the out of
bounds access always happens.  In fact, changing the code to 'size_a = 0' still
issues a warning.

On Tue, Sep 03, 2013 at 10:40:16AM -0500, Meador Inge wrote:
> > And I fail to see why the testcase should
> > not warn.  Clearly you have a definition of a here and it doesn't have
> > an element
> > so the access is out of bounds.
> 
> Not always, 'size_a' can be zero and the warning is worded such that the out of
> bounds access always happens.  In fact, changing the code to 'size_a = 0' still
> issues a warning.

How would that be different if you had that invalid access in a function
and never called the function, or called it only if (0) or similar?
We don't do reachability analysis, if any code we warn about can be
reachable from main, and still warn about invalid code, this is invalid
code, so it is IMHO just fine to warn about it.

	Jakub

On 09/03/2013 10:45 AM, Jakub Jelinek wrote:

> On Tue, Sep 03, 2013 at 10:40:16AM -0500, Meador Inge wrote:
>>> And I fail to see why the testcase should
>>> not warn.  Clearly you have a definition of a here and it doesn't have
>>> an element
>>> so the access is out of bounds.
>>
>> Not always, 'size_a' can be zero and the warning is worded such that the out of
>> bounds access always happens.  In fact, changing the code to 'size_a = 0' still
>> issues a warning.
> 
> How would that be different if you had that invalid access in a function
> and never called the function, or called it only if (0) or similar?
> We don't do reachability analysis, if any code we warn about can be
> reachable from main, and still warn about invalid code, this is invalid
> code, so it is IMHO just fine to warn about it.

True.  Perhaps I am getting too caught up in the wording.  I thought we
typically use "may" for warnings that aren't definitive, but in this case
we use "is" (instead of something like "may be"):

   warning: array subscript is above array bounds [-Warray-bounds]

On Tue, Sep 03, 2013 at 11:01:17AM -0500, Meador Inge wrote:
> On 09/03/2013 10:45 AM, Jakub Jelinek wrote:
> 
> > On Tue, Sep 03, 2013 at 10:40:16AM -0500, Meador Inge wrote:
> >>> And I fail to see why the testcase should
> >>> not warn.  Clearly you have a definition of a here and it doesn't have
> >>> an element
> >>> so the access is out of bounds.
> >>
> >> Not always, 'size_a' can be zero and the warning is worded such that the out of
> >> bounds access always happens.  In fact, changing the code to 'size_a = 0' still
> >> issues a warning.
> > 
> > How would that be different if you had that invalid access in a function
> > and never called the function, or called it only if (0) or similar?
> > We don't do reachability analysis, if any code we warn about can be
> > reachable from main, and still warn about invalid code, this is invalid
> > code, so it is IMHO just fine to warn about it.
> 
> True.  Perhaps I am getting too caught up in the wording.  I thought we
> typically use "may" for warnings that aren't definitive, but in this case
> we use "is" (instead of something like "may be"):
> 
>    warning: array subscript is above array bounds [-Warray-bounds]

That is only the case for uninit warnings which makes this distinction, no
other warning afaik does something similar.
And, even for uninit, even if you get is used uninitialized warning, it
still may be on dead code, the difference is that if you hit the statement
on which it warns, when it is the "is" variant, it means that you use
there an uninitialized value, while the "may" variant means you can hit it
both with uninitialized or initialized value.  If you hit the a[0]
statement, you always invoke undefined behavior, so it shouldn't be a "may".

	Jakub