[PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Simon Baldwin]

Attached below is a modest patch to provide a subset of the -Warray-bounds
warnings from tree-vrp.c in the C and C++ front ends.  This permits the
compiler to warn about egregious array bounds violations in unoptimized
compilations or compilations that may use -fno-tree-vrp.  At present, array
bounds checking is only done on optimized compilations.

A side effect of copying these warnings up into the language frontends is
that warnings are now printed even if the array access is in dead or
inaccessible code.

The current array bounds tests are modified to account for this new checking,
and additionally there are two new tests for warnings from -O0 compilations,
one for C and one for C++.

Bootstrapped, and regression tested on i686 Linux for gcc and g++.

Thoughts?  Okay for trunk?


:ADDPATCH diagnostic:

gcc/ChangeLog
2008-04-03  Simon Baldwin <simonb@google.com>

	* c-typeck.c (build_array_ref): Added array bounds checking for
	bounded arrays indexed by constants.

gcc/cp/ChangeLog
2008-04-03  Simon Baldwin <simonb@google.com>

	* typeck.c (build_array_ref): Added array bounds checking for
	bounded arrays indexed by constants.

gcc/testsuite/ChangeLog
2008-04-03  Simon Baldwin <simonb@google.com>
  
	* testsuite/gcc.dg/Warray-bounds.c: Updated for frontend warnings.
	* testsuite/g++.dg/warn/Warray-bounds.c: Updated for frontend warnings.
	* testsuite/gcc.dg/Warray-bounds-noopt.c: New testcase.
	* testsuite/g++.dg/warn/Warray-bounds-noopt.c: New testcase.


Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 133772)
+++ gcc/doc/invoke.texi	(working copy)
@@ -2658,7 +2658,7 @@
 @option{-Wall} turns on the following warning flags:
 
 @gccoptlist{-Waddress   @gol
--Warray-bounds @r{(only with} @option{-O2}@r{)}  @gol
+-Warray-bounds @r{(some checks, but more complete with} @option{-O2}@r{)}  @gol
 -Wc++0x-compat  @gol
 -Wchar-subscripts  @gol
 -Wimplicit-int  @gol
@@ -3361,7 +3361,8 @@
 @item -Warray-bounds
 @opindex Wno-array-bounds
 @opindex Warray-bounds
-This option is only active when @option{-ftree-vrp} is active
+This option performs a subset of checks in unoptimized compilations, and
+stricter checking when @option{-ftree-vrp} is active
 (default for -O2 and above). It warns about subscripts to arrays
 that are always out of bounds. This warning is enabled by @option{-Wall}.
 
Index: gcc/testsuite/gcc.dg/Warray-bounds.c
===================================================================
--- gcc/testsuite/gcc.dg/Warray-bounds.c	(revision 133772)
+++ gcc/testsuite/gcc.dg/Warray-bounds.c	(working copy)
@@ -56,13 +56,13 @@
     g(&a[8]);
     g(&a[9]);
     g(&a[10]);
-    g(&a[11]);             /* { dg-warning "array subscript" "" { xfail *-*-* } } */
-    g(&a[-30]+10);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+30);
+    g(&a[11]);             /* { dg-warning "array subscript" } */
+    g(&a[-30]+10);         /* { dg-warning "array subscript" } */
+    g(&a[-30]+30);         /* { dg-warning "array subscript" } */
 
     g(&b[10]);
     g(&c.c[10]);
-    g(&b[11]);             /* { dg-warning "array subscript" "" { xfail *-*-* } } */
+    g(&b[11]);             /* { dg-warning "array subscript" } */
     g(&c.c[11]);           /* { dg-warning "array subscript" } */
 
     g(&a[0]);
@@ -71,11 +71,11 @@
 
     g(&a[-1]);             /* { dg-warning "array subscript" } */
     g(&b[-1]);             /* { dg-warning "array subscript" } */ 
-    h(sizeof a[-1]);
+    h(sizeof a[-1]);       /* { dg-warning "array subscript" } */
     h(sizeof a[10]);
-    h(sizeof b[-1]);
+    h(sizeof b[-1]);       /* { dg-warning "array subscript" } */
     h(sizeof b[10]);
-    h(sizeof c.c[-1]);
+    h(sizeof c.c[-1]);     /* { dg-warning "array subscript" } */
     h(sizeof c.c[10]);
 
     if (10 < 10)
@@ -83,11 +83,10 @@
     if (10 < 10)
        b[10] = 0;
     if (-1 >= 0)
-       c.c[-1] = 0;
+       c.c[-1] = 0;        /* { dg-warning "array subscript" } */
 
     for (i = 20; i < 30; ++i)
              a[i] = 1;       /* { dg-warning "array subscript" } */
 
     return a;
 }
-
Index: gcc/testsuite/g++.dg/warn/Warray-bounds.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Warray-bounds.C	(revision 133772)
+++ gcc/testsuite/g++.dg/warn/Warray-bounds.C	(working copy)
@@ -57,12 +57,11 @@
     g(&a[9]);
     g(&a[10]);
     g(&a[11]);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+10);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+30);
+    g(&a[-30]+10);         /* { dg-warning "array subscript" } */
+    g(&a[-30]+30);         /* { dg-warning "array subscript" } */
 
     g(&b[10]);
     g(&c.c[10]);
-    g(&a[11]);             /* { dg-warning "array subscript" } */
     g(&b[11]);             /* { dg-warning "array subscript" } */
     g(&c.c[11]);           /* { dg-warning "array subscript" } */
 
@@ -72,11 +71,11 @@
 
     g(&a[-1]);             /* { dg-warning "array subscript" } */
     g(&b[-1]);             /* { dg-warning "array subscript" } */ 
-    h(sizeof a[-1]);
+    h(sizeof a[-1]);       /* { dg-warning "array subscript" } */
     h(sizeof a[10]);
-    h(sizeof b[-1]);
+    h(sizeof b[-1]);       /* { dg-warning "array subscript" } */
     h(sizeof b[10]);
-    h(sizeof c.c[-1]);
+    h(sizeof c.c[-1]);     /* { dg-warning "array subscript" } */
     h(sizeof c.c[10]);
 
     if (10 < 10)
@@ -84,8 +83,10 @@
     if (10 < 10)
        b[10] = 0;
     if (-1 >= 0)
-       c.c[-1] = 0;
+       c.c[-1] = 0;        /* { dg-warning "array subscript" } */
+
+    for (i = 20; i < 30; ++i)
+             a[i] = 1;       /* { dg-warning "array subscript" } */
 
     return a;
 }
-
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 133772)
+++ gcc/cp/typeck.c	(working copy)
@@ -2554,7 +2554,8 @@
 
   if (TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE)
     {
-      tree rval, type;
+      bool has_warned_on_bounds_check = false;
+      tree rval, type, ref;
 
       warn_array_subscript_with_type_char (idx);
 
@@ -2571,6 +2572,48 @@
 	 pointer arithmetic.)  */
       idx = perform_integral_promotions (idx);
 
+      /* Warn about obvious array bounds errors for fixed size arrays that
+         are indexed by a constant.  This is a subset of similar checks in
+         tree-vrp.c; by doing this here we can get some level of checking
+         from non-optimized, non-vrp compilation.  */
+      if (TREE_CODE (idx) == INTEGER_CST && TYPE_DOMAIN (TREE_TYPE (array)))
+        {
+          const_tree max_index;
+
+          max_index = TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (array)));
+          if (max_index && TREE_CODE (max_index) == INTEGER_CST
+              && tree_int_cst_lt (max_index, idx)
+              && !tree_int_cst_equal (idx, max_index)
+              /* Always allow off-by-one.  */
+              && !tree_int_cst_equal (int_const_binop (PLUS_EXPR,
+                                                       max_index,
+                                                       integer_one_node,
+                                                       0),
+                                      idx)
+              /* Accesses after the end of arrays of size 0 (gcc
+                 extension) and 1 are likely intentional ("struct
+                 hack").  */
+              && compare_tree_int (max_index, 1) > 0)
+            {
+              warning (OPT_Warray_bounds,
+                       "array subscript is above array bounds");
+              has_warned_on_bounds_check = true;
+            }
+          else
+            {
+              const_tree min_index;
+
+              min_index = TYPE_MIN_VALUE (TYPE_DOMAIN (TREE_TYPE (array)));
+              if (min_index && TREE_CODE (min_index) == INTEGER_CST
+                  && tree_int_cst_lt (idx, min_index))
+                {
+                  warning (OPT_Warray_bounds,
+                           "array subscript is below array bounds");
+                  has_warned_on_bounds_check = true;
+                }
+            }
+        }
+
       /* An array that is indexed by a non-constant
 	 cannot be stored in a register; we must be able to do
 	 address arithmetic on its address.
@@ -2621,7 +2664,12 @@
 	|= (CP_TYPE_VOLATILE_P (type) | TREE_SIDE_EFFECTS (array));
       TREE_THIS_VOLATILE (rval)
 	|= (CP_TYPE_VOLATILE_P (type) | TREE_THIS_VOLATILE (array));
-      return require_complete_type (fold_if_not_in_template (rval));
+      ref = require_complete_type (fold_if_not_in_template (rval));
+
+      /* Suppress bounds warning in tree-vrp.c if already warned here.  */
+      if (has_warned_on_bounds_check)
+        TREE_NO_WARNING (ref) = 1;
+      return ref;
     }
 
   {
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 133772)
+++ gcc/c-typeck.c	(working copy)
@@ -2086,7 +2086,50 @@
 
   if (TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE)
     {
-      tree rval, type;
+      bool has_warned_on_bounds_check = false;
+      tree rval, type, ref;
+
+      /* Warn about obvious array bounds errors for fixed size arrays that
+         are indexed by a constant.  This is a subset of similar checks in
+         tree-vrp.c; by doing this here we can get some level of checking
+         from non-optimized, non-vrp compilation.  */
+      if (TREE_CODE (index) == INTEGER_CST && TYPE_DOMAIN (TREE_TYPE (array)))
+        {
+          const_tree max_index;
+
+          max_index = TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (array)));
+          if (max_index && TREE_CODE (max_index) == INTEGER_CST
+              && tree_int_cst_lt (max_index, index)
+              && !tree_int_cst_equal (index, max_index)
+              /* Always allow off-by-one.  */
+              && !tree_int_cst_equal (int_const_binop (PLUS_EXPR,
+                                                       max_index,
+                                                       integer_one_node,
+                                                       0),
+                                      index)
+              /* Accesses after the end of arrays of size 0 (gcc
+                 extension) and 1 are likely intentional ("struct
+                 hack").  */
+              && compare_tree_int (max_index, 1) > 0)
+            {
+              warning (OPT_Warray_bounds,
+                       "array subscript is above array bounds");
+              has_warned_on_bounds_check = true;
+            }
+          else
+            {
+              const_tree min_index;
+
+              min_index = TYPE_MIN_VALUE (TYPE_DOMAIN (TREE_TYPE (array)));
+              if (min_index && TREE_CODE (min_index) == INTEGER_CST
+                  && tree_int_cst_lt (index, min_index))
+                {
+                  warning (OPT_Warray_bounds,
+                           "array subscript is below array bounds");
+                  has_warned_on_bounds_check = true;
+                }
+            }
+        }
 
       /* An array that is indexed by a non-constant
 	 cannot be stored in a register; we must be able to do
@@ -2139,7 +2182,12 @@
 	       in an inline function.
 	       Hope it doesn't break something else.  */
 	    | TREE_THIS_VOLATILE (array));
