Message ID | 1377875587-18004-1-git-send-email-meadori@codesourcery.com |
---|---|
State | New |
Headers | show |
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
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