[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends
From:       "Richard Guenther" <richard.guenther () gmail ! com>
Date:       2008-04-08 9:31:19
Message-ID: 84fc9c000804080231g6f04781g1ebce79ce5a638af () mail ! gmail ! com
[Download RAW message or body]

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.
[prev in list] [next in list] [prev in thread] [next in thread]