-      return require_complete_type (fold (rval));
+      ref = require_complete_type (fold (rval));
+
+      /* Suppress bounds warning in tree-vrp.c if already warned here.  */
+      if (has_warned_on_bounds_check)
+        TREE_NO_WARNING (ref) = 1;
+      return ref;
     }
   else
     {

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Richard Guenther]

On Fri, Apr 4, 2008 at 1:07 AM, Simon Baldwin <simonb@google.com> wrote:
> Attached below is a modest patch to provide a subset of the -Warray-bounds
>  warnings from tree-vrp.c in the C and C++ front ends.  This permits the
>  compiler to warn about egregious array bounds violations in unoptimized
>  compilations or compilations that may use -fno-tree-vrp.  At present, array
>  bounds checking is only done on optimized compilations.
>
>  A side effect of copying these warnings up into the language frontends is
>  that warnings are now printed even if the array access is in dead or
>  inaccessible code.
>
>  The current array bounds tests are modified to account for this new checking,
>  and additionally there are two new tests for warnings from -O0 compilations,
>  one for C and one for C++.
>
>  Bootstrapped, and regression tested on i686 Linux for gcc and g++.
>
>  Thoughts?  Okay for trunk?

I think it is a good thing to move these to the frontends.  Did you test
the warning on some real-world C and C++ code?  I expect some
"false" positives from deliberately partial dead code coming from
template instantiation or constant propagation from function arguments.

Thanks,
Richard.

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Simon Baldwin]

Richard Guenther wrote:
> On Fri, Apr 4, 2008 at 1:07 AM, Simon Baldwin <simonb@google.com> wrote:
>   
>> Attached below is a modest patch to provide a subset of the -Warray-bounds
>>  warnings from tree-vrp.c in the C and C++ front ends.  This permits the
>>  compiler to warn about egregious array bounds violations in unoptimized
>>  compilations or compilations that may use -fno-tree-vrp.  At present, array
>>  bounds checking is only done on optimized compilations.
>>
>>  A side effect of copying these warnings up into the language frontends is
>>  that warnings are now printed even if the array access is in dead or
>>  inaccessible code.
>>
>>  The current array bounds tests are modified to account for this new checking,
>>  and additionally there are two new tests for warnings from -O0 compilations,
>>  one for C and one for C++.
>>
>>  Bootstrapped, and regression tested on i686 Linux for gcc and g++.
>>
>>  Thoughts?  Okay for trunk?
>>     
>
> I think it is a good thing to move these to the frontends.  Did you test
> the warning on some real-world C and C++ code?  I expect some
> "false" positives from deliberately partial dead code coming from
> template instantiation or constant propagation from function arguments.

I tried it on a codebase consisting of more than 3,000 source modules, 
almost all C++.  Encouragingly, it produced no false positives, and only 
one true positive, the latter from a "plant" I'd deliberately placed 
into the code ahead of time just to be sure I'd set up the compilation 
correctly.

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Andrew Pinski]

On Fri, Apr 4, 2008 at 4:42 PM, Simon Baldwin <simonb@google.com> wrote:
>  I tried it on a codebase consisting of more than 3,000 source modules,
> almost all C++.  Encouragingly, it produced no false positives, and only one
> true positive, the latter from a "plant" I'd deliberately placed into the
> code ahead of time just to be sure I'd set up the compilation correctly.

Here is one case where this might break:

#define strlen(a) ({  int len = 0; if(__builtin_constant_p(a)) {if
(!a[0]) len=0; else if (!a[1]) len = 1; else if (!a[2]) len = 2; else
len = realstrlen(a); } else len = realstrlen (a); len;})

This is just a semi fake example (glibc does something like this for
one of the str* functions, I forget which one) where define comes up
with out of bounds array references in dead code.

Thanks,
Andrew Pinski

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Simon Baldwin]

Andrew Pinski wrote:
> On Fri, Apr 4, 2008 at 4:42 PM, Simon Baldwin <simonb@google.com> wrote:
>   
>>  I tried it on a codebase consisting of more than 3,000 source modules,
>> almost all C++.  Encouragingly, it produced no false positives, and only one
>> true positive, the latter from a "plant" I'd deliberately placed into the
>> code ahead of time just to be sure I'd set up the compilation correctly.
>>     
>
> Here is one case where this might break:
>
> #define strlen(a) ({  int len = 0; if(__builtin_constant_p(a)) {if
> (!a[0]) len=0; else if (!a[1]) len = 1; else if (!a[2]) len = 2; else
> len = realstrlen(a); } else len = realstrlen (a); len;})
>
> This is just a semi fake example (glibc does something like this for
> one of the str* functions, I forget which one) where define comes up
> with out of bounds array references in dead code.

Thanks for the note.

The semi-fake example above doesn't trigger the front-end warning added 
by the patch.  Warnings for subscripts above the array bounds are 
suppressed for arrays defined with zero or one element, and this warning 
also always allows over-by-one, so that accessing 'a[2]' for an array 
defined with two elements gives no warning (but also see * below).  As a 
further test, I compiled glibc 2.4 with the patched compiler, and no 
-Warray-bounds warnings triggered.

Can you recall or locate the glibc code that you think might have 
problems here?  I took a look through it and couldn't readily locate 
anything that looked dodgy.  There's bits/string2.h, of course, but 
since it's a system header it should be immune, I think.


*) One extra finding from further testing.  I believe that the check in 
the patch that reads

+              /* Accesses after the end of arrays of size 0 (gcc
+                 extension) and 1 are likely intentional ("struct
+                 hack").  */
+              && compare_tree_int (max_index, 1) > 0)

is off-by-one, in that it disagrees with the comment.  max_index here is 
the largest valid subscript, not the array dimension.  For 'int a[2]', 
max_index is 1, so with this test, the warning is suppressed for arrays 
dimensioned 2 or less, not 1 or less as noted in the comment.

I copied (and inverted) this out of check_array_ref() in tree-vrp.c, for 
complete consistency.  It uses

4541       /* Accesses after the end of arrays of size 0 (gcc
4542          extension) and 1 are likely intentional ("struct
4543          hack").  */
4544       || compare_tree_int (up_bound, 1) <= 0)

and this looks to be off-by-one or in disagreement with the comment in 
the same way, borne out by testing current prerelase gcc 4.3.1 
(unfortunately I don't have a top-of-tree unpatched 4.4 build readily to 
hand):

  1 int func(void) {
  2   const char a[0], b[1], c[2], d[3], e[4];
  3   int x = 0;
  4
  5   x += a[1];  // warning should be suppressed, and is
  6   x += b[2];  // warning should be suppressed, and is
  7   x += c[3];  // should warn, but doesn't
  8   x += d[4];  // should warn, and does
  9   x += e[5];  // should warn, and does
 10
 11   return x;
 12 }

 $ gcc -Wall -W -Warray-bounds -O2 -c a.c
 a.c: In function 'func':
 a.c:8: warning: array subscript is above array bounds
 a.c:9: warning: array subscript is above array bounds

If my reading of the code is correct here, the probable conservative 
course here is to amend the comments to reflect the code, rather than 
the other way round, noting that arrays sized two elements or less have 
this check suppressed.  And maybe add an explicit test case for 
clarity.  The riskier course is to enable the checks for arrays 
dimensioned two and above, and see what happens.  Any preference?

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Richard Guenther]

