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

Submitted by Meador Inge on Aug. 30, 2013, 3:13 p.m.

Details

Message ID 1377875587-18004-1-git-send-email-meadori@codesourcery.com
State New
Headers show

Commit Message

Meador Inge Aug. 30, 2013, 3:13 p.m.
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.

Comments

Jeff Law Aug. 30, 2013, 4:02 p.m.
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
Richard Guenther Sept. 2, 2013, 9:27 a.m.
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
Meador Inge Sept. 3, 2013, 3:40 p.m.
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.
Jakub Jelinek Sept. 3, 2013, 3:45 p.m.
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
Meador Inge Sept. 3, 2013, 4:01 p.m.
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]
Jakub Jelinek Sept. 3, 2013, 4:12 p.m.
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

Patch hide | download patch | download mbox

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))
     return;
 
   /* Accesses to trailing arrays via pointers may access storage