From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id DA67F3857C62 for ; Mon, 14 Sep 2020 18:34:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DA67F3857C62 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-76-CZIDk0yTPbubRpJFBL4nOA-1; Mon, 14 Sep 2020 14:34:34 -0400 X-MC-Unique: CZIDk0yTPbubRpJFBL4nOA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 458721084D7D; Mon, 14 Sep 2020 18:34:33 +0000 (UTC) Received: from pdp-11.redhat.com (ovpn-119-180.rdu2.redhat.com [10.10.119.180]) by smtp.corp.redhat.com (Postfix) with ESMTP id E87411002D77; Mon, 14 Sep 2020 18:34:31 +0000 (UTC) From: Marek Polacek To: Joseph Myers , Jason Merrill , GCC Patches Subject: [PATCH] c, c++: Implement -Wsizeof-array-div [PR91741] Date: Mon, 14 Sep 2020 14:34:15 -0400 Message-Id: <20200914183415.1184637-1-polacek@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0.003 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-14.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Sep 2020 18:34:41 -0000 This patch implements a new warning, -Wsizeof-array-div. It warns about code like int arr[10]; sizeof (arr) / sizeof (short); where we have a division of two sizeof expressions, where the first argument is an array, and the second sizeof does not equal the size of the array element. See e.g. . Clang makes it possible to suppress the warning by parenthesizing the second sizeof like this: sizeof (arr) / (sizeof (short)); so I followed suit. In the C++ FE this was rather easy, because finish_parenthesized_expr already set TREE_NO_WARNING. In the C FE it was trickier; I've added a NOP_EXPR to discern between the non-() and () versions. This warning is enabled by -Wall. An example of the output: x.c:5:23: warning: expression does not compute the number of elements in this array; element type is ‘int’, not ‘short int’ [-Wsizeof-array-div] 5 | return sizeof (arr) / sizeof (short); | ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ x.c:5:25: note: add parentheses around ‘sizeof (short int)’ to silence this warning 5 | return sizeof (arr) / sizeof (short); | ^~~~~~~~~~~~~~ | ( ) x.c:4:7: note: array ‘arr’ declared here 4 | int arr[10]; | ^~~ Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? gcc/c-family/ChangeLog: PR c++/91741 * c-common.h (maybe_warn_sizeof_array_div): Declare. * c-warn.c (sizeof_pointer_memaccess_warning): Unwrap NOP_EXPRs. (maybe_warn_sizeof_array_div): New function. * c.opt (Wsizeof-array-div): New option. gcc/c/ChangeLog: PR c++/91741 * c-parser.c (c_parser_binary_expression): Implement -Wsizeof-array-div. (c_parser_postfix_expression): Wrap a SIZEOF_EXPR in a NOP_EXPR. * c-tree.h (char_type_p): Declare. * c-typeck.c (char_type_p): No longer static. gcc/cp/ChangeLog: PR c++/91741 * typeck.c (cp_build_binary_op): Implement -Wsizeof-array-div. gcc/ChangeLog: PR c++/91741 * doc/invoke.texi: Document -Wsizeof-array-div. gcc/testsuite/ChangeLog: PR c++/91741 * c-c++-common/Wsizeof-pointer-div.c: Add dg-warning. * c-c++-common/Wsizeof-array-div1.c: New test. * g++.dg/warn/Wsizeof-array-div1.C: New test. * g++.dg/warn/Wsizeof-array-div2.C: New test. --- gcc/c-family/c-common.h | 1 + gcc/c-family/c-warn.c | 51 +++++++++++++++++ gcc/c-family/c.opt | 5 ++ gcc/c/c-parser.c | 46 ++++++++++----- gcc/c/c-tree.h | 1 + gcc/c/c-typeck.c | 2 +- gcc/cp/typeck.c | 10 +++- gcc/doc/invoke.texi | 19 +++++++ .../c-c++-common/Wsizeof-array-div1.c | 56 +++++++++++++++++++ .../c-c++-common/Wsizeof-pointer-div.c | 2 +- .../g++.dg/warn/Wsizeof-array-div1.C | 37 ++++++++++++ .../g++.dg/warn/Wsizeof-array-div2.C | 15 +++++ 12 files changed, 228 insertions(+), 17 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/Wsizeof-array-div1.c create mode 100644 gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C create mode 100644 gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 4fc64bc4aa6..443aaa2ca9c 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1321,6 +1321,7 @@ extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool); extern void warn_for_omitted_condop (location_t, tree); extern bool warn_for_restrict (unsigned, tree *, unsigned); extern void warn_for_address_or_pointer_of_packed_member (tree, tree); +extern void maybe_warn_sizeof_array_div (location_t, tree, tree, tree, tree); /* Places where an lvalue, or modifiable lvalue, may be required. Used to select diagnostic messages in lvalue_error and diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c index c32d8228b5c..e0f0cf65a57 100644 --- a/gcc/c-family/c-warn.c +++ b/gcc/c-family/c-warn.c @@ -864,6 +864,10 @@ sizeof_pointer_memaccess_warning (location_t *sizeof_arg_loc, tree callee, || error_operand_p (sizeof_arg[idx])) return; + /* Unwrap the NOP_EXPR signalling that the sizeof was parenthesized. */ + if (TREE_CODE (sizeof_arg[idx]) == NOP_EXPR) + sizeof_arg[idx] = TREE_OPERAND (sizeof_arg[idx], 0); + type = TYPE_P (sizeof_arg[idx]) ? sizeof_arg[idx] : TREE_TYPE (sizeof_arg[idx]); @@ -3099,3 +3103,50 @@ warn_for_address_or_pointer_of_packed_member (tree type, tree rhs) check_and_warn_address_or_pointer_of_packed_member (type, rhs); } + +/* Warn about divisions of two sizeof operators when the first one is applied + to an array and the divisor does not equal the size of the array element. + For instance: + + sizeof (ARR) / sizeof (OP) + + ARR is the array argument of the first sizeof, ARR_TYPE is its ARRAY_TYPE. + OP1 is the whole second SIZEOF_EXPR, or its argument; TYPE1 is the type + of the second argument. */ + +void +maybe_warn_sizeof_array_div (location_t loc, tree arr, tree arr_type, + tree op1, tree type1) +{ + tree elt_type = TREE_TYPE (arr_type); + + if (!warn_sizeof_array_div + /* Don't warn on multidimensional arrays. */ + || TREE_CODE (elt_type) == ARRAY_TYPE) + return; + + if (!tree_int_cst_equal (TYPE_SIZE (elt_type), TYPE_SIZE (type1))) + { + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wsizeof_array_div, + "expression does not compute the number of " + "elements in this array; element type is " + "%qT, not %qT", elt_type, type1)) + { + if (EXPR_HAS_LOCATION (op1)) + { + location_t op1_loc = EXPR_LOCATION (op1); + gcc_rich_location richloc (op1_loc); + richloc.add_fixit_insert_before (op1_loc, "("); + richloc.add_fixit_insert_after (op1_loc, ")"); + inform (&richloc, "add parentheses around %qE to " + "silence this warning", op1); + } + else + inform (loc, "add parentheses around the second % " + "to silence this warning"); + if (DECL_P (arr)) + inform (DECL_SOURCE_LOCATION (arr), "array %qD declared here", arr); + } + } +} diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index c1d8fd336e8..9ab44550140 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -799,6 +799,11 @@ Wsizeof-pointer-div C ObjC C++ ObjC++ Var(warn_sizeof_pointer_div) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about suspicious divisions of two sizeof expressions that don't work correctly with pointers. +Wsizeof-array-div +C ObjC C++ ObjC++ Var(warn_sizeof_array_div) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) +Warn about divisions of two sizeof operators when the first one is applied +to an array and the divisor does not equal the size of the array element. + Wsizeof-pointer-memaccess C ObjC C++ ObjC++ Var(warn_sizeof_pointer_memaccess) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about suspicious length parameters to certain string functions if the argument uses sizeof. diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index a8bc301ffad..7c9c5188b9e 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -7888,12 +7888,20 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, c_inhibit_evaluation_warnings -= (stack[sp - 1].expr.value \ == truthvalue_true_node); \ break; \ - case TRUNC_DIV_EXPR: \ + case TRUNC_DIV_EXPR: \ if (stack[sp - 1].expr.original_code == SIZEOF_EXPR \ && stack[sp].expr.original_code == SIZEOF_EXPR) \ { \ tree type0 = stack[sp - 1].sizeof_arg; \ tree type1 = stack[sp].sizeof_arg; \ + bool paren_p = false; \ + if (TREE_CODE (type0) == NOP_EXPR) \ + type0 = TREE_OPERAND (type0, 0); \ + if (TREE_CODE (type1) == NOP_EXPR) \ + { \ + type1 = TREE_OPERAND (type1, 0); \ + paren_p = true; \ + } \ tree first_arg = type0; \ if (!TYPE_P (type0)) \ type0 = TREE_TYPE (type0); \ @@ -7904,18 +7912,23 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, && !(TREE_CODE (first_arg) == PARM_DECL \ && C_ARRAY_PARAMETER (first_arg) \ && warn_sizeof_array_argument)) \ - { \ - auto_diagnostic_group d; \ - if (warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, \ - "division % " \ - "does not compute the number of array " \ - "elements", \ - type0, type1)) \ - if (DECL_P (first_arg)) \ - inform (DECL_SOURCE_LOCATION (first_arg), \ - "first % operand was declared here"); \ - } \ - } \ + { \ + auto_diagnostic_group d; \ + if (warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, \ + "division % " \ + "does not compute the number of array " \ + "elements", \ + type0, type1)) \ + if (DECL_P (first_arg)) \ + inform (DECL_SOURCE_LOCATION (first_arg), \ + "first % operand was declared here"); \ + } \ + else if (TREE_CODE (type0) == ARRAY_TYPE \ + && !char_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (type0))) \ + && !paren_p) \ + maybe_warn_sizeof_array_div (stack[sp].loc, first_arg, type0, \ + stack[sp].sizeof_arg, type1); \ + } \ break; \ default: \ break; \ @@ -9168,6 +9181,13 @@ c_parser_postfix_expression (c_parser *parser) expr = c_parser_expression (parser); if (TREE_CODE (expr.value) == MODIFY_EXPR) TREE_NO_WARNING (expr.value) = 1; + /* If we parsed a SIZEOF_EXPR, wrap it in a NOP_EXPR to remember + that it was parenthesized. */ + if (expr.original_code == SIZEOF_EXPR + && !error_operand_p (c_last_sizeof_arg)) + c_last_sizeof_arg = build1 (NOP_EXPR, + TREE_TYPE (c_last_sizeof_arg), + c_last_sizeof_arg); if (expr.original_code != C_MAYBE_CONST_EXPR && expr.original_code != SIZEOF_EXPR) expr.original_code = ERROR_MARK; diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index 10938cf0857..5b3751efb3a 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -664,6 +664,7 @@ extern location_t c_last_sizeof_loc; extern struct c_switch *c_switch_stack; +extern bool char_type_p (tree); extern tree c_objc_common_truthvalue_conversion (location_t, tree); extern tree require_complete_type (location_t, tree); extern bool same_translation_unit_p (const_tree, const_tree); diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index bb27099bfe1..8caf3dbf6db 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -3719,7 +3719,7 @@ parser_build_unary_op (location_t loc, enum tree_code code, struct c_expr arg) /* Returns true if TYPE is a character type, *not* including wchar_t. */ -static bool +bool char_type_p (tree type) { return (type == char_type_node diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index 9166156a5d5..7705f15c25f 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -4706,14 +4706,13 @@ cp_build_binary_op (const op_location_t &location, { tree type0 = TREE_OPERAND (op0, 0); tree type1 = TREE_OPERAND (op1, 0); - tree first_arg = type0; + tree first_arg = tree_strip_any_location_wrapper (type0); if (!TYPE_P (type0)) type0 = TREE_TYPE (type0); if (!TYPE_P (type1)) type1 = TREE_TYPE (type1); if (INDIRECT_TYPE_P (type0) && same_type_p (TREE_TYPE (type0), type1)) { - STRIP_ANY_LOCATION_WRAPPER (first_arg); if (!(TREE_CODE (first_arg) == PARM_DECL && DECL_ARRAY_PARAMETER_P (first_arg) && warn_sizeof_array_argument) @@ -4729,6 +4728,13 @@ cp_build_binary_op (const op_location_t &location, "first % operand was declared here"); } } + else if (TREE_CODE (type0) == ARRAY_TYPE + && !char_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (type0))) + /* Set by finish_parenthesized_expr. */ + && !TREE_NO_WARNING (op1) + && (complain & tf_warning)) + maybe_warn_sizeof_array_div (location, first_arg, type0, + op1, non_reference (type1)); } if ((code0 == INTEGER_TYPE || code0 == REAL_TYPE diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index e3e67c9b3bd..97ae25336ae 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -362,6 +362,7 @@ Objective-C and Objective-C++ Dialects}. -Wno-shift-overflow -Wshift-overflow=@var{n} @gol -Wsign-compare -Wsign-conversion @gol -Wno-sizeof-array-argument @gol +-Wsizeof-array-div @gol -Wsizeof-pointer-div -Wsizeof-pointer-memaccess @gol -Wstack-protector -Wstack-usage=@var{byte-size} -Wstrict-aliasing @gol -Wstrict-aliasing=n -Wstrict-overflow -Wstrict-overflow=@var{n} @gol @@ -5254,6 +5255,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wreturn-type @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol +-Wsizeof-array-div @gol -Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess @gol -Wstrict-aliasing @gol @@ -7946,6 +7948,23 @@ real to lower precision real values. This option is also enabled by @opindex Wscalar-storage-order Do not warn on suspicious constructs involving reverse scalar storage order. +@item -Wsizeof-array-div +@opindex Wsizeof-array-div +@opindex Wno-sizeof-array-div +Warn about divisions of two sizeof operators when the first one is applied +to an array and the divisor does not equal the size of the array element. +In such a case, the computation will not yield the number of elements in the +array, which is likely what the user intended. This warning warns e.g. about +@smallexample +int fn () +@{ + int arr[10]; + return sizeof (arr) / sizeof (short); +@} +@end smallexample + +This warning is enabled by @option{-Wall}. + @item -Wsizeof-pointer-div @opindex Wsizeof-pointer-div @opindex Wno-sizeof-pointer-div diff --git a/gcc/testsuite/c-c++-common/Wsizeof-array-div1.c b/gcc/testsuite/c-c++-common/Wsizeof-array-div1.c new file mode 100644 index 00000000000..84d9a730cba --- /dev/null +++ b/gcc/testsuite/c-c++-common/Wsizeof-array-div1.c @@ -0,0 +1,56 @@ +/* PR c++/91741 */ +/* { dg-do compile } */ +/* { dg-options "-Wall" } */ + +typedef int T; + +int +fn (int ap[]) +{ + int arr[10]; + int *arr2[10]; + int *p = &arr[0]; + int r = 0; + + r += sizeof (arr) / sizeof (*arr); + r += sizeof (arr) / sizeof (p); /* { dg-warning "expression does not compute" } */ + r += sizeof (arr) / sizeof p; /* { dg-warning "expression does not compute" } */ + r += sizeof (arr) / (sizeof p); + r += sizeof (arr) / (sizeof (p)); + r += sizeof (arr2) / sizeof p; + r += sizeof (arr2) / sizeof (int); /* { dg-warning "expression does not compute" } */ + r += sizeof (arr2) / sizeof (int *); + r += sizeof (arr2) / sizeof (short *); + r += sizeof (arr) / sizeof (int); + r += sizeof (arr) / sizeof (unsigned int); + r += sizeof (arr) / sizeof (T); + r += sizeof (arr) / sizeof (short); /* { dg-warning "expression does not compute" } */ + r += sizeof (arr) / (sizeof (short)); + + r += sizeof (ap) / sizeof (char); /* { dg-warning ".sizeof. on array function parameter" } */ + + const char arr3[] = "foo"; + r += sizeof (arr3) / sizeof(char); + r += sizeof (arr3) / sizeof(int); + r += sizeof (arr3) / sizeof (*arr3); + + int arr4[5][5]; + r += sizeof (arr4) / sizeof (arr4[0]); + r += sizeof (arr4) / sizeof (*arr4); + r += sizeof (arr4) / sizeof (**arr4); + r += sizeof (arr4) / sizeof (int *); + r += sizeof (arr4) / sizeof (int); + r += sizeof (arr4) / sizeof (short int); + + T arr5[10]; + r += sizeof (arr5) / sizeof (T); + r += sizeof (arr5) / sizeof (int); + r += sizeof (arr5) / sizeof (short); /* { dg-warning "expression does not compute" } */ + + double arr6[10]; + r += sizeof (arr6) / sizeof (double); + r += sizeof (arr6) / sizeof (float); /* { dg-warning "expression does not compute" } */ + r += sizeof (arr6) / sizeof (*arr6); + + return r; +} diff --git a/gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c b/gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c index 83116183902..e9bad1fa420 100644 --- a/gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c +++ b/gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c @@ -29,7 +29,7 @@ f2 (void) i += sizeof(array) / sizeof(array[0]); i += (sizeof(array)) / (sizeof(array[0])); i += sizeof(array) / sizeof(int); - i += sizeof(array) / sizeof(char); + i += sizeof(array) / sizeof(char); /* { dg-warning "expression does not compute" } */ i += sizeof(*array) / sizeof(char); i += sizeof(array[0]) / sizeof(char); return i; diff --git a/gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C b/gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C new file mode 100644 index 00000000000..da220cd57ba --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C @@ -0,0 +1,37 @@ +// PR c++/91741 +// { dg-do compile { target c++11 } } +// { dg-options "-Wall" } + +int +fn1 () +{ + int arr[10]; + return sizeof (arr) / sizeof (decltype(arr[0])); +} + +template +int fn2 (T (&arr)[N]) +{ + return sizeof (arr) / sizeof (T); +} + +template +int fn3 (T (&arr)[N]) +{ + return sizeof (arr) / sizeof (bool); // { dg-warning "expression does not compute" } +} + +template +int fn4 (T (&arr)[N]) +{ + return sizeof (arr) / sizeof (U); // { dg-warning "expression does not compute" } +} + +void +fn () +{ + int arr[10]; + fn2 (arr); + fn3 (arr); + fn4 (arr); +} diff --git a/gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C b/gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C new file mode 100644 index 00000000000..7962c23522c --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C @@ -0,0 +1,15 @@ +// PR c++/91741 +// { dg-do compile } +// { dg-options "-Wall" } +// From . + +const int kBaudrates[] = { 50, 75, 110 }; + +void +foo () +{ + for(int i = sizeof(kBaudrates) / sizeof(char*); // { dg-warning "expression does not compute" } + --i >= 0;) + { + } +} base-commit: 863e8d53eb2940e2c8850e632afe427e164f53cf -- 2.26.2