On Mon, Apr 7, 2008 at 11:40 PM, Simon Baldwin <simonb@google.com> wrote:
>
> Andrew Pinski wrote:
>
> > On Fri, Apr 4, 2008 at 4:42 PM, Simon Baldwin <simonb@google.com> wrote:
> >
> >
> > >  I tried it on a codebase consisting of more than 3,000 source modules,
> > > almost all C++.  Encouragingly, it produced no false positives, and only
> one
> > > true positive, the latter from a "plant" I'd deliberately placed into
> the
> > > code ahead of time just to be sure I'd set up the compilation correctly.
> > >
> > >
> >
> > Here is one case where this might break:
> >
> > #define strlen(a) ({  int len = 0; if(__builtin_constant_p(a)) {if
> > (!a[0]) len=0; else if (!a[1]) len = 1; else if (!a[2]) len = 2; else
> > len = realstrlen(a); } else len = realstrlen (a); len;})
> >
> > This is just a semi fake example (glibc does something like this for
> > one of the str* functions, I forget which one) where define comes up
> > with out of bounds array references in dead code.
> >
>
>  Thanks for the note.
>
>  The semi-fake example above doesn't trigger the front-end warning added by
> the patch.  Warnings for subscripts above the array bounds are suppressed
> for arrays defined with zero or one element, and this warning also always
> allows over-by-one, so that accessing 'a[2]' for an array defined with two
> elements gives no warning (but also see * below).  As a further test, I
> compiled glibc 2.4 with the patched compiler, and no -Warray-bounds warnings
> triggered.
>
>  Can you recall or locate the glibc code that you think might have problems
> here?  I took a look through it and couldn't readily locate anything that
> looked dodgy.  There's bits/string2.h, of course, but since it's a system
> header it should be immune, I think.
>
>
>  *) One extra finding from further testing.  I believe that the check in the
> patch that reads
>
>
>  +              /* Accesses after the end of arrays of size 0 (gcc
>  +                 extension) and 1 are likely intentional ("struct
>  +                 hack").  */
>  +              && compare_tree_int (max_index, 1) > 0)
>
>  is off-by-one, in that it disagrees with the comment.  max_index here is
> the largest valid subscript, not the array dimension.  For 'int a[2]',
> max_index is 1, so with this test, the warning is suppressed for arrays
> dimensioned 2 or less, not 1 or less as noted in the comment.
>
>  I copied (and inverted) this out of check_array_ref() in tree-vrp.c, for
> complete consistency.  It uses
>
>  4541       /* Accesses after the end of arrays of size 0 (gcc
>  4542          extension) and 1 are likely intentional ("struct
>  4543          hack").  */
>  4544       || compare_tree_int (up_bound, 1) <= 0)
>
>  and this looks to be off-by-one or in disagreement with the comment in the
> same way, borne out by testing current prerelase gcc 4.3.1 (unfortunately I
> don't have a top-of-tree unpatched 4.4 build readily to hand):
>
>   1 int func(void) {
>   2   const char a[0], b[1], c[2], d[3], e[4];
>   3   int x = 0;
>   4
>   5   x += a[1];  // warning should be suppressed, and is
>   6   x += b[2];  // warning should be suppressed, and is
>   7   x += c[3];  // should warn, but doesn't
>   8   x += d[4];  // should warn, and does
>   9   x += e[5];  // should warn, and does
>  10
>  11   return x;
>  12 }
>
>  $ gcc -Wall -W -Warray-bounds -O2 -c a.c
>  a.c: In function 'func':
>  a.c:8: warning: array subscript is above array bounds
>  a.c:9: warning: array subscript is above array bounds
>
>  If my reading of the code is correct here, the probable conservative course
> here is to amend the comments to reflect the code, rather than the other way
> round, noting that arrays sized two elements or less have this check
> suppressed.  And maybe add an explicit test case for clarity.  The riskier
> course is to enable the checks for arrays dimensioned two and above, and see
> what happens.  Any preference?

Feel free to fix the off-by-one errors.

Thanks,
Richard.

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Dirk Mueller]

On Tuesday 08 April 2008, Simon Baldwin wrote:

> If my reading of the code is correct here, the probable conservative
> course here is to amend the comments to reflect the code, rather than
> the other way round, noting that arrays sized two elements or less have
> this check suppressed.  And maybe add an explicit test case for
> clarity.  The riskier course is to enable the checks for arrays
> dimensioned two and above, and see what happens.  Any preference?

The code should be adjusted to match the comment, I believe. 

Greetings,
Dirk

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Dirk Mueller]

On Saturday 05 April 2008, Andrew Pinski wrote:

> This is just a semi fake example (glibc does something like this for
> one of the str* functions, I forget which one) where define comes up
> with out of bounds array references in dead code.

strpbrk usually triggers that. 

Greetings,
Dirk

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Andrew Pinski]

On Fri, Apr 4, 2008 at 4:50 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>  This is just a semi fake example (glibc does something like this for
>  one of the str* functions, I forget which one) where define comes up
>  with out of bounds array references in dead code.

I was reported an example of where this happens (even currently but
that is a different issue):
From #gcc:

[10:44:57] < ryanarn> So is this actually bad form in C? strcmp (arg,
"no");?  GCC 4.3 warns by default now.  It should be:char no[]=
"no\0"; strcmp(arg,no);?
[10:45:33] < apinski> what is the warning about?
[10:46:01] < ryanarn> warning: array subscript is above array bounds
[10:46:09] < apinski> false warnings
[10:46:13] < apinski> due to glibc
[10:46:34] < apinski> this is why I mentioned about why we should not
move these warnings to the front-end
[10:46:55] < ryanarn> apinski: well this is in glibc's libm-test.c

So if you want to test for the warning, please try running make check
with glibc also and not just building it.  Also try compiling glibc
2.6.x instead of 2.4.x or better yet the trunk of glibc where I think
Ryan found these warnings.

Thanks,
Andrew Pinski

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Mark Mitchell]

Richard Guenther wrote:

>>  Thoughts?  Okay for trunk?
> 
> I think it is a good thing to move these to the frontends. 

Me too!  The code all looks good.  However, I do think this should be 
factored into a function in c_common.

Also:

