* [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends @ 2008-04-04 1:04 Simon Baldwin 2008-04-04 9:59 ` Richard Guenther 2008-04-08 16:07 ` Dirk Mueller 0 siblings, 2 replies; 28+ messages in thread From: Simon Baldwin @ 2008-04-04 1:04 UTC (permalink / raw) To: gcc-patches 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 { ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-04 1:04 [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends Simon Baldwin @ 2008-04-04 9:59 ` Richard Guenther 2008-04-04 23:50 ` Simon Baldwin 2008-04-08 19:42 ` Mark Mitchell 2008-04-08 16:07 ` Dirk Mueller 1 sibling, 2 replies; 28+ messages in thread From: Richard Guenther @ 2008-04-04 9:59 UTC (permalink / raw) To: Simon Baldwin; +Cc: gcc-patches 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-04 9:59 ` Richard Guenther @ 2008-04-04 23:50 ` Simon Baldwin 2008-04-05 0:50 ` Andrew Pinski 2008-04-08 19:42 ` Mark Mitchell 1 sibling, 1 reply; 28+ messages in thread From: Simon Baldwin @ 2008-04-04 23:50 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-04 23:50 ` Simon Baldwin @ 2008-04-05 0:50 ` Andrew Pinski 2008-04-07 22:51 ` Simon Baldwin ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Andrew Pinski @ 2008-04-05 0:50 UTC (permalink / raw) To: Simon Baldwin; +Cc: Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-05 0:50 ` Andrew Pinski @ 2008-04-07 22:51 ` Simon Baldwin 2008-04-08 9:43 ` Richard Guenther 2008-04-08 15:39 ` Dirk Mueller 2008-04-08 15:21 ` Dirk Mueller 2008-04-10 18:05 ` Andrew Pinski 2 siblings, 2 replies; 28+ messages in thread From: Simon Baldwin @ 2008-04-07 22:51 UTC (permalink / raw) To: Andrew Pinski; +Cc: Richard Guenther, gcc-patches 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? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-07 22:51 ` Simon Baldwin @ 2008-04-08 9:43 ` Richard Guenther 2008-04-08 15:39 ` Dirk Mueller 1 sibling, 0 replies; 28+ messages in thread From: Richard Guenther @ 2008-04-08 9:43 UTC (permalink / raw) To: Simon Baldwin; +Cc: Andrew Pinski, gcc-patches 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-07 22:51 ` Simon Baldwin 2008-04-08 9:43 ` Richard Guenther @ 2008-04-08 15:39 ` Dirk Mueller 1 sibling, 0 replies; 28+ messages in thread From: Dirk Mueller @ 2008-04-08 15:39 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-05 0:50 ` Andrew Pinski 2008-04-07 22:51 ` Simon Baldwin @ 2008-04-08 15:21 ` Dirk Mueller 2008-04-10 18:05 ` Andrew Pinski 2 siblings, 0 replies; 28+ messages in thread From: Dirk Mueller @ 2008-04-08 15:21 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-05 0:50 ` Andrew Pinski 2008-04-07 22:51 ` Simon Baldwin 2008-04-08 15:21 ` Dirk Mueller @ 2008-04-10 18:05 ` Andrew Pinski 2 siblings, 0 replies; 28+ messages in thread From: Andrew Pinski @ 2008-04-10 18:05 UTC (permalink / raw) To: Simon Baldwin; +Cc: Richard Guenther, gcc-patches 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-04 9:59 ` Richard Guenther 2008-04-04 23:50 ` Simon Baldwin @ 2008-04-08 19:42 ` Mark Mitchell 2008-04-11 20:37 ` Simon Baldwin 1 sibling, 1 reply; 28+ messages in thread From: Mark Mitchell @ 2008-04-08 19:42 UTC (permalink / raw) To: Richard Guenther; +Cc: Simon Baldwin, gcc-patches 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-08 19:42 ` Mark Mitchell @ 2008-04-11 20:37 ` Simon Baldwin 2008-04-15 19:18 ` Tom Tromey 2008-04-26 13:42 ` Simon Baldwin 0 siblings, 2 replies; 28+ messages in thread From: Simon Baldwin @ 2008-04-11 20:37 UTC (permalink / raw) To: Mark Mitchell, Richard Guenther, gcc-patches, Dirk Mueller [-- 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); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-11 20:37 ` Simon Baldwin @ 2008-04-15 19:18 ` Tom Tromey 2008-04-26 13:42 ` Simon Baldwin 1 sibling, 0 replies; 28+ messages in thread From: Tom Tromey @ 2008-04-15 19:18 UTC (permalink / raw) To: Simon Baldwin; +Cc: Mark Mitchell, Richard Guenther, gcc-patches, Dirk Mueller >>>>> "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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-11 20:37 ` Simon Baldwin 2008-04-15 19:18 ` Tom Tromey @ 2008-04-26 13:42 ` Simon Baldwin 2008-04-27 14:56 ` Mark Mitchell 1 sibling, 1 reply; 28+ messages in thread From: Simon Baldwin @ 2008-04-26 13:42 UTC (permalink / raw) To: gcc-patches; +Cc: Mark Mitchell, Richard Guenther, Dirk Mueller [-- 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); ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-26 13:42 ` Simon Baldwin @ 2008-04-27 14:56 ` Mark Mitchell 2008-04-27 15:05 ` Joseph S. Myers 2008-05-01 19:07 ` Simon Baldwin 0 siblings, 2 replies; 28+ messages in thread From: Mark Mitchell @ 2008-04-27 14:56 UTC (permalink / raw) To: Simon Baldwin Cc: gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-27 14:56 ` Mark Mitchell @ 2008-04-27 15:05 ` Joseph S. Myers 2008-05-01 19:07 ` Simon Baldwin 1 sibling, 0 replies; 28+ messages in thread From: Joseph S. Myers @ 2008-04-27 15:05 UTC (permalink / raw) To: Mark Mitchell; +Cc: Simon Baldwin, gcc-patches, Richard Guenther, Dirk Mueller 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-27 14:56 ` Mark Mitchell 2008-04-27 15:05 ` Joseph S. Myers @ 2008-05-01 19:07 ` Simon Baldwin 2008-05-02 12:51 ` H.J. Lu 1 sibling, 1 reply; 28+ messages in thread From: Simon Baldwin @ 2008-05-01 19:07 UTC (permalink / raw) To: Mark Mitchell Cc: gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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, > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-01 19:07 ` Simon Baldwin @ 2008-05-02 12:51 ` H.J. Lu 2008-05-02 12:58 ` H.J. Lu 0 siblings, 1 reply; 28+ messages in thread From: H.J. Lu @ 2008-05-02 12:51 UTC (permalink / raw) To: Simon Baldwin Cc: Mark Mitchell, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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, > > > > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 12:51 ` H.J. Lu @ 2008-05-02 12:58 ` H.J. Lu 2008-05-02 14:06 ` Mark Mitchell 0 siblings, 1 reply; 28+ messages in thread From: H.J. Lu @ 2008-05-02 12:58 UTC (permalink / raw) To: Simon Baldwin Cc: Mark Mitchell, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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, > > > > > > > > > > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 12:58 ` H.J. Lu @ 2008-05-02 14:06 ` Mark Mitchell 2008-05-02 16:04 ` H.J. Lu 0 siblings, 1 reply; 28+ messages in thread From: Mark Mitchell @ 2008-05-02 14:06 UTC (permalink / raw) To: H.J. Lu Cc: Simon Baldwin, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 14:06 ` Mark Mitchell @ 2008-05-02 16:04 ` H.J. Lu 2008-05-02 16:21 ` Simon Baldwin 2008-05-02 16:24 ` Paul Koning 0 siblings, 2 replies; 28+ messages in thread From: H.J. Lu @ 2008-05-02 16:04 UTC (permalink / raw) To: Mark Mitchell Cc: Simon Baldwin, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 16:04 ` H.J. Lu @ 2008-05-02 16:21 ` Simon Baldwin 2008-05-02 16:35 ` Mark Mitchell 2008-05-02 16:24 ` Paul Koning 1 sibling, 1 reply; 28+ messages in thread From: Simon Baldwin @ 2008-05-02 16:21 UTC (permalink / raw) To: H.J. Lu Cc: Mark Mitchell, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 16:21 ` Simon Baldwin @ 2008-05-02 16:35 ` Mark Mitchell 2008-05-02 19:53 ` Andrew Pinski 0 siblings, 1 reply; 28+ messages in thread From: Mark Mitchell @ 2008-05-02 16:35 UTC (permalink / raw) To: Simon Baldwin Cc: H.J. Lu, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 16:35 ` Mark Mitchell @ 2008-05-02 19:53 ` Andrew Pinski 2008-05-02 20:01 ` H.J. Lu 0 siblings, 1 reply; 28+ messages in thread From: Andrew Pinski @ 2008-05-02 19:53 UTC (permalink / raw) To: Mark Mitchell Cc: Simon Baldwin, H.J. Lu, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 19:53 ` Andrew Pinski @ 2008-05-02 20:01 ` H.J. Lu 2008-05-02 20:04 ` Simon Baldwin 0 siblings, 1 reply; 28+ messages in thread From: H.J. Lu @ 2008-05-02 20:01 UTC (permalink / raw) To: Andrew Pinski Cc: Mark Mitchell, Simon Baldwin, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 20:01 ` H.J. Lu @ 2008-05-02 20:04 ` Simon Baldwin 2008-05-02 20:11 ` H.J. Lu 0 siblings, 1 reply; 28+ messages in thread From: Simon Baldwin @ 2008-05-02 20:04 UTC (permalink / raw) To: H.J. Lu Cc: Andrew Pinski, Mark Mitchell, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 20:04 ` Simon Baldwin @ 2008-05-02 20:11 ` H.J. Lu 0 siblings, 0 replies; 28+ messages in thread From: H.J. Lu @ 2008-05-02 20:11 UTC (permalink / raw) To: Simon Baldwin Cc: Andrew Pinski, Mark Mitchell, gcc-patches, Richard Guenther, Dirk Mueller, Joseph S. Myers 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-05-02 16:04 ` H.J. Lu 2008-05-02 16:21 ` Simon Baldwin @ 2008-05-02 16:24 ` Paul Koning 1 sibling, 0 replies; 28+ messages in thread From: Paul Koning @ 2008-05-02 16:24 UTC (permalink / raw) To: hjl.tools; +Cc: mark, simonb, gcc-patches, richard.guenther, dmuell, joseph >>>>> "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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends 2008-04-04 1:04 [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends Simon Baldwin 2008-04-04 9:59 ` Richard Guenther @ 2008-04-08 16:07 ` Dirk Mueller 1 sibling, 0 replies; 28+ messages in thread From: Dirk Mueller @ 2008-04-08 16:07 UTC (permalink / raw) To: gcc-patches; +Cc: Simon Baldwin 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2008-05-02 20:11 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-04-04 1:04 [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends Simon Baldwin 2008-04-04 9:59 ` Richard Guenther 2008-04-04 23:50 ` Simon Baldwin 2008-04-05 0:50 ` Andrew Pinski 2008-04-07 22:51 ` Simon Baldwin 2008-04-08 9:43 ` Richard Guenther 2008-04-08 15:39 ` Dirk Mueller 2008-04-08 15:21 ` Dirk Mueller 2008-04-10 18:05 ` Andrew Pinski 2008-04-08 19:42 ` Mark Mitchell 2008-04-11 20:37 ` Simon Baldwin 2008-04-15 19:18 ` Tom Tromey 2008-04-26 13:42 ` Simon Baldwin 2008-04-27 14:56 ` Mark Mitchell 2008-04-27 15:05 ` Joseph S. Myers 2008-05-01 19:07 ` Simon Baldwin 2008-05-02 12:51 ` H.J. Lu 2008-05-02 12:58 ` H.J. Lu 2008-05-02 14:06 ` Mark Mitchell 2008-05-02 16:04 ` H.J. Lu 2008-05-02 16:21 ` Simon Baldwin 2008-05-02 16:35 ` Mark Mitchell 2008-05-02 19:53 ` Andrew Pinski 2008-05-02 20:01 ` H.J. Lu 2008-05-02 20:04 ` Simon Baldwin 2008-05-02 20:11 ` H.J. Lu 2008-05-02 16:24 ` Paul Koning 2008-04-08 16:07 ` Dirk Mueller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).