From 83f83ddfe0fe41c9b553850d4ababd5089df8332 Mon Sep 17 00:00:00 2001 From: Marek Polacek Date: Fri, 11 Sep 2020 16:19:08 -0400 Subject: [PATCH] c, c++: Implement -Wsizeof-array-div [PR91741] MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 I've added a new tree code, PAREN_SIZEOF_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]; | ^~~ gcc/c-family/ChangeLog: PR c++/91741 * c-common.c (verify_tree): Handle PAREN_SIZEOF_EXPR. (c_common_init_ts): Likewise. * c-common.def (PAREN_SIZEOF_EXPR): New tree code. * 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): Set PAREN_SIZEOF_EXPR. (c_parser_expr_list): Handle PAREN_SIZEOF_EXPR like SIZEOF_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.c | 2 + gcc/c-family/c-common.def | 3 ++ gcc/c-family/c-common.h | 1 + gcc/c-family/c-warn.c | 47 ++++++++++++++++++++ gcc/c-family/c.opt | 5 +++ gcc/c/c-parser.c | 48 ++++++++++++-------- gcc/c/c-tree.h | 1 + gcc/c/c-typeck.c | 2 +- gcc/cp/typeck.c | 10 ++++- gcc/doc/invoke.texi | 19 ++++++++ gcc/testsuite/c-c++-common/Wsizeof-array-div1.c | 56 ++++++++++++++++++++++++ gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c | 2 +- gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C | 37 ++++++++++++++++ gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C | 15 +++++++ 14 files changed, 226 insertions(+), 22 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.c b/gcc/c-family/c-common.c index e16ca38..2c85634 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -1854,6 +1854,7 @@ verify_tree (tree x, struct tlist **pbefore_sp, struct tlist **pno_sp, { case CONSTRUCTOR: case SIZEOF_EXPR: + case PAREN_SIZEOF_EXPR: return; case COMPOUND_EXPR: @@ -8142,6 +8143,7 @@ void c_common_init_ts (void) { MARK_TS_EXP (SIZEOF_EXPR); + MARK_TS_EXP (PAREN_SIZEOF_EXPR); MARK_TS_EXP (C_MAYBE_CONST_EXPR); MARK_TS_EXP (EXCESS_PRECISION_EXPR); MARK_TS_EXP (BREAK_STMT); diff --git a/gcc/c-family/c-common.def b/gcc/c-family/c-common.def index 1954bfe..3d3e497 100644 --- a/gcc/c-family/c-common.def +++ b/gcc/c-family/c-common.def @@ -55,6 +55,9 @@ DEFTREECODE (USERDEF_LITERAL, "userdef_literal", tcc_exceptional, 3) or for the purpose of -Wsizeof-pointer-memaccess warning. */ DEFTREECODE (SIZEOF_EXPR, "sizeof_expr", tcc_expression, 1) +/* Like above, but enclosed in parentheses. Used to suppress warnings. */ +DEFTREECODE (PAREN_SIZEOF_EXPR, "paren_sizeof_expr", tcc_expression, 1) + /* Used to represent a `for' statement. The operands are FOR_INIT_STMT, FOR_COND, FOR_EXPR, FOR_BODY, and FOR_SCOPE, respectively. */ diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 3d96092..bb38e6c 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1373,6 +1373,7 @@ 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 warn_parm_array_mismatch (location_t, 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 a1b9450..68b093e 100644 --- a/gcc/c-family/c-warn.c +++ b/gcc/c-family/c-warn.c @@ -3665,3 +3665,50 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms) inform (origloc, "previously declared as %s", curparmstr.c_str ()); } } + +/* 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 bbf7da8..1009def 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -816,6 +816,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 7d58356..b6a7ef4 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -7876,7 +7876,7 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, enum tree_code op; /* The source location of this operation. */ location_t loc; - /* The sizeof argument if expr.original_code == SIZEOF_EXPR. */ + /* The sizeof argument if expr.original_code == {PAREN_,}SIZEOF_EXPR. */ tree sizeof_arg; } stack[NUM_PRECS]; int sp; @@ -7894,9 +7894,11 @@ 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: \ - if (stack[sp - 1].expr.original_code == SIZEOF_EXPR \ - && stack[sp].expr.original_code == SIZEOF_EXPR) \ + case TRUNC_DIV_EXPR: \ + if ((stack[sp - 1].expr.original_code == SIZEOF_EXPR \ + || stack[sp - 1].expr.original_code == PAREN_SIZEOF_EXPR) \ + && (stack[sp].expr.original_code == SIZEOF_EXPR \ + || stack[sp].expr.original_code == PAREN_SIZEOF_EXPR)) \ { \ tree type0 = stack[sp - 1].sizeof_arg; \ tree type1 = stack[sp].sizeof_arg; \ @@ -7910,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))) \ + && stack[sp].expr.original_code != PAREN_SIZEOF_EXPR) \ + maybe_warn_sizeof_array_div (stack[sp].loc, first_arg, type0, \ + stack[sp].sizeof_arg, type1); \ + } \ break; \ default: \ break; \ @@ -9177,6 +9184,9 @@ c_parser_postfix_expression (c_parser *parser) if (expr.original_code != C_MAYBE_CONST_EXPR && expr.original_code != SIZEOF_EXPR) expr.original_code = ERROR_MARK; + /* Remember that we saw ( ) around the sizeof. */ + if (expr.original_code == SIZEOF_EXPR) + expr.original_code = PAREN_SIZEOF_EXPR; /* Don't change EXPR.ORIGINAL_TYPE. */ location_t loc_close_paren = c_parser_peek_token (parser)->location; set_c_expr_source_range (&expr, loc_open_paren, loc_close_paren); @@ -10792,7 +10802,8 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, if (locations) locations->safe_push (expr.get_location ()); if (sizeof_arg != NULL - && expr.original_code == SIZEOF_EXPR) + && (expr.original_code == SIZEOF_EXPR + || expr.original_code == PAREN_SIZEOF_EXPR)) { sizeof_arg[0] = c_last_sizeof_arg; sizeof_arg_loc[0] = c_last_sizeof_loc; @@ -10815,7 +10826,8 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, locations->safe_push (expr.get_location ()); if (++idx < 3 && sizeof_arg != NULL - && expr.original_code == SIZEOF_EXPR) + && (expr.original_code == SIZEOF_EXPR + || expr.original_code == PAREN_SIZEOF_EXPR)) { sizeof_arg[idx] = c_last_sizeof_arg; sizeof_arg_loc[idx] = c_last_sizeof_loc; diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index 287b1df..1f783db 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -675,6 +675,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 dd3e309..459090e 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 95b36a9..48d34f1 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 3c3c51c..edea7ee 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -363,6 +363,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 @@ -5301,6 +5302,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 @@ -8055,6 +8057,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 0000000..84d9a73 --- /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 8311618..e9bad1f 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 0000000..da220cd --- /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 0000000..7962c23 --- /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;) + { + } +} -- 2.7.4