+              if (min_index && TREE_CODE (min_index) == INTEGER_CST
+                  && tree_int_cst_lt (idx, min_index))
+                {
+                  warning (OPT_Warray_bounds,
+                           "array subscript is below array bounds");

could do with a better error message.  In C/C++, the min_index is (I 
believe) always zero.  I think you're right to have written the test in 
terms of min_index (future-proofing!), but I think we should special 
case the message.  If the idx is negative and the min_index is zero, say:

   "array subscript is negative"

That's more obvious to the average user.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Simon Baldwin]

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

Thanks for the feedback.

I've incorporated most comments, and a revised patch is attached below.  
One thing I wasn't able to fathom was how to suppress warnings about 
"sizeof(a[-1])" in the C and C++ frontends; if anyone's got any hints on 
this, I'd love to hear them.

In testing, the new version passes bootstrap and gcc tests, and also the 
3k C++ source files real world test.

When compiling libc 2.4 with it, two new warnings arise, in optimized 
(tree-vrp) builds only, so from tree-vrp.c:check_array_ref():

  digits_dots.c: In function '__nss_hostname_digits_dots':
  digits_dots.c:159: warning: array subscript is above array bounds
  digits_dots.c:291: warning: array subscript is above array bounds

Abstracting out the problematic parts of this gives:

  void func(void) {
     typedef unsigned char host_addr_t[16];
     host_addr_t *host_addr;
     typedef char *host_addr_list_t[2];
     host_addr_list_t *h_addr_ptrs;
     char **h_alias_ptr;

     host_addr = (host_addr_t *) 0;
     h_addr_ptrs = (host_addr_list_t *) ((char *) host_addr + sizeof 
(*host_addr));
     h_alias_ptr = (char **) ((char *) h_addr_ptrs + sizeof (*h_addr_ptrs));
     h_alias_ptr[0] = ((void *)0);  // Warning here
  }

This doesn't show up with un-patched gcc because h_alias_ptr appears in 
tree-vrp as having dimension 2, and prior to this patch 
check_array_ref() let a[2], as well as a[1] and a[0], slide by 
unchecked.  I'm wondering if check_array_ref() doesn't actually have a 
bona-fide complaint here.


[-- Attachment #2: bounds.patch --]
[-- Type: text/x-patch, Size: 13998 bytes --]

This is a modest patch to provide a subset of the -Warray-bounds warnings
from tree-vrp.c in the C and C++ front ends.  This permits the compiler to
warn about egregious array bounds violations in unoptimized compilations or
compilations that may use -fno-tree-vrp.  At present, array bounds checking
is only done on optimized compilations.

A side effect of copying these warnings up into the language frontends is
that warnings are now printed even if the array access is in dead or
inaccessible code.

The current array bounds tests are modified to account for this new checking,
and additionally there are two new tests for warnings from -O0 compilations,
one for C and one for C++.

Bootstrapped, and regression tested on i686 Linux for gcc and g++.


:ADDPATCH diagnostic:

gcc/ChangeLog
2008-04-09  Simon Baldwin <simonb@google.com>

	* c-common.h (warn_array_subscript_range): New function.
	* c-common.c (warn_array_subscript_range): Ditto.
	* tree-vrp.c (check_array_ref): Corrected code to agree with
	comment, ignoring only arrays of size 0 or size 1.
	* c-typeck.c (build_array_ref): Call warn_array_subscript_range.

gcc/cp/ChangeLog
2008-04-09  Simon Baldwin <simonb@google.com>

	* typeck.c (build_array_ref): Call warn_array_subscript_range.

gcc/testsuite/ChangeLog
2008-04-09  Simon Baldwin <simonb@google.com>
  
	* testsuite/gcc.dg/Warray-bounds.c: Updated for frontend warnings,
	additional tests for arrays of size 0 and size 1.
	* testsuite/g++.dg/warn/Warray-bounds.c: Ditto.
	* testsuite/gcc.dg/Warray-bounds-noopt.c: New testcase.
	* testsuite/g++.dg/warn/Warray-bounds-noopt.c: Ditto.


Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c	(revision 133772)
+++ gcc/tree-vrp.c	(working copy)
@@ -4540,8 +4540,8 @@
           && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (ref))) == NULL_TREE)
       /* Accesses after the end of arrays of size 0 (gcc
          extension) and 1 are likely intentional ("struct
-         hack").  */
-      || compare_tree_int (up_bound, 1) <= 0)
+         hack").  Note that up_bound is array dimension - 1.  */
+      || compare_tree_int (up_bound, 1) < 0)
     return;
 
   low_bound = array_ref_low_bound (ref);
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 133772)
+++ gcc/doc/invoke.texi	(working copy)
@@ -2658,7 +2658,7 @@
 @option{-Wall} turns on the following warning flags:
 
 @gccoptlist{-Waddress   @gol
--Warray-bounds @r{(only with} @option{-O2}@r{)}  @gol
+-Warray-bounds @r{(some checks, but more complete with} @option{-O2}@r{)}  @gol
 -Wc++0x-compat  @gol
 -Wchar-subscripts  @gol
 -Wimplicit-int  @gol
@@ -3361,7 +3361,8 @@
 @item -Warray-bounds
 @opindex Wno-array-bounds
 @opindex Warray-bounds
-This option is only active when @option{-ftree-vrp} is active
+This option performs a subset of checks in unoptimized compilations, and
+stricter checking when @option{-ftree-vrp} is active
 (default for -O2 and above). It warns about subscripts to arrays
 that are always out of bounds. This warning is enabled by @option{-Wall}.
 
Index: gcc/testsuite/gcc.dg/Warray-bounds.c
===================================================================
--- gcc/testsuite/gcc.dg/Warray-bounds.c	(revision 133772)
+++ gcc/testsuite/gcc.dg/Warray-bounds.c	(working copy)
@@ -17,6 +17,7 @@
     struct {
        int c[10];
     } c;
+    int p[0], q[1], r[2], s[3], t[4];
 
     a[-1] = 0;             /* { dg-warning "array subscript" } */
     a[ 0] = 0;
@@ -56,13 +57,13 @@
     g(&a[8]);
     g(&a[9]);
     g(&a[10]);
-    g(&a[11]);             /* { dg-warning "array subscript" "" { xfail *-*-* } } */
-    g(&a[-30]+10);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+30);
+    g(&a[11]);             /* { dg-warning "array subscript" } */
+    g(&a[-30]+10);         /* { dg-warning "array subscript" } */
+    g(&a[-30]+30);         /* { dg-warning "array subscript" } */
 
     g(&b[10]);
     g(&c.c[10]);
-    g(&b[11]);             /* { dg-warning "array subscript" "" { xfail *-*-* } } */
+    g(&b[11]);             /* { dg-warning "array subscript" } */
     g(&c.c[11]);           /* { dg-warning "array subscript" } */
 
     g(&a[0]);
@@ -71,23 +72,52 @@
 
     g(&a[-1]);             /* { dg-warning "array subscript" } */
     g(&b[-1]);             /* { dg-warning "array subscript" } */ 
-    h(sizeof a[-1]);
+    h(sizeof a[-1]);       /* { dg-warning "array subscript" } */
     h(sizeof a[10]);
-    h(sizeof b[-1]);
+    h(sizeof b[-1]);       /* { dg-warning "array subscript" } */
     h(sizeof b[10]);
-    h(sizeof c.c[-1]);
+    h(sizeof c.c[-1]);     /* { dg-warning "array subscript" } */
     h(sizeof c.c[10]);
 
+    p[-1] = 0;             /* { dg-warning "array subscript" } */
+    p[0] = 0;
+    p[1] = 0;
+
+    q[-1] = 0;             /* { dg-warning "array subscript" } */
+    q[0] = 0;
+    q[1] = 0;
+    q[2] = 0;
+
+    r[-1] = 0;             /* { dg-warning "array subscript" } */
+    r[0] = 0;
+    r[1] = 0;
+    r[2] = 0;
+    r[3] = 0;              /* { dg-warning "array subscript" } */
+
+    s[-1] = 0;             /* { dg-warning "array subscript" } */
+    s[0] = 0;
+    s[1] = 0;
+    s[2] = 0;
+    s[3] = 0;
+    s[4] = 0;              /* { dg-warning "array subscript" } */
+
+    t[-1] = 0;             /* { dg-warning "array subscript" } */
+    t[0] = 0;
+    t[1] = 0;
+    t[2] = 0;
+    t[3] = 0;
+    t[4] = 0;
+    t[5] = 0;              /* { dg-warning "array subscript" } */
+
     if (10 < 10)
        a[10] = 0;
     if (10 < 10)
        b[10] = 0;
     if (-1 >= 0)
-       c.c[-1] = 0;
+       c.c[-1] = 0;        /* { dg-warning "array subscript" } */
 
     for (i = 20; i < 30; ++i)
              a[i] = 1;       /* { dg-warning "array subscript" } */
 
     return a;
 }
-
Index: gcc/testsuite/g++.dg/warn/Warray-bounds.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Warray-bounds.C	(revision 133772)
+++ gcc/testsuite/g++.dg/warn/Warray-bounds.C	(working copy)
@@ -17,6 +17,7 @@
     struct {
        int c[10];
     } c;
+    int p[0], q[1], r[2], s[3], t[4];
 
     a[-1] = 0;             /* { dg-warning "array subscript" } */
     a[ 0] = 0;
@@ -57,12 +58,11 @@
     g(&a[9]);
     g(&a[10]);
     g(&a[11]);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+10);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+30);
+    g(&a[-30]+10);         /* { dg-warning "array subscript" } */
+    g(&a[-30]+30);         /* { dg-warning "array subscript" } */
 
     g(&b[10]);
     g(&c.c[10]);
-    g(&a[11]);             /* { dg-warning "array subscript" } */
     g(&b[11]);             /* { dg-warning "array subscript" } */
     g(&c.c[11]);           /* { dg-warning "array subscript" } */
 
@@ -72,20 +72,52 @@
 
     g(&a[-1]);             /* { dg-warning "array subscript" } */
     g(&b[-1]);             /* { dg-warning "array subscript" } */ 
-    h(sizeof a[-1]);
+    h(sizeof a[-1]);       /* { dg-warning "array subscript" } */
     h(sizeof a[10]);
-    h(sizeof b[-1]);
+    h(sizeof b[-1]);       /* { dg-warning "array subscript" } */
     h(sizeof b[10]);
-    h(sizeof c.c[-1]);
+    h(sizeof c.c[-1]);     /* { dg-warning "array subscript" } */
     h(sizeof c.c[10]);
 
+    p[-1] = 0;             /* { dg-warning "array subscript" } */
+    p[0] = 0;
+    p[1] = 0;
+
+    q[-1] = 0;             /* { dg-warning "array subscript" } */
+    q[0] = 0;
+    q[1] = 0;
+    q[2] = 0;
+
+    r[-1] = 0;             /* { dg-warning "array subscript" } */
+    r[0] = 0;
+    r[1] = 0;
+    r[2] = 0;
+    r[3] = 0;              /* { dg-warning "array subscript" } */
+
+    s[-1] = 0;             /* { dg-warning "array subscript" } */
+    s[0] = 0;
+    s[1] = 0;
+    s[2] = 0;
+    s[3] = 0;
+    s[4] = 0;              /* { dg-warning "array subscript" } */
+
+    t[-1] = 0;             /* { dg-warning "array subscript" } */
+    t[0] = 0;
+    t[1] = 0;
+    t[2] = 0;
+    t[3] = 0;
+    t[4] = 0;
+    t[5] = 0;              /* { dg-warning "array subscript" } */
+
     if (10 < 10)
        a[10] = 0;
     if (10 < 10)
        b[10] = 0;
     if (-1 >= 0)
-       c.c[-1] = 0;
+       c.c[-1] = 0;        /* { dg-warning "array subscript" } */
+
+    for (i = 20; i < 30; ++i)
+             a[i] = 1;       /* { dg-warning "array subscript" } */
 
     return a;
 }
-
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 133772)
+++ gcc/cp/typeck.c	(working copy)
@@ -2554,7 +2554,8 @@
 
   if (TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE)
     {
-      tree rval, type;
+      bool has_warned_on_bounds_check = false;
+      tree rval, type, ref;
 
       warn_array_subscript_with_type_char (idx);
 
@@ -2571,6 +2572,10 @@
 	 pointer arithmetic.)  */
       idx = perform_integral_promotions (idx);
 
