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

[Simon Baldwin <simonb at google dot com>]

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?