From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20094 invoked by alias); 11 Apr 2008 19:48:01 -0000 Received: (qmail 20020 invoked by uid 22791); 11 Apr 2008 19:47:58 -0000 X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.33.17) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 11 Apr 2008 19:47:40 +0000 Received: from zps75.corp.google.com (zps75.corp.google.com [172.25.146.75]) by smtp-out.google.com with ESMTP id m3BJkabS003301; Fri, 11 Apr 2008 20:46:36 +0100 Received: from [172.18.116.157] (godot.corp.google.com [172.18.116.157]) by zps75.corp.google.com with ESMTP id m3BJkYC7017500; Fri, 11 Apr 2008 12:46:35 -0700 Message-ID: <47FFC01A.6050000@google.com> Date: Fri, 11 Apr 2008 20:37:00 -0000 From: Simon Baldwin User-Agent: Thunderbird 1.5.0.14pre (X11/20071022) MIME-Version: 1.0 To: Mark Mitchell , Richard Guenther , gcc-patches@gcc.gnu.org, Dirk Mueller Subject: Re: [PATCH][RFC] Add a subset of -Warray-bounds warnings to C/C++ front ends References: <20080404000715.3C75741AB58@localhost> <84fc9c000804040241l98b2f6esd052290fa66b4303@mail.gmail.com> <47FBC58A.5020702@codesourcery.com> In-Reply-To: <47FBC58A.5020702@codesourcery.com> Content-Type: multipart/mixed; boundary="------------070303020003050407060909" X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2008-04/txt/msg00966.txt.bz2 This is a multi-part message in MIME format. --------------070303020003050407060909 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1513 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. --------------070303020003050407060909 Content-Type: text/x-patch; name="bounds.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="bounds.patch" Content-length: 13998 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 * 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 * typeck.c (build_array_ref): Call warn_array_subscript_range. gcc/testsuite/ChangeLog 2008-04-09 Simon Baldwin * 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 %"); } +/* 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); --------------070303020003050407060909--