+      /* Warn about any obvious array bounds errors for fixed size arrays that
+         are indexed by a constant.  */
+      has_warned_on_bounds_check = warn_array_subscript_range (array, idx);
+
       /* An array that is indexed by a non-constant
 	 cannot be stored in a register; we must be able to do
 	 address arithmetic on its address.
@@ -2621,7 +2626,12 @@
 	|= (CP_TYPE_VOLATILE_P (type) | TREE_SIDE_EFFECTS (array));
       TREE_THIS_VOLATILE (rval)
 	|= (CP_TYPE_VOLATILE_P (type) | TREE_THIS_VOLATILE (array));
-      return require_complete_type (fold_if_not_in_template (rval));
+      ref = require_complete_type (fold_if_not_in_template (rval));
+
+      /* Suppress bounds warning in tree-vrp.c if already warned here.  */
+      if (has_warned_on_bounds_check)
+        TREE_NO_WARNING (ref) = 1;
+      return ref;
     }
 
   {
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 133772)
+++ gcc/c-typeck.c	(working copy)
@@ -2086,7 +2086,12 @@
 
   if (TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE)
     {
-      tree rval, type;
+      tree rval, type, ref;
+      bool has_warned_on_bounds_check = false;
+
+      /* Warn about any obvious array bounds errors for fixed size arrays that
+         are indexed by a constant.  */
+      has_warned_on_bounds_check = warn_array_subscript_range (array, index);
 
       /* An array that is indexed by a non-constant
 	 cannot be stored in a register; we must be able to do
@@ -2139,7 +2144,12 @@
 	       in an inline function.
 	       Hope it doesn't break something else.  */
 	    | TREE_THIS_VOLATILE (array));
-      return require_complete_type (fold (rval));
+      ref = require_complete_type (fold (rval));
+
+      /* Suppress bounds warning in tree-vrp.c if already warned here.  */
+      if (has_warned_on_bounds_check)
+        TREE_NO_WARNING (ref) = 1;
+      return ref;
     }
   else
     {
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 133772)
+++ gcc/c-common.c	(working copy)
@@ -7281,6 +7281,59 @@
     warning (OPT_Wchar_subscripts, "array subscript has type %<char%>");
 }
 
+/* Warn about obvious array bounds errors for fixed size arrays that
+   are indexed by a constant.  This is a subset of similar checks in
+   tree-vrp.c; by doing this here we can get some level of checking
+   from non-optimized, non-vrp compilation.  Returns true if a warning
+   is issued.  */
+
+bool
+warn_array_subscript_range (const_tree array, const_tree index)
+{
+  if (TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE
+      && TYPE_DOMAIN (TREE_TYPE (array)) && TREE_CODE (index) == INTEGER_CST)
+    {
+      const_tree max_index;
+
+      max_index = TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (array)));
+      if (max_index && TREE_CODE (max_index) == INTEGER_CST
+          && tree_int_cst_lt (max_index, index)
+          && !tree_int_cst_equal (index, max_index)
+          /* Always allow off-by-one.  */
+          && !tree_int_cst_equal (int_const_binop (PLUS_EXPR,
+                                                   max_index,
+                                                   integer_one_node,
+                                                   0),
+                                  index)
+          /* Accesses after the end of arrays of size 0 (gcc
+             extension) and 1 are likely intentional ("struct
+             hack").  Note that max_index is array dimension - 1.  */
+          && compare_tree_int (max_index, 1) >= 0)
+        {
+          warning (OPT_Warray_bounds,
+                   "array subscript is above array bounds");
+          return true;
+        }
+      else
+        {
+          const_tree min_index;
+
+          min_index = TYPE_MIN_VALUE (TYPE_DOMAIN (TREE_TYPE (array)));
+          if (min_index && TREE_CODE (min_index) == INTEGER_CST
+              && tree_int_cst_lt (index, min_index))
+            {
+              warning (OPT_Warray_bounds,
+                       compare_tree_int (min_index, 0) == 0
+                           ? "array subscript is negative"
+                           : "array subscript is below array bounds");
+              return true;
+            }
+        }
+    }
+
+  return false;
+}
+
 /* Implement -Wparentheses for the unexpected C precedence rules, to
    cover cases like x + y << z which readers are likely to
    misinterpret.  We have seen an expression in which CODE is a binary
Index: gcc/c-common.h
===================================================================
--- gcc/c-common.h	(revision 133772)
+++ gcc/c-common.h	(working copy)
@@ -884,6 +884,7 @@
 extern tree builtin_type_for_size (int, bool);
 
 extern void warn_array_subscript_with_type_char (tree);
+extern bool warn_array_subscript_range (const_tree, const_tree);
 extern void warn_about_parentheses (enum tree_code, enum tree_code,
 				    enum tree_code);
 extern void warn_for_unused_label (tree label);

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Tom Tromey]

>>>>> "Simon" == Simon Baldwin <simonb@google.com> writes:

Simon> One thing I wasn't able to fathom was how to suppress warnings
Simon> about "sizeof(a[-1])" in the C and C++ frontends; if anyone's got any
Simon> hints on this, I'd love to hear them.

The global 'skip_evaluation' is set while handling sizeof and some
other things.  It may be what you want.

Tom

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Simon Baldwin]

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

Thanks to a well targeted pointer from Tom Tromey, I've now added the 
extra logic to suppress warnings about "sizeof(a[-1])" in the C and C++ 
frontends.  Attached is the revised version.

So far I've been unable to reproduce the false warning from the current 
(unpatched) tree-vrp.c implementation, reported in gcc PR 35903.  I'll 
keep trying.

In the meantime, go ahead and commit, leave in limbo, or consign to 
history?  Thanks.



[-- Attachment #2: bounds.patch --]
[-- Type: text/x-patch, Size: 13290 bytes --]

This is version 3 of a patch to provide a subset of -Warray-bounds warnings
from tree-vrp.c in the C and C++ front ends.  This permits the compiler to
warn about egregious array bounds violations in unoptimized compilations or
compilations that may use -fno-tree-vrp.  At present, array bounds checking
is only done on optimized compilations.

A side effect of copying these warnings up into the language frontends is
that warnings are now printed even if the array access is in dead or
inaccessible code.

The current array bounds tests are modified to account for this new checking,
and additionally there are two new tests for warnings from -O0 compilations,
one for C and one for C++.

The patch also updates the current array bounds checking logic in tree-vrp.c
to agree with the code comments.

Bootstrapped, and regression tested on i686 Linux for gcc and g++.


:ADDPATCH diagnostic:

gcc/ChangeLog
2008-04-25  Simon Baldwin <simonb@google.com>

	* c-common.h (warn_array_subscript_range): New function.
	* c-common.c (warn_array_subscript_range): Ditto.
	* tree-vrp.c (check_array_ref): Corrected code to agree with
	comment, ignoring only arrays of size 0 or size 1.
	* c-typeck.c (build_array_ref): Call warn_array_subscript_range.

gcc/cp/ChangeLog
2008-04-25  Simon Baldwin <simonb@google.com>

	* typeck.c (build_array_ref): Call warn_array_subscript_range.

gcc/testsuite/ChangeLog
2008-04-25  Simon Baldwin <simonb@google.com>
  
	* testsuite/gcc.dg/Warray-bounds.c: Updated for frontend warnings,
	additional tests for arrays of size 0 and size 1.
	* testsuite/g++.dg/warn/Warray-bounds.c: Ditto.
	* testsuite/gcc.dg/Warray-bounds-noopt.c: New testcase.
	* testsuite/g++.dg/warn/Warray-bounds-noopt.c: Ditto.


Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c	(revision 133772)
+++ gcc/tree-vrp.c	(working copy)
@@ -4540,8 +4540,8 @@
           && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (ref))) == NULL_TREE)
       /* Accesses after the end of arrays of size 0 (gcc
          extension) and 1 are likely intentional ("struct
-         hack").  */
-      || compare_tree_int (up_bound, 1) <= 0)
+         hack").  Note that up_bound is array dimension - 1.  */
+      || compare_tree_int (up_bound, 1) < 0)
     return;
 
   low_bound = array_ref_low_bound (ref);
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 133772)
+++ gcc/doc/invoke.texi	(working copy)
@@ -2658,7 +2658,7 @@
 @option{-Wall} turns on the following warning flags:
 
 @gccoptlist{-Waddress   @gol
--Warray-bounds @r{(only with} @option{-O2}@r{)}  @gol
+-Warray-bounds @r{(some checks, but more complete with} @option{-O2}@r{)}  @gol
 -Wc++0x-compat  @gol
 -Wchar-subscripts  @gol
 -Wimplicit-int  @gol
@@ -3361,7 +3361,8 @@
 @item -Warray-bounds
 @opindex Wno-array-bounds
 @opindex Warray-bounds
-This option is only active when @option{-ftree-vrp} is active
+This option performs a subset of checks in unoptimized compilations, and
+stricter checking when @option{-ftree-vrp} is active
 (default for -O2 and above). It warns about subscripts to arrays
 that are always out of bounds. This warning is enabled by @option{-Wall}.
 
Index: gcc/testsuite/gcc.dg/Warray-bounds.c
===================================================================
--- gcc/testsuite/gcc.dg/Warray-bounds.c	(revision 133772)
+++ gcc/testsuite/gcc.dg/Warray-bounds.c	(working copy)
@@ -17,6 +17,7 @@
     struct {
        int c[10];
     } c;
+    int p[0], q[1], r[2], s[3], t[4];
 
     a[-1] = 0;             /* { dg-warning "array subscript" } */
     a[ 0] = 0;
@@ -56,13 +57,13 @@
     g(&a[8]);
     g(&a[9]);
     g(&a[10]);
-    g(&a[11]);             /* { dg-warning "array subscript" "" { xfail *-*-* } } */
-    g(&a[-30]+10);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+30);
+    g(&a[11]);             /* { dg-warning "array subscript" } */
+    g(&a[-30]+10);         /* { dg-warning "array subscript" } */
+    g(&a[-30]+30);         /* { dg-warning "array subscript" } */
 
     g(&b[10]);
     g(&c.c[10]);
-    g(&b[11]);             /* { dg-warning "array subscript" "" { xfail *-*-* } } */
+    g(&b[11]);             /* { dg-warning "array subscript" } */
     g(&c.c[11]);           /* { dg-warning "array subscript" } */
 
     g(&a[0]);
@@ -78,16 +79,45 @@
     h(sizeof c.c[-1]);
     h(sizeof c.c[10]);
 
+    p[-1] = 0;             /* { dg-warning "array subscript" } */
+    p[0] = 0;
+    p[1] = 0;
+
+    q[-1] = 0;             /* { dg-warning "array subscript" } */
+    q[0] = 0;
+    q[1] = 0;
+    q[2] = 0;
+
+    r[-1] = 0;             /* { dg-warning "array subscript" } */
+    r[0] = 0;
+    r[1] = 0;
+    r[2] = 0;
+    r[3] = 0;              /* { dg-warning "array subscript" } */
+
+    s[-1] = 0;             /* { dg-warning "array subscript" } */
+    s[0] = 0;
+    s[1] = 0;
+    s[2] = 0;
+    s[3] = 0;
+    s[4] = 0;              /* { dg-warning "array subscript" } */
+
+    t[-1] = 0;             /* { dg-warning "array subscript" } */
+    t[0] = 0;
+    t[1] = 0;
+    t[2] = 0;
+    t[3] = 0;
+    t[4] = 0;
+    t[5] = 0;              /* { dg-warning "array subscript" } */
+
     if (10 < 10)
        a[10] = 0;
     if (10 < 10)
        b[10] = 0;
     if (-1 >= 0)
-       c.c[-1] = 0;
+       c.c[-1] = 0;        /* { dg-warning "array subscript" } */
 
     for (i = 20; i < 30; ++i)
              a[i] = 1;       /* { dg-warning "array subscript" } */
 
     return a;
 }
-
Index: gcc/testsuite/g++.dg/warn/Warray-bounds.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Warray-bounds.C	(revision 133772)
+++ gcc/testsuite/g++.dg/warn/Warray-bounds.C	(working copy)
@@ -17,6 +17,7 @@
     struct {
        int c[10];
     } c;
+    int p[0], q[1], r[2], s[3], t[4];
 
     a[-1] = 0;             /* { dg-warning "array subscript" } */
     a[ 0] = 0;
@@ -57,12 +58,11 @@
     g(&a[9]);
     g(&a[10]);
     g(&a[11]);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+10);             /* { dg-warning "array subscript" } */
-    g(&a[-30]+30);
+    g(&a[-30]+10);         /* { dg-warning "array subscript" } */
+    g(&a[-30]+30);         /* { dg-warning "array subscript" } */
 
     g(&b[10]);
     g(&c.c[10]);
-    g(&a[11]);             /* { dg-warning "array subscript" } */
     g(&b[11]);             /* { dg-warning "array subscript" } */
     g(&c.c[11]);           /* { dg-warning "array subscript" } */
 
@@ -79,13 +79,45 @@
     h(sizeof c.c[-1]);
     h(sizeof c.c[10]);
 
+    p[-1] = 0;             /* { dg-warning "array subscript" } */
+    p[0] = 0;
+    p[1] = 0;
+
+    q[-1] = 0;             /* { dg-warning "array subscript" } */
+    q[0] = 0;
+    q[1] = 0;
+    q[2] = 0;
+
+    r[-1] = 0;             /* { dg-warning "array subscript" } */
+    r[0] = 0;
+    r[1] = 0;
+    r[2] = 0;
+    r[3] = 0;              /* { dg-warning "array subscript" } */
+
+    s[-1] = 0;             /* { dg-warning "array subscript" } */
+    s[0] = 0;
+    s[1] = 0;
+    s[2] = 0;
+    s[3] = 0;
+    s[4] = 0;              /* { dg-warning "array subscript" } */
+
+    t[-1] = 0;             /* { dg-warning "array subscript" } */
+    t[0] = 0;
+    t[1] = 0;
+    t[2] = 0;
+    t[3] = 0;
+    t[4] = 0;
+    t[5] = 0;              /* { dg-warning "array subscript" } */
+
     if (10 < 10)
        a[10] = 0;
     if (10 < 10)
        b[10] = 0;
     if (-1 >= 0)
-       c.c[-1] = 0;
+       c.c[-1] = 0;        /* { dg-warning "array subscript" } */
+
+    for (i = 20; i < 30; ++i)
+             a[i] = 1;       /* { dg-warning "array subscript" } */
 
     return a;
 }
-
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 133772)
+++ gcc/cp/typeck.c	(working copy)
@@ -2554,7 +2554,8 @@
 
   if (TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE)
     {
-      tree rval, type;
+      bool has_warned_on_bounds_check = false;
+      tree rval, type, ref;
 
       warn_array_subscript_with_type_char (idx);
 
@@ -2571,6 +2572,10 @@
 	 pointer arithmetic.)  */
       idx = perform_integral_promotions (idx);
 
+      /* Warn about any obvious array bounds errors for fixed size arrays that
+         are indexed by a constant.  */
+      has_warned_on_bounds_check = warn_array_subscript_range (array, idx);
+
       /* An array that is indexed by a non-constant
 	 cannot be stored in a register; we must be able to do
 	 address arithmetic on its address.
@@ -2621,7 +2626,12 @@
 	|= (CP_TYPE_VOLATILE_P (type) | TREE_SIDE_EFFECTS (array));
       TREE_THIS_VOLATILE (rval)
 	|= (CP_TYPE_VOLATILE_P (type) | TREE_THIS_VOLATILE (array));
-      return require_complete_type (fold_if_not_in_template (rval));
+      ref = require_complete_type (fold_if_not_in_template (rval));
+
+      /* Suppress bounds warning in tree-vrp.c if already warned here.  */
+      if (has_warned_on_bounds_check)
+        TREE_NO_WARNING (ref) = 1;
+      return ref;
     }
 
   {
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 133772)
+++ gcc/c-typeck.c	(working copy)
@@ -2086,7 +2086,12 @@
 
   if (TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE)
     {
-      tree rval, type;
+      tree rval, type, ref;
+      bool has_warned_on_bounds_check = false;
+
+      /* Warn about any obvious array bounds errors for fixed size arrays that
+         are indexed by a constant.  */
+      has_warned_on_bounds_check = warn_array_subscript_range (array, index);
 
       /* An array that is indexed by a non-constant
 	 cannot be stored in a register; we must be able to do
@@ -2139,7 +2144,12 @@
 	       in an inline function.
 	       Hope it doesn't break something else.  */
 	    | TREE_THIS_VOLATILE (array));
-      return require_complete_type (fold (rval));
+      ref = require_complete_type (fold (rval));
+
+      /* Suppress bounds warning in tree-vrp.c if already warned here.  */
+      if (has_warned_on_bounds_check)
+        TREE_NO_WARNING (ref) = 1;
+      return ref;
     }
   else
     {
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 133772)
+++ gcc/c-common.c	(working copy)
@@ -7281,6 +7281,60 @@
     warning (OPT_Wchar_subscripts, "array subscript has type %<char%>");
 }
 
+/* Warn about obvious array bounds errors for fixed size arrays that
+   are indexed by a constant.  This is a subset of similar checks in
+   tree-vrp.c; by doing this here we can get some level of checking
+   from non-optimized, non-vrp compilation.  Returns true if a warning
+   is issued.  */
+
+bool
+warn_array_subscript_range (const_tree array, const_tree index)
+{
+  if (skip_evaluation == 0
+      && TREE_CODE (TREE_TYPE (array)) == ARRAY_TYPE
+      && TYPE_DOMAIN (TREE_TYPE (array)) && TREE_CODE (index) == INTEGER_CST)
+    {
+      const_tree max_index;
+
+      max_index = TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (array)));
+      if (max_index && TREE_CODE (max_index) == INTEGER_CST
+          && tree_int_cst_lt (max_index, index)
+          && !tree_int_cst_equal (index, max_index)
+          /* Always allow off-by-one.  */
+          && !tree_int_cst_equal (int_const_binop (PLUS_EXPR,
+                                                   max_index,
+                                                   integer_one_node,
+                                                   0),
+                                  index)
+          /* Accesses after the end of arrays of size 0 (gcc
+             extension) and 1 are likely intentional ("struct
+             hack").  Note that max_index is array dimension - 1.  */
+          && compare_tree_int (max_index, 1) >= 0)
+        {
+          warning (OPT_Warray_bounds,
+                   "array subscript is above array bounds");
+          return true;
+        }
+      else
+        {
+          const_tree min_index;
+
+          min_index = TYPE_MIN_VALUE (TYPE_DOMAIN (TREE_TYPE (array)));
+          if (min_index && TREE_CODE (min_index) == INTEGER_CST
+              && tree_int_cst_lt (index, min_index))
+            {
+              warning (OPT_Warray_bounds,
+                       compare_tree_int (min_index, 0) == 0
+                           ? "array subscript is negative"
+                           : "array subscript is below array bounds");
+              return true;
+            }
+        }
+    }
+
+  return false;
+}
+
 /* Implement -Wparentheses for the unexpected C precedence rules, to
    cover cases like x + y << z which readers are likely to
    misinterpret.  We have seen an expression in which CODE is a binary
Index: gcc/c-common.h
===================================================================
--- gcc/c-common.h	(revision 133772)
+++ gcc/c-common.h	(working copy)
@@ -884,6 +884,7 @@
 extern tree builtin_type_for_size (int, bool);
 
 extern void warn_array_subscript_with_type_char (tree);
+extern bool warn_array_subscript_range (const_tree, const_tree);
 extern void warn_about_parentheses (enum tree_code, enum tree_code,
 				    enum tree_code);
 extern void warn_for_unused_label (tree label);

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Mark Mitchell]

Simon Baldwin wrote:
> Thanks to a well targeted pointer from Tom Tromey, I've now added the 
> extra logic to suppress warnings about "sizeof(a[-1])" in the C and C++ 
> frontends.  Attached is the revised version.

Thanks for following up.  I think this is a worthwhile patch.

> +This option performs a subset of checks in unoptimized compilations, and
> +stricter checking when @option{-ftree-vrp} is active
>  (default for -O2 and above). 

I think "stricter" could be made more explicit.  How about something like:

This option detects some cases of out-of-bounds accesses in unoptimized 
compilations.  More cases are detected when @option{-ftree-vrp} is 
enabled.  (The @option{-ftree-vrp} option is enabled automatically when 
compiling with @option{-O2} or higher optimization options.)

Joseph, do you have any comments on the C changes?  Simon, if you do not 
hear otherwise from Joseph within 72 hours, this patch is OK.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Joseph S. Myers]

On Sat, 26 Apr 2008, Mark Mitchell wrote:

> Joseph, do you have any comments on the C changes?  Simon, if you do not hear

I have no comments on this patch to add to those other people have already 
made on the various versions of it.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Simon Baldwin]

Thanks all.  Committed, with suggested doc/invoke.texi change, as 
revision 134865.


Mark Mitchell wrote:
> Simon Baldwin wrote:
>> Thanks to a well targeted pointer from Tom Tromey, I've now added the 
>> extra logic to suppress warnings about "sizeof(a[-1])" in the C and 
>> C++ frontends.  Attached is the revised version.
>
> Thanks for following up.  I think this is a worthwhile patch.
>
>> +This option performs a subset of checks in unoptimized compilations, 
>> and
>> +stricter checking when @option{-ftree-vrp} is active
>>  (default for -O2 and above). 
>
> I think "stricter" could be made more explicit.  How about something 
> like:
>
> This option detects some cases of out-of-bounds accesses in 
> unoptimized compilations.  More cases are detected when 
> @option{-ftree-vrp} is enabled.  (The @option{-ftree-vrp} option is 
> enabled automatically when compiling with @option{-O2} or higher 
> optimization options.)
>
> Joseph, do you have any comments on the C changes?  Simon, if you do 
> not hear otherwise from Joseph within 72 hours, this patch is OK.
>
> Thanks,
>

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[H.J. Lu]

Hi Simon,

Your patch breaks gcc bootstrap:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36108


H.J.
On Thu, May 1, 2008 at 12:07 PM, Simon Baldwin <simonb@google.com> wrote:
> Thanks all.  Committed, with suggested doc/invoke.texi change, as revision
> 134865.
>
>
>
>
>  Mark Mitchell wrote:
>
> > Simon Baldwin wrote:
> >
> > > Thanks to a well targeted pointer from Tom Tromey, I've now added the
> extra logic to suppress warnings about "sizeof(a[-1])" in the C and C++
> frontends.  Attached is the revised version.
> > >
> >
> > Thanks for following up.  I think this is a worthwhile patch.
> >
> >
> > > +This option performs a subset of checks in unoptimized compilations,
> and
> > > +stricter checking when @option{-ftree-vrp} is active
> > >  (default for -O2 and above).
> > >
> >
> > I think "stricter" could be made more explicit.  How about something like:
> >
> > This option detects some cases of out-of-bounds accesses in unoptimized
> compilations.  More cases are detected when @option{-ftree-vrp} is enabled.
> (The @option{-ftree-vrp} option is enabled automatically when compiling with
> @option{-O2} or higher optimization options.)
> >
> > Joseph, do you have any comments on the C changes?  Simon, if you do not
> hear otherwise from Joseph within 72 hours, this patch is OK.
> >
> > Thanks,
> >
> >
>
>

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[H.J. Lu]

Hi Simon,

In http://gcc.gnu.org/ml/gcc-patches/2008-04/msg01945.html, you said

---
A side effect of copying these warnings up into the language frontends is
that warnings are now printed even if the array access is in dead or
inaccessible code.
---

So this behavior is intentional. I don't see how it will be useful. Should
this patch be reverted for now?

Thanks.


H.J.
---
On Fri, May 2, 2008 at 5:50 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> Hi Simon,
>
>  Your patch breaks gcc bootstrap:
>
>  http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36108
>
>
>  H.J.
>
>
> On Thu, May 1, 2008 at 12:07 PM, Simon Baldwin <simonb@google.com> wrote:
>  > Thanks all.  Committed, with suggested doc/invoke.texi change, as revision
>  > 134865.
>  >
>  >
>  >
>  >
>  >  Mark Mitchell wrote:
>  >
>  > > Simon Baldwin wrote:
>  > >
>  > > > Thanks to a well targeted pointer from Tom Tromey, I've now added the
>  > extra logic to suppress warnings about "sizeof(a[-1])" in the C and C++
>  > frontends.  Attached is the revised version.
>  > > >
>  > >
>  > > Thanks for following up.  I think this is a worthwhile patch.
>  > >
>  > >
>  > > > +This option performs a subset of checks in unoptimized compilations,
>  > and
>  > > > +stricter checking when @option{-ftree-vrp} is active
>  > > >  (default for -O2 and above).
>  > > >
>  > >
>  > > I think "stricter" could be made more explicit.  How about something like:
>  > >
>  > > This option detects some cases of out-of-bounds accesses in unoptimized
>  > compilations.  More cases are detected when @option{-ftree-vrp} is enabled.
>  > (The @option{-ftree-vrp} option is enabled automatically when compiling with
>  > @option{-O2} or higher optimization options.)
>  > >
>  > > Joseph, do you have any comments on the C changes?  Simon, if you do not
>  > hear otherwise from Joseph within 72 hours, this patch is OK.
>  > >
>  > > Thanks,
>  > >
>  > >
>  >
>  >
>

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Mark Mitchell]

H.J. Lu wrote:

> A side effect of copying these warnings up into the language frontends is
> that warnings are now printed even if the array access is in dead or
> inaccessible code.

We've had this discussion for years.  The arguments are:

1. Users only care about problems that actually affect their build.  Let 
the optimizers do their thing and warn about problems that are detected 
that way.  This avoids warning about things in dead code, and it results 
in warnings that occur after (say) simplifications from inlining.

2. Users care about problems that might occur when building their code 
in some different mode.  Users want the same warnings across platforms, 
and whether optimizing or not.  Thus, we should emit warnings only from 
front ends, without trying to avoid dead code, and without doing a lot 
of transformation on the code.

GCC has traditionally done (1).  I've argued for (2), which is what most 
compilers do.  Simon's patch makes it do a bit of (2), while still doing 
(1).  This seems to me like a reasonable compromise; you give up none of 
the warnings in (1), but still get many of the benefits of (2).

Obviously, Simon needs to fix the bootstrap issue.  But, I don't see a 
fundamental problem with the patch -- at least not yet.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[H.J. Lu]

On Fri, May 2, 2008 at 7:06 AM, Mark Mitchell <mark@codesourcery.com> wrote:
> H.J. Lu wrote:
>
>
> > A side effect of copying these warnings up into the language frontends is
> > that warnings are now printed even if the array access is in dead or
> > inaccessible code.
> >
>
>  We've had this discussion for years.  The arguments are:
>
>  1. Users only care about problems that actually affect their build.  Let
> the optimizers do their thing and warn about problems that are detected that
> way.  This avoids warning about things in dead code, and it results in
> warnings that occur after (say) simplifications from inlining.
>
>  2. Users care about problems that might occur when building their code in
> some different mode.  Users want the same warnings across platforms, and
> whether optimizing or not.  Thus, we should emit warnings only from front
> ends, without trying to avoid dead code, and without doing a lot of
> transformation on the code.
>
>  GCC has traditionally done (1).  I've argued for (2), which is what most
> compilers do.  Simon's patch makes it do a bit of (2), while still doing
> (1).  This seems to me like a reasonable compromise; you give up none of the
> warnings in (1), but still get many of the benefits of (2).
>
>  Obviously, Simon needs to fix the bootstrap issue.  But, I don't see a
> fundamental problem with the patch -- at least not yet.
>

If it doesn't work on gcc itself, I don' think it will be much useful elsewhere.
I suggest

1. At -O0, the frond-end warning should never be turned into an error
even if -Werror is used since the frond-end doesn't know if the code
will be dead or not.
2, At -O1 and above, the frond-end warning should be queued and
optimizer can remove it from the queue if the code in question turns
out dead.


H.J.

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Simon Baldwin]

H.J. Lu wrote:
> On Fri, May 2, 2008 at 7:06 AM, Mark Mitchell <mark@codesourcery.com> wrote:
>   
>> H.J. Lu wrote:
>>
>>
>>     
>>> A side effect of copying these warnings up into the language frontends is
>>> that warnings are now printed even if the array access is in dead or
>>> inaccessible code.
>>>
>>>       
>>  We've had this discussion for years.  The arguments are:
>>
>>  1. Users only care about problems that actually affect their build.  Let
>> the optimizers do their thing and warn about problems that are detected that
>> way.  This avoids warning about things in dead code, and it results in
>> warnings that occur after (say) simplifications from inlining.
>>
>>  2. Users care about problems that might occur when building their code in
>> some different mode.  Users want the same warnings across platforms, and
>> whether optimizing or not.  Thus, we should emit warnings only from front
>> ends, without trying to avoid dead code, and without doing a lot of
>> transformation on the code.
>>
>>  GCC has traditionally done (1).  I've argued for (2), which is what most
>> compilers do.  Simon's patch makes it do a bit of (2), while still doing
>> (1).  This seems to me like a reasonable compromise; you give up none of the
>> warnings in (1), but still get many of the benefits of (2).
>>
>>  Obviously, Simon needs to fix the bootstrap issue.  But, I don't see a
>> fundamental problem with the patch -- at least not yet.
>>
>>     
>
> If it doesn't work on gcc itself, I don' think it will be much useful elsewhere.
> I suggest
>
> 1. At -O0, the frond-end warning should never be turned into an error
> even if -Werror is used since the frond-end doesn't know if the code
> will be dead or not.
> 2, At -O1 and above, the frond-end warning should be queued and
> optimizer can remove it from the queue if the code in question turns
> out dead.
>   


I think the wisest course for now is to roll back this change until I 
have a better understanding of the issues.  There's obviously a case 
here that's not going be rare overall, and with which the additional 
front-end warning cannot cope well in its current form.

I'm putting together the roll back change.  It should be done in a short 
while.

Thanks all for the comments and feedback here.

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Mark Mitchell]

Simon Baldwin wrote:

> I think the wisest course for now is to roll back this change until I 
> have a better understanding of the issues.

If you want to roll back, that's fine.  You can do that without any 
further approval.

Thanks,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Andrew Pinski]

On Fri, May 2, 2008 at 9:34 AM, Mark Mitchell <mark@codesourcery.com> wrote:
> Simon Baldwin wrote:
>
>
> > I think the wisest course for now is to roll back this change until I have
> a better understanding of the issues.
> >
>
>  If you want to roll back, that's fine.  You can do that without any further
> approval.

x86-darwin is also broken with the following warning:
/home/regress/tbox/svn-gcc/gcc/config/i386/i386.c:20877: error: array
subscript is above array bounds

which is a false warning.
We have:
mode0 = insn_data[icode].operand[0].mode;

where insn_data is defined as:
extern const struct insn_data insn_data[];

operand is a pointer so that is not an issue.

Thanks,
Andrew Pinski

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[H.J. Lu]

On Fri, May 2, 2008 at 12:53 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Fri, May 2, 2008 at 9:34 AM, Mark Mitchell <mark@codesourcery.com> wrote:
>  > Simon Baldwin wrote:
>  >
>  >
>  > > I think the wisest course for now is to roll back this change until I have
>  > a better understanding of the issues.
>  > >
>  >
>  >  If you want to roll back, that's fine.  You can do that without any further
>  > approval.
>
>  x86-darwin is also broken with the following warning:
>  /home/regress/tbox/svn-gcc/gcc/config/i386/i386.c:20877: error: array
>
> subscript is above array bounds
>
>  which is a false warning.
>  We have:
>  mode0 = insn_data[icode].operand[0].mode;
>
>  where insn_data is defined as:
>  extern const struct insn_data insn_data[];
>
>  operand is a pointer so that is not an issue.
>

May I suggest to revert it now and use a branch to implement it
properly before merging it with trunk? I can prepare a patch in half
an hour.

Thanks.

H.J.

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Simon Baldwin]

H.J. Lu wrote:
> On Fri, May 2, 2008 at 12:53 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>   
>> On Fri, May 2, 2008 at 9:34 AM, Mark Mitchell <mark@codesourcery.com> wrote:
>>  > Simon Baldwin wrote:
>>  >
>>  >
>>  > > I think the wisest course for now is to roll back this change until I have
>>  > a better understanding of the issues.
>>  > >
>>  >
>>  >  If you want to roll back, that's fine.  You can do that without any further
>>  > approval.
>>
>>  x86-darwin is also broken with the following warning:
>>  /home/regress/tbox/svn-gcc/gcc/config/i386/i386.c:20877: error: array
>>
>> subscript is above array bounds
>>
>>  which is a false warning.
>>  We have:
>>  mode0 = insn_data[icode].operand[0].mode;
>>
>>  where insn_data is defined as:
>>  extern const struct insn_data insn_data[];
>>
>>  operand is a pointer so that is not an issue.
>>
>>     
>
> May I suggest to revert it now and use a branch to implement it
> properly before merging it with trunk? I can prepare a patch in half
> an hour.
>   

I've just this minute committed the rollback -- revision 134889.  
Testing did not complete as quick as I'd hoped; sorry for the delay.

Thanks.  --S

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[H.J. Lu]

On Fri, May 2, 2008 at 1:03 PM, Simon Baldwin <simonb@google.com> wrote:
>
>  I've just this minute committed the rollback -- revision 134889.  Testing
> did not complete as quick as I'd hoped; sorry for the delay.
>

Thanks.


H.J.

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Paul Koning]

>>>>> "H" == H J Lu <H.J.> writes:

 H> On Fri, May 2, 2008 at 7:06 AM, Mark Mitchell
 H> <mark@codesourcery.com> wrote:
 >> H.J. Lu wrote:
 >> 
 >> 
 >> > A side effect of copying these warnings up into the language
 >> frontends is > that warnings are now printed even if the array
 >> access is in dead or > inaccessible code.
 >> >
 >> 
 >> We've had this discussion for years.  The arguments are:
 >> 
 >> 1. Users only care about problems that actually affect their
 >> build.  Let the optimizers do their thing and warn about problems
 >> that are detected that way.  This avoids warning about things in
 >> dead code, and it results in warnings that occur after (say)
 >> simplifications from inlining.
 >> 
 >> 2. Users care about problems that might occur when building their
 >> code in some different mode.  Users want the same warnings across
 >> platforms, and whether optimizing or not.  Thus, we should emit
 >> warnings only from front ends, without trying to avoid dead code,
 >> and without doing a lot of transformation on the code.
 >> 
 >> GCC has traditionally done (1).  I've argued for (2), which is
 >> what most compilers do.  Simon's patch makes it do a bit of (2),
 >> while still doing (1).  This seems to me like a reasonable
 >> compromise; you give up none of the warnings in (1), but still get
 >> many of the benefits of (2).
 >> 
 >> Obviously, Simon needs to fix the bootstrap issue.  But, I don't
 >> see a fundamental problem with the patch -- at least not yet.
 >> 

 H> If it doesn't work on gcc itself, I don' think it will be much
 H> useful elsewhere.  I suggest

 H> 1. At -O0, the frond-end warning should never be turned into an
 H> error even if -Werror is used since the frond-end doesn't know if
 H> the code will be dead or not.  2, At -O1 and above, the frond-end
 H> warning should be queued and optimizer can remove it from the
 H> queue if the code in question turns out dead.

I don't like this notion at all.

If a bit of code is wrong, I believe it is perfectly reasonable (and
in fact desirable) to call it wrong even if it happens to be
unreachable.

I wouldn't make that a requirement -- it's ok for warnings to come out
of the back end after optimization.  But I don't agree with the notion
that warnings about dead code are bad.

Dead code can come alive with very small source code changes.  As a
matter of good software engineering, it's helpful for warnings to
appear when the bad code is created.  If the warning appears 6 months
later as a side effect of a small change elsewhere, the repair can be
significantly harder.

Also, it seems to me that suppressing warnings as a side effect of
optimization can produce results somewhat like the one we recently
debated at length with CERT.  Not quite the same, but somewhat like
it. 

In other words, I would support Mark's preference for approach #2, for
the same sort of reasons he gave.

    paul

Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends

[Dirk Mueller]

On Friday 04 April 2008, Simon Baldwin wrote:

> Attached below is a modest patch to provide a subset of the -Warray-bounds
> warnings from tree-vrp.c in the C and C++ front ends.  

Is there a reason you haven't implemented that in c-common.c with hooks in the 
different frontends calling it?

 dg-warning "array subscript" } */

> -    h(sizeof a[-1]);
> +    h(sizeof a[-1]);       /* { dg-warning "array subscript" } */

I'm not sure it should be warning here, and I think it would be possible to 
suppress the warning here. 


Thanks,
Dirk