* [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) @ 2017-04-20 21:33 Bernd Edlinger 2017-04-22 6:51 ` Martin Sebor 0 siblings, 1 reply; 13+ messages in thread From: Bernd Edlinger @ 2017-04-20 21:33 UTC (permalink / raw) To: gcc-patches, Joseph Myers, Jason Merrill, Jeff Law, Richard Biener, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 1117 bytes --] Hi! This implements a new -Wall enabled warning for a rather common, but completely wrong way to compute an array size by dividing the sizeof(pointer) / sizeof(pointer[0]) or sizeof(*pointer). It is often hard to find this kind of error by simple code inspection in real code, because using sizeof in this way is a quite common idiom to get the array size of an array variable. And furthermore this expression may be used in macros, which makes it even more important to have this warning. There is a similar warning -Wsizeof-pointer-memaccess which helped in implementing the infrastructure for the new warning in the C FE. However I noticed that the -Wsizeof-pointer-memaccess warning was missing in C, when the sizeof is used inside parentheses, which is different from C++, so I fixed that too. Of course, I added some test cases for that as well. To illustrate the usefulness of this warning, it revealed quite a few places where bogus sizeof divisions were used in our testsuite. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. [-- Attachment #2: changelog-warn-sizeof-pointer.txt --] [-- Type: text/plain, Size: 1978 bytes --] gcc: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * doc/invoke.texi: Document the -Wsizeof-pointer-div warning. gcc/c-family: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c.opt (Wsizeof-pointer-div): New warning option. gcc/c: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-parser.c (c_parser_binary_expression): Implement the -Wsizeof_pointer_div warning. (c_parser_postfix_expression): Allow SIZEOF_EXPR as expr.original_code from a parenthesized expression. (c_parser_expr_list): Use c_last_sizeof_loc. * c-tree.h (c_last_sizeof_loc): New external. * c-typeck.c (c_last_sizeof_loc): New variable. (c_expr_sizeof_expr, c_expr_sizeof_type): Assign c_last_sizeof_loc. gcc/cp: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. gcc/testsuite: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-c++-common/Wsizeof-pointer-div.c: New test. * gcc.dg/Wsizeof-pointer-memaccess1.c: Add test cases with parens. * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Likewise. * gcc.target/i386/sse-init-v4hi-1.c: Fix test case. * gcc.target/i386/sse-init-v4sf-1.c: Likewise. * gcc.target/i386/sse-set-ps-1.c: Likewise. * gcc.target/i386/sse2-init-v16qi-1.c: Likewise. * gcc.target/i386/sse2-init-v2di-1.c: Likewise. * gcc.target/i386/sse2-init-v4si-1.c: Likewise. * gcc.target/i386/sse2-init-v8hi-1.c: Likewise. * gcc.target/i386/sse2-set-epi32-1.c: Likewise. * gcc.target/i386/sse2-set-epi64x-1.c: Likewise. * gcc.target/i386/sse4_1-init-v16qi-1.c: Likewise. * gcc.target/i386/sse4_1-init-v2di-1.c: Likewise. * gcc.target/i386/sse4_1-init-v4sf-1.c: Likewise. * gcc.target/i386/sse4_1-init-v4si-1.c: Likewise. * gcc.target/i386/sse4_1-set-epi32-1.c: Likewise. * gcc.target/i386/sse4_1-set-epi64x-1.c: Likewise. * gcc.target/i386/sse4_1-set-ps-1.c: Likewise. * libgomp.c/pr39591-2.c: Likewise. * libgomp.c/pr39591-3.c: Likewise. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: patch-warn-sizeof-pointer.diff --] [-- Type: text/x-patch; name="patch-warn-sizeof-pointer.diff", Size: 24914 bytes --] Index: gcc/c/c-parser.c =================================================================== --- gcc/c/c-parser.c (revision 246482) +++ gcc/c/c-parser.c (working copy) @@ -6652,6 +6652,8 @@ c_parser_binary_expression (c_parser *parser, stru enum tree_code op; /* The source location of this operation. */ location_t loc; + /* The sizeof argument if expr.original_code == SIZEOF_EXPR. */ + tree sizeof_arg; } stack[NUM_PRECS]; int sp; /* Location of the binary operator. */ @@ -6668,6 +6670,22 @@ c_parser_binary_expression (c_parser *parser, stru 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) \ + { \ + tree type0 = stack[sp - 1].sizeof_arg; \ + tree type1 = stack[sp].sizeof_arg; \ + if (!TYPE_P (type0)) \ + type0 = TREE_TYPE (type0); \ + if (!TYPE_P (type1)) \ + type1 = TREE_TYPE (type1); \ + if (POINTER_TYPE_P (type0) \ + && comptypes (TREE_TYPE (type0), type1)) \ + warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, \ + "dividing the pointer size by the element size"); \ + } \ + break; \ default: \ break; \ } \ @@ -6701,6 +6719,7 @@ c_parser_binary_expression (c_parser *parser, stru stack[0].loc = c_parser_peek_token (parser)->location; stack[0].expr = c_parser_cast_expression (parser, after); stack[0].prec = PREC_NONE; + stack[0].sizeof_arg = c_last_sizeof_arg; sp = 0; while (true) { @@ -6824,6 +6843,7 @@ c_parser_binary_expression (c_parser *parser, stru stack[sp].expr = c_parser_cast_expression (parser, NULL); stack[sp].prec = oprec; stack[sp].op = ocode; + stack[sp].sizeof_arg = c_last_sizeof_arg; } out: while (sp > 0) @@ -7715,7 +7735,8 @@ 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 (expr.original_code != C_MAYBE_CONST_EXPR) + if (expr.original_code != C_MAYBE_CONST_EXPR + && expr.original_code != SIZEOF_EXPR) expr.original_code = ERROR_MARK; /* Don't change EXPR.ORIGINAL_TYPE. */ location_t loc_close_paren = c_parser_peek_token (parser)->location; @@ -8674,7 +8695,6 @@ c_parser_expr_list (c_parser *parser, bool convert vec<tree, va_gc> *orig_types; struct c_expr expr; location_t loc = c_parser_peek_token (parser)->location; - location_t cur_sizeof_arg_loc = UNKNOWN_LOCATION; unsigned int idx = 0; ret = make_tree_vector (); @@ -8683,9 +8703,6 @@ c_parser_expr_list (c_parser *parser, bool convert else orig_types = make_tree_vector (); - if (sizeof_arg != NULL - && c_parser_next_token_is_keyword (parser, RID_SIZEOF)) - cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, 0); expr = c_parser_expr_no_commas (parser, NULL); @@ -8699,21 +8716,15 @@ c_parser_expr_list (c_parser *parser, bool convert if (locations) locations->safe_push (loc); if (sizeof_arg != NULL - && cur_sizeof_arg_loc != UNKNOWN_LOCATION && expr.original_code == SIZEOF_EXPR) { sizeof_arg[0] = c_last_sizeof_arg; - sizeof_arg_loc[0] = cur_sizeof_arg_loc; + sizeof_arg_loc[0] = c_last_sizeof_loc; } while (c_parser_next_token_is (parser, CPP_COMMA)) { c_parser_consume_token (parser); loc = c_parser_peek_token (parser)->location; - if (sizeof_arg != NULL - && c_parser_next_token_is_keyword (parser, RID_SIZEOF)) - cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; - else - cur_sizeof_arg_loc = UNKNOWN_LOCATION; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, idx + 1); expr = c_parser_expr_no_commas (parser, NULL); @@ -8728,11 +8739,10 @@ c_parser_expr_list (c_parser *parser, bool convert locations->safe_push (loc); if (++idx < 3 && sizeof_arg != NULL - && cur_sizeof_arg_loc != UNKNOWN_LOCATION && expr.original_code == SIZEOF_EXPR) { sizeof_arg[idx] = c_last_sizeof_arg; - sizeof_arg_loc[idx] = cur_sizeof_arg_loc; + sizeof_arg_loc[idx] = c_last_sizeof_loc; } } if (orig_types) Index: gcc/c/c-tree.h =================================================================== --- gcc/c/c-tree.h (revision 246482) +++ gcc/c/c-tree.h (working copy) @@ -606,6 +606,7 @@ extern int in_sizeof; extern int in_typeof; extern tree c_last_sizeof_arg; +extern location_t c_last_sizeof_loc; extern struct c_switch *c_switch_stack; Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 246482) +++ gcc/c/c-typeck.c (working copy) @@ -72,6 +72,7 @@ int in_typeof; /* The argument of last parsed sizeof expression, only to be tested if expr.original_code == SIZEOF_EXPR. */ tree c_last_sizeof_arg; +location_t c_last_sizeof_loc; /* Nonzero if we might need to print a "missing braces around initializer" message within this initializer. */ @@ -2909,6 +2910,7 @@ c_expr_sizeof_expr (location_t loc, struct c_expr &expr_const_operands); ret.value = c_sizeof (loc, TREE_TYPE (folded_expr)); c_last_sizeof_arg = expr.value; + c_last_sizeof_loc = loc; ret.original_code = SIZEOF_EXPR; ret.original_type = NULL; if (c_vla_type_p (TREE_TYPE (folded_expr))) @@ -2938,6 +2940,7 @@ c_expr_sizeof_type (location_t loc, struct c_type_ type = groktypename (t, &type_expr, &type_expr_const); ret.value = c_sizeof (loc, type); c_last_sizeof_arg = type; + c_last_sizeof_loc = loc; ret.original_code = SIZEOF_EXPR; ret.original_type = NULL; if ((type_expr || TREE_CODE (ret.value) == INTEGER_CST) Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 246482) +++ gcc/c-family/c.opt (working copy) @@ -698,6 +698,10 @@ Wsized-deallocation C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra) Warn about missing sized deallocation functions. +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-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. Index: gcc/cp/cp-gimplify.c =================================================================== --- gcc/cp/cp-gimplify.c (revision 246482) +++ gcc/cp/cp-gimplify.c (working copy) @@ -2223,6 +2223,22 @@ cp_fold (tree x) else x = fold (x); + if (TREE_CODE (x) == INTEGER_CST && + TREE_CODE (org_x) == TRUNC_DIV_EXPR + && TREE_CODE (TREE_OPERAND (org_x, 0)) == SIZEOF_EXPR + && TREE_CODE (TREE_OPERAND (org_x, 1)) == SIZEOF_EXPR) + { + tree type0 = TREE_OPERAND (TREE_OPERAND (org_x, 0), 0); + tree type1 = TREE_OPERAND (TREE_OPERAND (org_x, 1), 0); + if (!TYPE_P (type0)) + type0 = TREE_TYPE (type0); + if (!TYPE_P (type1)) + type1 = TREE_TYPE (type1); + if (POINTER_TYPE_P (type0) && same_type_p (TREE_TYPE (type0), type1)) + warning_at (loc, OPT_Wsizeof_pointer_div, + "dividing the pointer size by the element size"); + } + if (TREE_NO_WARNING (org_x) && warn_nonnull_compare && COMPARISON_CLASS_P (org_x)) Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 246482) +++ gcc/doc/invoke.texi (working copy) @@ -304,7 +304,7 @@ Objective-C and Objective-C++ Dialects}. -Wshift-overflow -Wshift-overflow=@var{n} @gol -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol -Wsign-compare -Wsign-conversion -Wfloat-conversion @gol --Wno-scalar-storage-order @gol +-Wno-scalar-storage-order -Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess -Wsizeof-array-argument @gol -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol -Wstrict-aliasing=n -Wstrict-overflow -Wstrict-overflow=@var{n} @gol @@ -3777,6 +3777,7 @@ Options} and @ref{Objective-C and Objective-C++ Di -Wreturn-type @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol +-Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess @gol -Wstrict-aliasing @gol -Wstrict-overflow=1 @gol @@ -6022,6 +6023,15 @@ void operator delete[] (void *, std::size_t) noexc or vice versa. Enabled by @option{-Wextra} along with @option{-fsized-deallocation}. +@item -Wsizeof-pointer-div +@opindex Wsizeof-pointer-div +@opindex Wno-sizeof-pointer-div +Warn for suspicious divisions of two sizeof expressions that divide +the pointer size by the element size, which is the usual way to compute +the array size but won't work out correctly with pointers. This warning +warns e.g.@: about @code{sizeof (ptr) / sizeof (ptr[0])} if @code{ptr} is +not an array, but a pointer. This warning is enabled by @option{-Wall}. + @item -Wsizeof-pointer-memaccess @opindex Wsizeof-pointer-memaccess @opindex Wno-sizeof-pointer-memaccess Index: gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c =================================================================== --- gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c (revision 0) +++ gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c (working copy) @@ -0,0 +1,36 @@ +/* Test -Wsizeof-pointer-div warnings. */ +/* { dg-do compile } */ +/* { dg-options "-Wall" } */ + +int +f1 (int *array) +{ + int i; + i = sizeof array / sizeof *array; /* { dg-warning "dividing the pointer size by the element size" } */ + i += sizeof array / sizeof array[0]; /* { dg-warning "dividing the pointer size by the element size" } */ + i += sizeof(array) / sizeof(*array); /* { dg-warning "dividing the pointer size by the element size" } */ + i += sizeof(array) / sizeof(array[0]); /* { dg-warning "dividing the pointer size by the element size" } */ + i += (sizeof(array)) / (sizeof(array[0])); /* { dg-warning "dividing the pointer size by the element size" } */ + i += sizeof(array) / sizeof(int); /* { dg-warning "dividing the pointer size by the element size" } */ + i += sizeof(array) / sizeof(char); + i += sizeof(*array) / sizeof(char); + i += sizeof(array[0]) / sizeof(char); + return i; +} + +int +f2 (void) +{ + int array[10]; + int i; + i = sizeof array / sizeof *array; + i += sizeof array / sizeof array[0]; + i += sizeof(array) / sizeof(*array); + 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); + i += sizeof(array[0]) / sizeof(char); + return i; +} Index: gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c =================================================================== --- gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c (revision 246482) +++ gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c (working copy) @@ -73,6 +73,15 @@ f1 (void *x, int z) z += bcmp (x, pa2, sizeof (PTA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ z += bcmp (x, pa3, sizeof (PA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (&a), (sizeof (&a))); /* { dg-warning "call is the same expression as the second source; did you mean to remove the addressof" } */ + z += bcmp (x, (pa1), (sizeof (pa1))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa2), (sizeof pa2)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa3), (sizeof (pa3))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa4), (sizeof pa4)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa1), (sizeof (struct A *)));/* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (pa2), (sizeof (PTA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (pa3), (sizeof (PA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + /* These are correct, no warning. */ bzero (&a, sizeof a); bzero (&a, sizeof (a)); Index: gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c =================================================================== --- gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c (revision 246482) +++ gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c (working copy) @@ -150,6 +150,15 @@ f1 (void *x, int z) z += memcmp (x, pa2, sizeof (PTA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ z += memcmp (x, pa3, sizeof (PA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (&a), (sizeof (&a))); /* { dg-warning "call is the same expression as the second source; did you mean to remove the addressof" } */ + z += memcmp (x, (pa1), (sizeof (pa1))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa2), (sizeof pa2)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa3), (sizeof (pa3))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa4), (sizeof pa4)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa1), (sizeof (struct A *)));/* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (pa2), (sizeof (PTA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (pa3), (sizeof (PA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + /* These are correct, no warning. */ memset (&a, 0, sizeof a); memset (&a, 0, sizeof (a)); Index: gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c (working copy) @@ -17,13 +17,13 @@ check (__m64 x, unsigned short *v, int j) union { __m64 x; - unsigned short i[8]; + unsigned short i[4]; } u; unsigned int i; u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128 x, float *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.f[i]) Index: gcc/testsuite/gcc.target/i386/sse-set-ps-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-set-ps-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-set-ps-1.c (working copy) @@ -23,7 +23,7 @@ test (float *v) u.x = _mm_set_ps (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.f[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned char *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned long long *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned int *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned short *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned int *v) u.x = _mm_set_epi32 (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned long long *v) u.x = _mm_set_epi64x (v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned char *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned long long *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128 x, float *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.f[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned int *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned int *v) u.x = _mm_set_epi32 (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned long long *v) u.x = _mm_set_epi64x (v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c (working copy) @@ -23,7 +23,7 @@ test (float *v) u.x = _mm_set_ps (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.f[i]) { #ifdef DEBUG Index: libgomp/testsuite/libgomp.c/pr39591-2.c =================================================================== --- libgomp/testsuite/libgomp.c/pr39591-2.c (revision 246482) +++ libgomp/testsuite/libgomp.c/pr39591-2.c (working copy) @@ -11,7 +11,7 @@ foo (int *array) #pragma omp task { int j; - for (j = 0; j < sizeof array / sizeof array[0]; j++) + for (j = 0; j < 40; j++) if (array[j] != 0x55555555) #pragma omp atomic err++; Index: libgomp/testsuite/libgomp.c/pr39591-3.c =================================================================== --- libgomp/testsuite/libgomp.c/pr39591-3.c (revision 246482) +++ libgomp/testsuite/libgomp.c/pr39591-3.c (working copy) @@ -11,7 +11,7 @@ foo (int *array) #pragma omp task { int j; - for (j = 0; j < sizeof array / sizeof array[0]; j++) + for (j = 0; j < 40; j++) if (array[j] != 0x55555555) #pragma omp atomic err++; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-04-20 21:33 [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) Bernd Edlinger @ 2017-04-22 6:51 ` Martin Sebor 2017-04-22 17:50 ` Bernd Edlinger 0 siblings, 1 reply; 13+ messages in thread From: Martin Sebor @ 2017-04-22 6:51 UTC (permalink / raw) To: Bernd Edlinger, gcc-patches, Joseph Myers, Jason Merrill, Jeff Law, Richard Biener, Jakub Jelinek On 04/20/2017 02:35 PM, Bernd Edlinger wrote: > Hi! > > > This implements a new -Wall enabled warning for a rather common, but > completely wrong way to compute an array size by dividing the > sizeof(pointer) / sizeof(pointer[0]) or sizeof(*pointer). > > It is often hard to find this kind of error by simple code inspection > in real code, because using sizeof in this way is a quite common idiom > to get the array size of an array variable. And furthermore this > expression may be used in macros, which makes it even more important to > have this warning. > > There is a similar warning -Wsizeof-pointer-memaccess which helped in > implementing the infrastructure for the new warning in the C FE. > > However I noticed that the -Wsizeof-pointer-memaccess warning was > missing in C, when the sizeof is used inside parentheses, which is > different from C++, so I fixed that too. > > Of course, I added some test cases for that as well. > > To illustrate the usefulness of this warning, it revealed quite a few > places where bogus sizeof divisions were used in our testsuite. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? That seems like a useful warning. Just a few comments. First, -Wsizeof-array-argument already diagnoses a subset of the same problems. For example, with the patch applied, GCC issues the two warnings below for following test case. One should be sufficient. $ cat y.c && gcc -S -Wall y.c int f (int a[]) { return sizeof a / sizeof *a; } y.c: In function âfâ: y.c:3:17: warning: âsizeofâ on array function parameter âaâ will return size of âint *â [-Wsizeof-array-argument] return sizeof a / sizeof *a; ^ y.c:1:12: note: declared here int f (int a[]) ^ y.c:3:19: warning: dividing the pointer size by the element size [-Wsizeof-pointer-div] return sizeof a / sizeof *a; ^ Second, I would suggest mentioning the actual types of the operands rather than referring to "pointer size" and "element size." Maybe something like: division 'sizeof (int*) / sizeof (int)' does not compute the number of array elements I suggest avoiding "element size" because the pointed-to argument need not be an array. Mentioning the types should help users better understand the problem (especially in C++ where types are often obscured by layers of templates). It might also be a nice touch to add a note pointing to the declaration of the first sizeof operand (if it's an object). Martin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-04-22 6:51 ` Martin Sebor @ 2017-04-22 17:50 ` Bernd Edlinger 2017-04-28 3:23 ` Martin Sebor 0 siblings, 1 reply; 13+ messages in thread From: Bernd Edlinger @ 2017-04-22 17:50 UTC (permalink / raw) To: Martin Sebor, gcc-patches, Joseph Myers, Jason Merrill, Jeff Law, Richard Biener, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 2919 bytes --] On 04/22/17 01:50, Martin Sebor wrote: > On 04/20/2017 02:35 PM, Bernd Edlinger wrote: >> Hi! >> >> >> This implements a new -Wall enabled warning for a rather common, but >> completely wrong way to compute an array size by dividing the >> sizeof(pointer) / sizeof(pointer[0]) or sizeof(*pointer). >> >> It is often hard to find this kind of error by simple code inspection >> in real code, because using sizeof in this way is a quite common idiom >> to get the array size of an array variable. And furthermore this >> expression may be used in macros, which makes it even more important to >> have this warning. >> >> There is a similar warning -Wsizeof-pointer-memaccess which helped in >> implementing the infrastructure for the new warning in the C FE. >> >> However I noticed that the -Wsizeof-pointer-memaccess warning was >> missing in C, when the sizeof is used inside parentheses, which is >> different from C++, so I fixed that too. >> >> Of course, I added some test cases for that as well. >> >> To illustrate the usefulness of this warning, it revealed quite a few >> places where bogus sizeof divisions were used in our testsuite. >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? > > That seems like a useful warning. Just a few comments. > > First, -Wsizeof-array-argument already diagnoses a subset of > the same problems. For example, with the patch applied, GCC > issues the two warnings below for following test case. One > should be sufficient. > > $ cat y.c && gcc -S -Wall y.c > int f (int a[]) > { > return sizeof a / sizeof *a; > } > y.c: In function ‘f’: > y.c:3:17: warning: ‘sizeof’ on array function parameter ‘a’ will > return size of ‘int *’ [-Wsizeof-array-argument] > return sizeof a / sizeof *a; > ^ > y.c:1:12: note: declared here > int f (int a[]) > ^ > y.c:3:19: warning: dividing the pointer size by the element size > [-Wsizeof-pointer-div] > return sizeof a / sizeof *a; > ^ > > Second, I would suggest mentioning the actual types of the operands > rather than referring to "pointer size" and "element size." Maybe > something like: > > division 'sizeof (int*) / sizeof (int)' does not compute the number of > array elements > > I suggest avoiding "element size" because the pointed-to argument > need not be an array. Mentioning the types should help users better > understand the problem (especially in C++ where types are often > obscured by layers of templates). It might also be a nice touch > to add a note pointing to the declaration of the first sizeof > operand (if it's an object). > Yes, many thanks for your suggestions. Do the new warning and info messages look right? Is it OK for trunk after bootstrap / reg-testing? Thanks Bernd. [-- Attachment #2: changelog-warn-sizeof-pointer.txt --] [-- Type: text/plain, Size: 1978 bytes --] gcc: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * doc/invoke.texi: Document the -Wsizeof-pointer-div warning. gcc/c-family: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c.opt (Wsizeof-pointer-div): New warning option. gcc/c: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-parser.c (c_parser_binary_expression): Implement the -Wsizeof_pointer_div warning. (c_parser_postfix_expression): Allow SIZEOF_EXPR as expr.original_code from a parenthesized expression. (c_parser_expr_list): Use c_last_sizeof_loc. * c-tree.h (c_last_sizeof_loc): New external. * c-typeck.c (c_last_sizeof_loc): New variable. (c_expr_sizeof_expr, c_expr_sizeof_type): Assign c_last_sizeof_loc. gcc/cp: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. gcc/testsuite: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-c++-common/Wsizeof-pointer-div.c: New test. * gcc.dg/Wsizeof-pointer-memaccess1.c: Add test cases with parens. * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Likewise. * gcc.target/i386/sse-init-v4hi-1.c: Fix test case. * gcc.target/i386/sse-init-v4sf-1.c: Likewise. * gcc.target/i386/sse-set-ps-1.c: Likewise. * gcc.target/i386/sse2-init-v16qi-1.c: Likewise. * gcc.target/i386/sse2-init-v2di-1.c: Likewise. * gcc.target/i386/sse2-init-v4si-1.c: Likewise. * gcc.target/i386/sse2-init-v8hi-1.c: Likewise. * gcc.target/i386/sse2-set-epi32-1.c: Likewise. * gcc.target/i386/sse2-set-epi64x-1.c: Likewise. * gcc.target/i386/sse4_1-init-v16qi-1.c: Likewise. * gcc.target/i386/sse4_1-init-v2di-1.c: Likewise. * gcc.target/i386/sse4_1-init-v4sf-1.c: Likewise. * gcc.target/i386/sse4_1-init-v4si-1.c: Likewise. * gcc.target/i386/sse4_1-set-epi32-1.c: Likewise. * gcc.target/i386/sse4_1-set-epi64x-1.c: Likewise. * gcc.target/i386/sse4_1-set-ps-1.c: Likewise. * libgomp.c/pr39591-2.c: Likewise. * libgomp.c/pr39591-3.c: Likewise. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: patch-warn-sizeof-pointer.diff --] [-- Type: text/x-patch; name="patch-warn-sizeof-pointer.diff", Size: 25852 bytes --] Index: gcc/c/c-parser.c =================================================================== --- gcc/c/c-parser.c (revision 246482) +++ gcc/c/c-parser.c (working copy) @@ -6652,6 +6652,8 @@ c_parser_binary_expression (c_parser *parser, stru enum tree_code op; /* The source location of this operation. */ location_t loc; + /* The sizeof argument if expr.original_code == SIZEOF_EXPR. */ + tree sizeof_arg; } stack[NUM_PRECS]; int sp; /* Location of the binary operator. */ @@ -6668,6 +6670,31 @@ c_parser_binary_expression (c_parser *parser, stru 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) \ + { \ + tree type0 = stack[sp - 1].sizeof_arg; \ + tree type1 = stack[sp].sizeof_arg; \ + tree first_arg = type0; \ + if (!TYPE_P (type0)) \ + type0 = TREE_TYPE (type0); \ + if (!TYPE_P (type1)) \ + type1 = TREE_TYPE (type1); \ + if (POINTER_TYPE_P (type0) \ + && comptypes (TREE_TYPE (type0), type1) \ + && !(TREE_CODE (first_arg) == PARM_DECL \ + && C_ARRAY_PARAMETER (first_arg) \ + && warn_sizeof_array_argument)) \ + if (warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, \ + "division 'sizeof (%qT) / sizeof (%qT)' does " \ + "not compute the number of array elements", \ + type0, type1)) \ + if (DECL_P (first_arg)) \ + inform (DECL_SOURCE_LOCATION (first_arg), \ + "first sizeof operand was declared here"); \ + } \ + break; \ default: \ break; \ } \ @@ -6701,6 +6728,7 @@ c_parser_binary_expression (c_parser *parser, stru stack[0].loc = c_parser_peek_token (parser)->location; stack[0].expr = c_parser_cast_expression (parser, after); stack[0].prec = PREC_NONE; + stack[0].sizeof_arg = c_last_sizeof_arg; sp = 0; while (true) { @@ -6824,6 +6852,7 @@ c_parser_binary_expression (c_parser *parser, stru stack[sp].expr = c_parser_cast_expression (parser, NULL); stack[sp].prec = oprec; stack[sp].op = ocode; + stack[sp].sizeof_arg = c_last_sizeof_arg; } out: while (sp > 0) @@ -7715,7 +7744,8 @@ 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 (expr.original_code != C_MAYBE_CONST_EXPR) + if (expr.original_code != C_MAYBE_CONST_EXPR + && expr.original_code != SIZEOF_EXPR) expr.original_code = ERROR_MARK; /* Don't change EXPR.ORIGINAL_TYPE. */ location_t loc_close_paren = c_parser_peek_token (parser)->location; @@ -8674,7 +8704,6 @@ c_parser_expr_list (c_parser *parser, bool convert vec<tree, va_gc> *orig_types; struct c_expr expr; location_t loc = c_parser_peek_token (parser)->location; - location_t cur_sizeof_arg_loc = UNKNOWN_LOCATION; unsigned int idx = 0; ret = make_tree_vector (); @@ -8683,9 +8712,6 @@ c_parser_expr_list (c_parser *parser, bool convert else orig_types = make_tree_vector (); - if (sizeof_arg != NULL - && c_parser_next_token_is_keyword (parser, RID_SIZEOF)) - cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, 0); expr = c_parser_expr_no_commas (parser, NULL); @@ -8699,21 +8725,15 @@ c_parser_expr_list (c_parser *parser, bool convert if (locations) locations->safe_push (loc); if (sizeof_arg != NULL - && cur_sizeof_arg_loc != UNKNOWN_LOCATION && expr.original_code == SIZEOF_EXPR) { sizeof_arg[0] = c_last_sizeof_arg; - sizeof_arg_loc[0] = cur_sizeof_arg_loc; + sizeof_arg_loc[0] = c_last_sizeof_loc; } while (c_parser_next_token_is (parser, CPP_COMMA)) { c_parser_consume_token (parser); loc = c_parser_peek_token (parser)->location; - if (sizeof_arg != NULL - && c_parser_next_token_is_keyword (parser, RID_SIZEOF)) - cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; - else - cur_sizeof_arg_loc = UNKNOWN_LOCATION; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, idx + 1); expr = c_parser_expr_no_commas (parser, NULL); @@ -8728,11 +8748,10 @@ c_parser_expr_list (c_parser *parser, bool convert locations->safe_push (loc); if (++idx < 3 && sizeof_arg != NULL - && cur_sizeof_arg_loc != UNKNOWN_LOCATION && expr.original_code == SIZEOF_EXPR) { sizeof_arg[idx] = c_last_sizeof_arg; - sizeof_arg_loc[idx] = cur_sizeof_arg_loc; + sizeof_arg_loc[idx] = c_last_sizeof_loc; } } if (orig_types) Index: gcc/c/c-tree.h =================================================================== --- gcc/c/c-tree.h (revision 246482) +++ gcc/c/c-tree.h (working copy) @@ -606,6 +606,7 @@ extern int in_sizeof; extern int in_typeof; extern tree c_last_sizeof_arg; +extern location_t c_last_sizeof_loc; extern struct c_switch *c_switch_stack; Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 246482) +++ gcc/c/c-typeck.c (working copy) @@ -72,6 +72,7 @@ int in_typeof; /* The argument of last parsed sizeof expression, only to be tested if expr.original_code == SIZEOF_EXPR. */ tree c_last_sizeof_arg; +location_t c_last_sizeof_loc; /* Nonzero if we might need to print a "missing braces around initializer" message within this initializer. */ @@ -2909,6 +2910,7 @@ c_expr_sizeof_expr (location_t loc, struct c_expr &expr_const_operands); ret.value = c_sizeof (loc, TREE_TYPE (folded_expr)); c_last_sizeof_arg = expr.value; + c_last_sizeof_loc = loc; ret.original_code = SIZEOF_EXPR; ret.original_type = NULL; if (c_vla_type_p (TREE_TYPE (folded_expr))) @@ -2938,6 +2940,7 @@ c_expr_sizeof_type (location_t loc, struct c_type_ type = groktypename (t, &type_expr, &type_expr_const); ret.value = c_sizeof (loc, type); c_last_sizeof_arg = type; + c_last_sizeof_loc = loc; ret.original_code = SIZEOF_EXPR; ret.original_type = NULL; if ((type_expr || TREE_CODE (ret.value) == INTEGER_CST) Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 246482) +++ gcc/c-family/c.opt (working copy) @@ -698,6 +698,10 @@ Wsized-deallocation C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra) Warn about missing sized deallocation functions. +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-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. Index: gcc/cp/cp-gimplify.c =================================================================== --- gcc/cp/cp-gimplify.c (revision 246482) +++ gcc/cp/cp-gimplify.c (working copy) @@ -2223,6 +2223,22 @@ cp_fold (tree x) else x = fold (x); + if (TREE_CODE (x) == INTEGER_CST && + TREE_CODE (org_x) == TRUNC_DIV_EXPR + && TREE_CODE (TREE_OPERAND (org_x, 0)) == SIZEOF_EXPR + && TREE_CODE (TREE_OPERAND (org_x, 1)) == SIZEOF_EXPR) + { + tree type0 = TREE_OPERAND (TREE_OPERAND (org_x, 0), 0); + tree type1 = TREE_OPERAND (TREE_OPERAND (org_x, 1), 0); + tree first_arg = type0; + if (!TYPE_P (type0)) + type0 = TREE_TYPE (type0); + if (!TYPE_P (type1)) + type1 = TREE_TYPE (type1); + if (POINTER_TYPE_P (type0) && same_type_p (TREE_TYPE (type0), type1) + && !(TREE_CODE (first_arg) == PARM_DECL + && DECL_ARRAY_PARAMETER_P (first_arg) + && warn_sizeof_array_argument)) + if (warning_at (loc, OPT_Wsizeof_pointer_div, + "division 'sizeof (%qT) / sizeof (%qT)' does " + "not compute the number of array elements", + type0, type1)) + if (DECL_P (first_arg)) + inform (DECL_SOURCE_LOCATION (first_arg), + "first sizeof operand was declared here"); + } + if (TREE_NO_WARNING (org_x) && warn_nonnull_compare && COMPARISON_CLASS_P (org_x)) Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 246482) +++ gcc/doc/invoke.texi (working copy) @@ -304,7 +304,7 @@ Objective-C and Objective-C++ Dialects}. -Wshift-overflow -Wshift-overflow=@var{n} @gol -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol -Wsign-compare -Wsign-conversion -Wfloat-conversion @gol --Wno-scalar-storage-order @gol +-Wno-scalar-storage-order -Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess -Wsizeof-array-argument @gol -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol -Wstrict-aliasing=n -Wstrict-overflow -Wstrict-overflow=@var{n} @gol @@ -3777,6 +3777,7 @@ Options} and @ref{Objective-C and Objective-C++ Di -Wreturn-type @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol +-Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess @gol -Wstrict-aliasing @gol -Wstrict-overflow=1 @gol @@ -6022,6 +6023,15 @@ void operator delete[] (void *, std::size_t) noexc or vice versa. Enabled by @option{-Wextra} along with @option{-fsized-deallocation}. +@item -Wsizeof-pointer-div +@opindex Wsizeof-pointer-div +@opindex Wno-sizeof-pointer-div +Warn for suspicious divisions of two sizeof expressions that divide +the pointer size by the element size, which is the usual way to compute +the array size but won't work out correctly with pointers. This warning +warns e.g.@: about @code{sizeof (ptr) / sizeof (ptr[0])} if @code{ptr} is +not an array, but a pointer. This warning is enabled by @option{-Wall}. + @item -Wsizeof-pointer-memaccess @opindex Wsizeof-pointer-memaccess @opindex Wno-sizeof-pointer-memaccess Index: gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c =================================================================== --- gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c (revision 0) +++ gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c (working copy) @@ -0,0 +1,41 @@ +/* Test -Wsizeof-pointer-div warnings. */ +/* { dg-do compile } */ +/* { dg-options "-Wall" } */ + +int +f1 (int *array) +{ + int i; + i = sizeof array / sizeof *array; /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof array / sizeof array[0]; /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(*array); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(array[0]); /* { dg-warning "does not compute the number of array elements" } */ + i += (sizeof(array)) / (sizeof(array[0])); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(int); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(char); + i += sizeof(*array) / sizeof(char); + i += sizeof(array[0]) / sizeof(char); + return i; +} + +int +f2 (void) +{ + int array[10]; + int i; + i = sizeof array / sizeof *array; + i += sizeof array / sizeof array[0]; + i += sizeof(array) / sizeof(*array); + 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); + i += sizeof(array[0]) / sizeof(char); + return i; +} + +int f3 (int a[]) +{ + return sizeof a / sizeof *a; /* { dg-warning "Wsizeof-array-argument" } */ +} Index: gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c =================================================================== --- gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c (revision 246482) +++ gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c (working copy) @@ -73,6 +73,15 @@ f1 (void *x, int z) z += bcmp (x, pa2, sizeof (PTA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ z += bcmp (x, pa3, sizeof (PA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (&a), (sizeof (&a))); /* { dg-warning "call is the same expression as the second source; did you mean to remove the addressof" } */ + z += bcmp (x, (pa1), (sizeof (pa1))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa2), (sizeof pa2)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa3), (sizeof (pa3))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa4), (sizeof pa4)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa1), (sizeof (struct A *)));/* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (pa2), (sizeof (PTA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (pa3), (sizeof (PA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + /* These are correct, no warning. */ bzero (&a, sizeof a); bzero (&a, sizeof (a)); Index: gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c =================================================================== --- gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c (revision 246482) +++ gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c (working copy) @@ -150,6 +150,15 @@ f1 (void *x, int z) z += memcmp (x, pa2, sizeof (PTA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ z += memcmp (x, pa3, sizeof (PA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (&a), (sizeof (&a))); /* { dg-warning "call is the same expression as the second source; did you mean to remove the addressof" } */ + z += memcmp (x, (pa1), (sizeof (pa1))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa2), (sizeof pa2)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa3), (sizeof (pa3))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa4), (sizeof pa4)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa1), (sizeof (struct A *)));/* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (pa2), (sizeof (PTA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (pa3), (sizeof (PA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + /* These are correct, no warning. */ memset (&a, 0, sizeof a); memset (&a, 0, sizeof (a)); Index: gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c (working copy) @@ -17,13 +17,13 @@ check (__m64 x, unsigned short *v, int j) union { __m64 x; - unsigned short i[8]; + unsigned short i[4]; } u; unsigned int i; u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128 x, float *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.f[i]) Index: gcc/testsuite/gcc.target/i386/sse-set-ps-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-set-ps-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-set-ps-1.c (working copy) @@ -23,7 +23,7 @@ test (float *v) u.x = _mm_set_ps (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.f[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned char *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned long long *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned int *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned short *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned int *v) u.x = _mm_set_epi32 (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned long long *v) u.x = _mm_set_epi64x (v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned char *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned long long *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128 x, float *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.f[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned int *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned int *v) u.x = _mm_set_epi32 (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned long long *v) u.x = _mm_set_epi64x (v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c (working copy) @@ -23,7 +23,7 @@ test (float *v) u.x = _mm_set_ps (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.f[i]) { #ifdef DEBUG Index: libgomp/testsuite/libgomp.c/pr39591-2.c =================================================================== --- libgomp/testsuite/libgomp.c/pr39591-2.c (revision 246482) +++ libgomp/testsuite/libgomp.c/pr39591-2.c (working copy) @@ -11,7 +11,7 @@ foo (int *array) #pragma omp task { int j; - for (j = 0; j < sizeof array / sizeof array[0]; j++) + for (j = 0; j < 40; j++) if (array[j] != 0x55555555) #pragma omp atomic err++; Index: libgomp/testsuite/libgomp.c/pr39591-3.c =================================================================== --- libgomp/testsuite/libgomp.c/pr39591-3.c (revision 246482) +++ libgomp/testsuite/libgomp.c/pr39591-3.c (working copy) @@ -11,7 +11,7 @@ foo (int *array) #pragma omp task { int j; - for (j = 0; j < sizeof array / sizeof array[0]; j++) + for (j = 0; j < 40; j++) if (array[j] != 0x55555555) #pragma omp atomic err++; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-04-22 17:50 ` Bernd Edlinger @ 2017-04-28 3:23 ` Martin Sebor 2017-04-28 14:42 ` Bernd Edlinger 0 siblings, 1 reply; 13+ messages in thread From: Martin Sebor @ 2017-04-28 3:23 UTC (permalink / raw) To: Bernd Edlinger, gcc-patches, Joseph Myers, Jason Merrill, Jeff Law, Richard Biener, Jakub Jelinek >> I suggest avoiding "element size" because the pointed-to argument >> need not be an array. Mentioning the types should help users better >> understand the problem (especially in C++ where types are often >> obscured by layers of templates). It might also be a nice touch >> to add a note pointing to the declaration of the first sizeof >> operand (if it's an object). >> > > Yes, many thanks for your suggestions. > > Do the new warning and info messages look right? Sorry for not replying sooner. The new warning looks good to me, with just one minor nit. For proper highlighting within the message, the expression should be quoted in a pair of %< and %> directives, like so: warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, "division %<sizeof (%T) / sizeof (%T)%> does " "not compute the number of array elements", type0, type1)) This way the whole quoted expression will be highlighted in the style appropriate for quoted text. Similarly, in the inform call, sizeof should be quoted in the same pair of %< and %> directives. Martin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-04-28 3:23 ` Martin Sebor @ 2017-04-28 14:42 ` Bernd Edlinger 2017-04-28 16:27 ` Martin Sebor 0 siblings, 1 reply; 13+ messages in thread From: Bernd Edlinger @ 2017-04-28 14:42 UTC (permalink / raw) To: Martin Sebor, gcc-patches, Joseph Myers, Jason Merrill, Jeff Law, Richard Biener, Jakub Jelinek On 04/28/17 00:52, Martin Sebor wrote: >>> I suggest avoiding "element size" because the pointed-to argument >>> need not be an array. Mentioning the types should help users better >>> understand the problem (especially in C++ where types are often >>> obscured by layers of templates). It might also be a nice touch >>> to add a note pointing to the declaration of the first sizeof >>> operand (if it's an object). >>> >> >> Yes, many thanks for your suggestions. >> >> Do the new warning and info messages look right? > > Sorry for not replying sooner. The new warning looks good to > me, with just one minor nit. For proper highlighting within > the message, the expression should be quoted in a pair of %< > and %> directives, like so: > > warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, > "division %<sizeof (%T) / sizeof (%T)%> does " > "not compute the number of array elements", > type0, type1)) > > This way the whole quoted expression will be highlighted in > the style appropriate for quoted text. > > Similarly, in the inform call, sizeof should be quoted in > the same pair of %< and %> directives. > Do you want me to change the %qT format strings to %T ? Bernd. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-04-28 14:42 ` Bernd Edlinger @ 2017-04-28 16:27 ` Martin Sebor 2017-04-28 17:20 ` Bernd Edlinger 0 siblings, 1 reply; 13+ messages in thread From: Martin Sebor @ 2017-04-28 16:27 UTC (permalink / raw) To: Bernd Edlinger, gcc-patches, Joseph Myers, Jason Merrill, Jeff Law, Richard Biener, Jakub Jelinek On 04/28/2017 08:12 AM, Bernd Edlinger wrote: > On 04/28/17 00:52, Martin Sebor wrote: >>>> I suggest avoiding "element size" because the pointed-to argument >>>> need not be an array. Mentioning the types should help users better >>>> understand the problem (especially in C++ where types are often >>>> obscured by layers of templates). It might also be a nice touch >>>> to add a note pointing to the declaration of the first sizeof >>>> operand (if it's an object). >>>> >>> >>> Yes, many thanks for your suggestions. >>> >>> Do the new warning and info messages look right? >> >> Sorry for not replying sooner. The new warning looks good to >> me, with just one minor nit. For proper highlighting within >> the message, the expression should be quoted in a pair of %< >> and %> directives, like so: >> >> warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, >> "division %<sizeof (%T) / sizeof (%T)%> does " >> "not compute the number of array elements", >> type0, type1)) >> >> This way the whole quoted expression will be highlighted in >> the style appropriate for quoted text. >> >> Similarly, in the inform call, sizeof should be quoted in >> the same pair of %< and %> directives. >> > > Do you want me to change the %qT format strings to %T ? Yes, with the surrounding %< and %> the nested directives should use the unquoted forms, otherwise the printer would end up quoting both the whole expression and the type operand. FWIW, to help avoid this mistake, I think this might be something for GCC -Wformat to warn on and the pretty-printer to detect (and ICE on). Martin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-04-28 16:27 ` Martin Sebor @ 2017-04-28 17:20 ` Bernd Edlinger 2017-05-01 15:54 ` Jason Merrill 0 siblings, 1 reply; 13+ messages in thread From: Bernd Edlinger @ 2017-04-28 17:20 UTC (permalink / raw) To: Martin Sebor, gcc-patches, Joseph Myers, Jason Merrill, Jeff Law, Richard Biener, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 715 bytes --] On 04/28/17 17:29, Martin Sebor wrote: > On 04/28/2017 08:12 AM, Bernd Edlinger wrote: >> >> Do you want me to change the %qT format strings to %T ? > > Yes, with the surrounding %< and %> the nested directives should > use the unquoted forms, otherwise the printer would end up quoting > both the whole expression and the type operand. > > FWIW, to help avoid this mistake, I think this might be something > for GCC -Wformat to warn on and the pretty-printer to detect (and > ICE on). > Ah, now I understand. That's pretty advanced. Here is the modified patch with correct quoting of the expression. Bootstrap and reg-testing on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. [-- Attachment #2: changelog-warn-sizeof-pointer.txt --] [-- Type: text/plain, Size: 1978 bytes --] gcc: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * doc/invoke.texi: Document the -Wsizeof-pointer-div warning. gcc/c-family: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c.opt (Wsizeof-pointer-div): New warning option. gcc/c: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-parser.c (c_parser_binary_expression): Implement the -Wsizeof_pointer_div warning. (c_parser_postfix_expression): Allow SIZEOF_EXPR as expr.original_code from a parenthesized expression. (c_parser_expr_list): Use c_last_sizeof_loc. * c-tree.h (c_last_sizeof_loc): New external. * c-typeck.c (c_last_sizeof_loc): New variable. (c_expr_sizeof_expr, c_expr_sizeof_type): Assign c_last_sizeof_loc. gcc/cp: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. gcc/testsuite: 2017-04-20 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-c++-common/Wsizeof-pointer-div.c: New test. * gcc.dg/Wsizeof-pointer-memaccess1.c: Add test cases with parens. * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Likewise. * gcc.target/i386/sse-init-v4hi-1.c: Fix test case. * gcc.target/i386/sse-init-v4sf-1.c: Likewise. * gcc.target/i386/sse-set-ps-1.c: Likewise. * gcc.target/i386/sse2-init-v16qi-1.c: Likewise. * gcc.target/i386/sse2-init-v2di-1.c: Likewise. * gcc.target/i386/sse2-init-v4si-1.c: Likewise. * gcc.target/i386/sse2-init-v8hi-1.c: Likewise. * gcc.target/i386/sse2-set-epi32-1.c: Likewise. * gcc.target/i386/sse2-set-epi64x-1.c: Likewise. * gcc.target/i386/sse4_1-init-v16qi-1.c: Likewise. * gcc.target/i386/sse4_1-init-v2di-1.c: Likewise. * gcc.target/i386/sse4_1-init-v4sf-1.c: Likewise. * gcc.target/i386/sse4_1-init-v4si-1.c: Likewise. * gcc.target/i386/sse4_1-set-epi32-1.c: Likewise. * gcc.target/i386/sse4_1-set-epi64x-1.c: Likewise. * gcc.target/i386/sse4_1-set-ps-1.c: Likewise. * libgomp.c/pr39591-2.c: Likewise. * libgomp.c/pr39591-3.c: Likewise. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: patch-warn-sizeof-pointer.diff --] [-- Type: text/x-patch; name="patch-warn-sizeof-pointer.diff", Size: 25859 bytes --] Index: gcc/c/c-parser.c =================================================================== --- gcc/c/c-parser.c (revision 246482) +++ gcc/c/c-parser.c (working copy) @@ -6652,6 +6652,8 @@ c_parser_binary_expression (c_parser *parser, stru enum tree_code op; /* The source location of this operation. */ location_t loc; + /* The sizeof argument if expr.original_code == SIZEOF_EXPR. */ + tree sizeof_arg; } stack[NUM_PRECS]; int sp; /* Location of the binary operator. */ @@ -6668,6 +6670,31 @@ c_parser_binary_expression (c_parser *parser, stru 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) \ + { \ + tree type0 = stack[sp - 1].sizeof_arg; \ + tree type1 = stack[sp].sizeof_arg; \ + tree first_arg = type0; \ + if (!TYPE_P (type0)) \ + type0 = TREE_TYPE (type0); \ + if (!TYPE_P (type1)) \ + type1 = TREE_TYPE (type1); \ + if (POINTER_TYPE_P (type0) \ + && comptypes (TREE_TYPE (type0), type1) \ + && !(TREE_CODE (first_arg) == PARM_DECL \ + && C_ARRAY_PARAMETER (first_arg) \ + && warn_sizeof_array_argument)) \ + if (warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, \ + "division %<sizeof (%T) / sizeof (%T)%> does " \ + "not compute the number of array elements", \ + type0, type1)) \ + if (DECL_P (first_arg)) \ + inform (DECL_SOURCE_LOCATION (first_arg), \ + "first %<sizeof%> operand was declared here"); \ + } \ + break; \ default: \ break; \ } \ @@ -6701,6 +6728,7 @@ c_parser_binary_expression (c_parser *parser, stru stack[0].loc = c_parser_peek_token (parser)->location; stack[0].expr = c_parser_cast_expression (parser, after); stack[0].prec = PREC_NONE; + stack[0].sizeof_arg = c_last_sizeof_arg; sp = 0; while (true) { @@ -6824,6 +6852,7 @@ c_parser_binary_expression (c_parser *parser, stru stack[sp].expr = c_parser_cast_expression (parser, NULL); stack[sp].prec = oprec; stack[sp].op = ocode; + stack[sp].sizeof_arg = c_last_sizeof_arg; } out: while (sp > 0) @@ -7715,7 +7744,8 @@ 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 (expr.original_code != C_MAYBE_CONST_EXPR) + if (expr.original_code != C_MAYBE_CONST_EXPR + && expr.original_code != SIZEOF_EXPR) expr.original_code = ERROR_MARK; /* Don't change EXPR.ORIGINAL_TYPE. */ location_t loc_close_paren = c_parser_peek_token (parser)->location; @@ -8674,7 +8704,6 @@ c_parser_expr_list (c_parser *parser, bool convert vec<tree, va_gc> *orig_types; struct c_expr expr; location_t loc = c_parser_peek_token (parser)->location; - location_t cur_sizeof_arg_loc = UNKNOWN_LOCATION; unsigned int idx = 0; ret = make_tree_vector (); @@ -8683,9 +8712,6 @@ c_parser_expr_list (c_parser *parser, bool convert else orig_types = make_tree_vector (); - if (sizeof_arg != NULL - && c_parser_next_token_is_keyword (parser, RID_SIZEOF)) - cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, 0); expr = c_parser_expr_no_commas (parser, NULL); @@ -8699,21 +8725,15 @@ c_parser_expr_list (c_parser *parser, bool convert if (locations) locations->safe_push (loc); if (sizeof_arg != NULL - && cur_sizeof_arg_loc != UNKNOWN_LOCATION && expr.original_code == SIZEOF_EXPR) { sizeof_arg[0] = c_last_sizeof_arg; - sizeof_arg_loc[0] = cur_sizeof_arg_loc; + sizeof_arg_loc[0] = c_last_sizeof_loc; } while (c_parser_next_token_is (parser, CPP_COMMA)) { c_parser_consume_token (parser); loc = c_parser_peek_token (parser)->location; - if (sizeof_arg != NULL - && c_parser_next_token_is_keyword (parser, RID_SIZEOF)) - cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; - else - cur_sizeof_arg_loc = UNKNOWN_LOCATION; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, idx + 1); expr = c_parser_expr_no_commas (parser, NULL); @@ -8728,11 +8748,10 @@ c_parser_expr_list (c_parser *parser, bool convert locations->safe_push (loc); if (++idx < 3 && sizeof_arg != NULL - && cur_sizeof_arg_loc != UNKNOWN_LOCATION && expr.original_code == SIZEOF_EXPR) { sizeof_arg[idx] = c_last_sizeof_arg; - sizeof_arg_loc[idx] = cur_sizeof_arg_loc; + sizeof_arg_loc[idx] = c_last_sizeof_loc; } } if (orig_types) Index: gcc/c/c-tree.h =================================================================== --- gcc/c/c-tree.h (revision 246482) +++ gcc/c/c-tree.h (working copy) @@ -606,6 +606,7 @@ extern int in_sizeof; extern int in_typeof; extern tree c_last_sizeof_arg; +extern location_t c_last_sizeof_loc; extern struct c_switch *c_switch_stack; Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 246482) +++ gcc/c/c-typeck.c (working copy) @@ -72,6 +72,7 @@ int in_typeof; /* The argument of last parsed sizeof expression, only to be tested if expr.original_code == SIZEOF_EXPR. */ tree c_last_sizeof_arg; +location_t c_last_sizeof_loc; /* Nonzero if we might need to print a "missing braces around initializer" message within this initializer. */ @@ -2909,6 +2910,7 @@ c_expr_sizeof_expr (location_t loc, struct c_expr &expr_const_operands); ret.value = c_sizeof (loc, TREE_TYPE (folded_expr)); c_last_sizeof_arg = expr.value; + c_last_sizeof_loc = loc; ret.original_code = SIZEOF_EXPR; ret.original_type = NULL; if (c_vla_type_p (TREE_TYPE (folded_expr))) @@ -2938,6 +2940,7 @@ c_expr_sizeof_type (location_t loc, struct c_type_ type = groktypename (t, &type_expr, &type_expr_const); ret.value = c_sizeof (loc, type); c_last_sizeof_arg = type; + c_last_sizeof_loc = loc; ret.original_code = SIZEOF_EXPR; ret.original_type = NULL; if ((type_expr || TREE_CODE (ret.value) == INTEGER_CST) Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 246482) +++ gcc/c-family/c.opt (working copy) @@ -698,6 +698,10 @@ Wsized-deallocation C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra) Warn about missing sized deallocation functions. +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-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. Index: gcc/cp/cp-gimplify.c =================================================================== --- gcc/cp/cp-gimplify.c (revision 246482) +++ gcc/cp/cp-gimplify.c (working copy) @@ -2223,6 +2223,22 @@ cp_fold (tree x) else x = fold (x); + if (TREE_CODE (x) == INTEGER_CST && + TREE_CODE (org_x) == TRUNC_DIV_EXPR + && TREE_CODE (TREE_OPERAND (org_x, 0)) == SIZEOF_EXPR + && TREE_CODE (TREE_OPERAND (org_x, 1)) == SIZEOF_EXPR) + { + tree type0 = TREE_OPERAND (TREE_OPERAND (org_x, 0), 0); + tree type1 = TREE_OPERAND (TREE_OPERAND (org_x, 1), 0); + tree first_arg = type0; + if (!TYPE_P (type0)) + type0 = TREE_TYPE (type0); + if (!TYPE_P (type1)) + type1 = TREE_TYPE (type1); + if (POINTER_TYPE_P (type0) && same_type_p (TREE_TYPE (type0), type1) + && !(TREE_CODE (first_arg) == PARM_DECL + && DECL_ARRAY_PARAMETER_P (first_arg) + && warn_sizeof_array_argument)) + if (warning_at (loc, OPT_Wsizeof_pointer_div, + "division %<sizeof (%T) / sizeof (%T)%> does " + "not compute the number of array elements", + type0, type1)) + if (DECL_P (first_arg)) + inform (DECL_SOURCE_LOCATION (first_arg), + "first %<sizeof%> operand was declared here"); + } + if (TREE_NO_WARNING (org_x) && warn_nonnull_compare && COMPARISON_CLASS_P (org_x)) Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 246482) +++ gcc/doc/invoke.texi (working copy) @@ -304,7 +304,7 @@ Objective-C and Objective-C++ Dialects}. -Wshift-overflow -Wshift-overflow=@var{n} @gol -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol -Wsign-compare -Wsign-conversion -Wfloat-conversion @gol --Wno-scalar-storage-order @gol +-Wno-scalar-storage-order -Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess -Wsizeof-array-argument @gol -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol -Wstrict-aliasing=n -Wstrict-overflow -Wstrict-overflow=@var{n} @gol @@ -3777,6 +3777,7 @@ Options} and @ref{Objective-C and Objective-C++ Di -Wreturn-type @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol +-Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess @gol -Wstrict-aliasing @gol -Wstrict-overflow=1 @gol @@ -6022,6 +6023,15 @@ void operator delete[] (void *, std::size_t) noexc or vice versa. Enabled by @option{-Wextra} along with @option{-fsized-deallocation}. +@item -Wsizeof-pointer-div +@opindex Wsizeof-pointer-div +@opindex Wno-sizeof-pointer-div +Warn for suspicious divisions of two sizeof expressions that divide +the pointer size by the element size, which is the usual way to compute +the array size but won't work out correctly with pointers. This warning +warns e.g.@: about @code{sizeof (ptr) / sizeof (ptr[0])} if @code{ptr} is +not an array, but a pointer. This warning is enabled by @option{-Wall}. + @item -Wsizeof-pointer-memaccess @opindex Wsizeof-pointer-memaccess @opindex Wno-sizeof-pointer-memaccess Index: gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c =================================================================== --- gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c (revision 0) +++ gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c (working copy) @@ -0,0 +1,41 @@ +/* Test -Wsizeof-pointer-div warnings. */ +/* { dg-do compile } */ +/* { dg-options "-Wall" } */ + +int +f1 (int *array) +{ + int i; + i = sizeof array / sizeof *array; /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof array / sizeof array[0]; /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(*array); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(array[0]); /* { dg-warning "does not compute the number of array elements" } */ + i += (sizeof(array)) / (sizeof(array[0])); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(int); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(char); + i += sizeof(*array) / sizeof(char); + i += sizeof(array[0]) / sizeof(char); + return i; +} + +int +f2 (void) +{ + int array[10]; + int i; + i = sizeof array / sizeof *array; + i += sizeof array / sizeof array[0]; + i += sizeof(array) / sizeof(*array); + 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); + i += sizeof(array[0]) / sizeof(char); + return i; +} + +int f3 (int a[]) +{ + return sizeof a / sizeof *a; /* { dg-warning "Wsizeof-array-argument" } */ +} Index: gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c =================================================================== --- gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c (revision 246482) +++ gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c (working copy) @@ -73,6 +73,15 @@ f1 (void *x, int z) z += bcmp (x, pa2, sizeof (PTA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ z += bcmp (x, pa3, sizeof (PA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (&a), (sizeof (&a))); /* { dg-warning "call is the same expression as the second source; did you mean to remove the addressof" } */ + z += bcmp (x, (pa1), (sizeof (pa1))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa2), (sizeof pa2)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa3), (sizeof (pa3))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa4), (sizeof pa4)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa1), (sizeof (struct A *)));/* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (pa2), (sizeof (PTA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (pa3), (sizeof (PA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + /* These are correct, no warning. */ bzero (&a, sizeof a); bzero (&a, sizeof (a)); Index: gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c =================================================================== --- gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c (revision 246482) +++ gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c (working copy) @@ -150,6 +150,15 @@ f1 (void *x, int z) z += memcmp (x, pa2, sizeof (PTA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ z += memcmp (x, pa3, sizeof (PA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (&a), (sizeof (&a))); /* { dg-warning "call is the same expression as the second source; did you mean to remove the addressof" } */ + z += memcmp (x, (pa1), (sizeof (pa1))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa2), (sizeof pa2)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa3), (sizeof (pa3))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa4), (sizeof pa4)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa1), (sizeof (struct A *)));/* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (pa2), (sizeof (PTA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (pa3), (sizeof (PA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + /* These are correct, no warning. */ memset (&a, 0, sizeof a); memset (&a, 0, sizeof (a)); Index: gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c (working copy) @@ -17,13 +17,13 @@ check (__m64 x, unsigned short *v, int j) union { __m64 x; - unsigned short i[8]; + unsigned short i[4]; } u; unsigned int i; u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128 x, float *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.f[i]) Index: gcc/testsuite/gcc.target/i386/sse-set-ps-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-set-ps-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse-set-ps-1.c (working copy) @@ -23,7 +23,7 @@ test (float *v) u.x = _mm_set_ps (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.f[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned char *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned long long *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned int *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned short *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned int *v) u.x = _mm_set_epi32 (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned long long *v) u.x = _mm_set_epi64x (v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned char *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned long long *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128 x, float *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.f[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned int *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned int *v) u.x = _mm_set_epi32 (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned long long *v) u.x = _mm_set_epi64x (v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c (revision 246482) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c (working copy) @@ -23,7 +23,7 @@ test (float *v) u.x = _mm_set_ps (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.f[i]) { #ifdef DEBUG Index: libgomp/testsuite/libgomp.c/pr39591-2.c =================================================================== --- libgomp/testsuite/libgomp.c/pr39591-2.c (revision 246482) +++ libgomp/testsuite/libgomp.c/pr39591-2.c (working copy) @@ -11,7 +11,7 @@ foo (int *array) #pragma omp task { int j; - for (j = 0; j < sizeof array / sizeof array[0]; j++) + for (j = 0; j < 40; j++) if (array[j] != 0x55555555) #pragma omp atomic err++; Index: libgomp/testsuite/libgomp.c/pr39591-3.c =================================================================== --- libgomp/testsuite/libgomp.c/pr39591-3.c (revision 246482) +++ libgomp/testsuite/libgomp.c/pr39591-3.c (working copy) @@ -11,7 +11,7 @@ foo (int *array) #pragma omp task { int j; - for (j = 0; j < sizeof array / sizeof array[0]; j++) + for (j = 0; j < 40; j++) if (array[j] != 0x55555555) #pragma omp atomic err++; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-04-28 17:20 ` Bernd Edlinger @ 2017-05-01 15:54 ` Jason Merrill 2017-05-02 13:38 ` Bernd Edlinger 0 siblings, 1 reply; 13+ messages in thread From: Jason Merrill @ 2017-05-01 15:54 UTC (permalink / raw) To: Bernd Edlinger Cc: Martin Sebor, gcc-patches, Joseph Myers, Jeff Law, Richard Biener, Jakub Jelinek On Fri, Apr 28, 2017 at 1:05 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > On 04/28/17 17:29, Martin Sebor wrote: >> On 04/28/2017 08:12 AM, Bernd Edlinger wrote: >>> >>> Do you want me to change the %qT format strings to %T ? >> >> Yes, with the surrounding %< and %> the nested directives should >> use the unquoted forms, otherwise the printer would end up quoting >> both the whole expression and the type operand. >> >> FWIW, to help avoid this mistake, I think this might be something >> for GCC -Wformat to warn on and the pretty-printer to detect (and >> ICE on). >> > > Ah, now I understand. That's pretty advanced. > > Here is the modified patch with correct quoting of the expression. > > Bootstrap and reg-testing on x86_64-pc-linux-gnu. > * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. I think this warning belongs in cp_build_binary_op rather than cp_fold. Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-05-01 15:54 ` Jason Merrill @ 2017-05-02 13:38 ` Bernd Edlinger 2017-05-03 13:15 ` Jason Merrill 0 siblings, 1 reply; 13+ messages in thread From: Bernd Edlinger @ 2017-05-02 13:38 UTC (permalink / raw) To: Jason Merrill Cc: Martin Sebor, gcc-patches, Joseph Myers, Jeff Law, Richard Biener, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 1122 bytes --] On 05/01/17 17:54, Jason Merrill wrote: > On Fri, Apr 28, 2017 at 1:05 PM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> On 04/28/17 17:29, Martin Sebor wrote: >>> On 04/28/2017 08:12 AM, Bernd Edlinger wrote: >>>> >>>> Do you want me to change the %qT format strings to %T ? >>> >>> Yes, with the surrounding %< and %> the nested directives should >>> use the unquoted forms, otherwise the printer would end up quoting >>> both the whole expression and the type operand. >>> >>> FWIW, to help avoid this mistake, I think this might be something >>> for GCC -Wformat to warn on and the pretty-printer to detect (and >>> ICE on). >>> >> >> Ah, now I understand. That's pretty advanced. >> >> Here is the modified patch with correct quoting of the expression. >> >> Bootstrap and reg-testing on x86_64-pc-linux-gnu. > >> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. > > I think this warning belongs in cp_build_binary_op rather than cp_fold. > Done, as suggested. Bootstrap and reg-testing on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. [-- Attachment #2: changelog-warn-sizeof-pointer.txt --] [-- Type: text/plain, Size: 1985 bytes --] gcc: 2017-05-01 Bernd Edlinger <bernd.edlinger@hotmail.de> * doc/invoke.texi: Document the -Wsizeof-pointer-div warning. gcc/c-family: 2017-05-01 Bernd Edlinger <bernd.edlinger@hotmail.de> * c.opt (Wsizeof-pointer-div): New warning option. gcc/c: 2017-05-01 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-parser.c (c_parser_binary_expression): Implement the -Wsizeof_pointer_div warning. (c_parser_postfix_expression): Allow SIZEOF_EXPR as expr.original_code from a parenthesized expression. (c_parser_expr_list): Use c_last_sizeof_loc. * c-tree.h (c_last_sizeof_loc): New external. * c-typeck.c (c_last_sizeof_loc): New variable. (c_expr_sizeof_expr, c_expr_sizeof_type): Assign c_last_sizeof_loc. gcc/cp: 2017-05-01 Bernd Edlinger <bernd.edlinger@hotmail.de> * typeck.c (cp_build_binary_op): Implement the -Wsizeof_pointer_div warning. gcc/testsuite: 2017-05-01 Bernd Edlinger <bernd.edlinger@hotmail.de> * c-c++-common/Wsizeof-pointer-div.c: New test. * gcc.dg/Wsizeof-pointer-memaccess1.c: Add test cases with parens. * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Likewise. * gcc.target/i386/sse-init-v4hi-1.c: Fix test case. * gcc.target/i386/sse-init-v4sf-1.c: Likewise. * gcc.target/i386/sse-set-ps-1.c: Likewise. * gcc.target/i386/sse2-init-v16qi-1.c: Likewise. * gcc.target/i386/sse2-init-v2di-1.c: Likewise. * gcc.target/i386/sse2-init-v4si-1.c: Likewise. * gcc.target/i386/sse2-init-v8hi-1.c: Likewise. * gcc.target/i386/sse2-set-epi32-1.c: Likewise. * gcc.target/i386/sse2-set-epi64x-1.c: Likewise. * gcc.target/i386/sse4_1-init-v16qi-1.c: Likewise. * gcc.target/i386/sse4_1-init-v2di-1.c: Likewise. * gcc.target/i386/sse4_1-init-v4sf-1.c: Likewise. * gcc.target/i386/sse4_1-init-v4si-1.c: Likewise. * gcc.target/i386/sse4_1-set-epi32-1.c: Likewise. * gcc.target/i386/sse4_1-set-epi64x-1.c: Likewise. * gcc.target/i386/sse4_1-set-ps-1.c: Likewise. * libgomp.c/pr39591-2.c: Likewise. * libgomp.c/pr39591-3.c: Likewise. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: patch-warn-sizeof-pointer.diff --] [-- Type: text/x-patch; name="patch-warn-sizeof-pointer.diff", Size: 25764 bytes --] Index: gcc/c/c-parser.c =================================================================== --- gcc/c/c-parser.c (revision 247440) +++ gcc/c/c-parser.c (working copy) @@ -6657,6 +6657,8 @@ c_parser_binary_expression (c_parser *parser, stru enum tree_code op; /* The source location of this operation. */ location_t loc; + /* The sizeof argument if expr.original_code == SIZEOF_EXPR. */ + tree sizeof_arg; } stack[NUM_PRECS]; int sp; /* Location of the binary operator. */ @@ -6673,6 +6675,31 @@ c_parser_binary_expression (c_parser *parser, stru 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) \ + { \ + tree type0 = stack[sp - 1].sizeof_arg; \ + tree type1 = stack[sp].sizeof_arg; \ + tree first_arg = type0; \ + if (!TYPE_P (type0)) \ + type0 = TREE_TYPE (type0); \ + if (!TYPE_P (type1)) \ + type1 = TREE_TYPE (type1); \ + if (POINTER_TYPE_P (type0) \ + && comptypes (TREE_TYPE (type0), type1) \ + && !(TREE_CODE (first_arg) == PARM_DECL \ + && C_ARRAY_PARAMETER (first_arg) \ + && warn_sizeof_array_argument)) \ + if (warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, \ + "division %<sizeof (%T) / sizeof (%T)%> does " \ + "not compute the number of array elements", \ + type0, type1)) \ + if (DECL_P (first_arg)) \ + inform (DECL_SOURCE_LOCATION (first_arg), \ + "first %<sizeof%> operand was declared here"); \ + } \ + break; \ default: \ break; \ } \ @@ -6706,6 +6733,7 @@ c_parser_binary_expression (c_parser *parser, stru stack[0].loc = c_parser_peek_token (parser)->location; stack[0].expr = c_parser_cast_expression (parser, after); stack[0].prec = PREC_NONE; + stack[0].sizeof_arg = c_last_sizeof_arg; sp = 0; while (true) { @@ -6829,6 +6857,7 @@ c_parser_binary_expression (c_parser *parser, stru stack[sp].expr = c_parser_cast_expression (parser, NULL); stack[sp].prec = oprec; stack[sp].op = ocode; + stack[sp].sizeof_arg = c_last_sizeof_arg; } out: while (sp > 0) @@ -7720,7 +7749,8 @@ 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 (expr.original_code != C_MAYBE_CONST_EXPR) + if (expr.original_code != C_MAYBE_CONST_EXPR + && expr.original_code != SIZEOF_EXPR) expr.original_code = ERROR_MARK; /* Don't change EXPR.ORIGINAL_TYPE. */ location_t loc_close_paren = c_parser_peek_token (parser)->location; @@ -8679,7 +8709,6 @@ c_parser_expr_list (c_parser *parser, bool convert vec<tree, va_gc> *orig_types; struct c_expr expr; location_t loc = c_parser_peek_token (parser)->location; - location_t cur_sizeof_arg_loc = UNKNOWN_LOCATION; unsigned int idx = 0; ret = make_tree_vector (); @@ -8688,9 +8717,6 @@ c_parser_expr_list (c_parser *parser, bool convert else orig_types = make_tree_vector (); - if (sizeof_arg != NULL - && c_parser_next_token_is_keyword (parser, RID_SIZEOF)) - cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, 0); expr = c_parser_expr_no_commas (parser, NULL); @@ -8704,21 +8730,15 @@ c_parser_expr_list (c_parser *parser, bool convert if (locations) locations->safe_push (loc); if (sizeof_arg != NULL - && cur_sizeof_arg_loc != UNKNOWN_LOCATION && expr.original_code == SIZEOF_EXPR) { sizeof_arg[0] = c_last_sizeof_arg; - sizeof_arg_loc[0] = cur_sizeof_arg_loc; + sizeof_arg_loc[0] = c_last_sizeof_loc; } while (c_parser_next_token_is (parser, CPP_COMMA)) { c_parser_consume_token (parser); loc = c_parser_peek_token (parser)->location; - if (sizeof_arg != NULL - && c_parser_next_token_is_keyword (parser, RID_SIZEOF)) - cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; - else - cur_sizeof_arg_loc = UNKNOWN_LOCATION; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, idx + 1); expr = c_parser_expr_no_commas (parser, NULL); @@ -8733,11 +8753,10 @@ c_parser_expr_list (c_parser *parser, bool convert locations->safe_push (loc); if (++idx < 3 && sizeof_arg != NULL - && cur_sizeof_arg_loc != UNKNOWN_LOCATION && expr.original_code == SIZEOF_EXPR) { sizeof_arg[idx] = c_last_sizeof_arg; - sizeof_arg_loc[idx] = cur_sizeof_arg_loc; + sizeof_arg_loc[idx] = c_last_sizeof_loc; } } if (orig_types) Index: gcc/c/c-tree.h =================================================================== --- gcc/c/c-tree.h (revision 247440) +++ gcc/c/c-tree.h (working copy) @@ -606,6 +606,7 @@ extern int in_sizeof; extern int in_typeof; extern tree c_last_sizeof_arg; +extern location_t c_last_sizeof_loc; extern struct c_switch *c_switch_stack; Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 247440) +++ gcc/c/c-typeck.c (working copy) @@ -72,6 +72,7 @@ int in_typeof; /* The argument of last parsed sizeof expression, only to be tested if expr.original_code == SIZEOF_EXPR. */ tree c_last_sizeof_arg; +location_t c_last_sizeof_loc; /* Nonzero if we might need to print a "missing braces around initializer" message within this initializer. */ @@ -2909,6 +2910,7 @@ c_expr_sizeof_expr (location_t loc, struct c_expr &expr_const_operands); ret.value = c_sizeof (loc, TREE_TYPE (folded_expr)); c_last_sizeof_arg = expr.value; + c_last_sizeof_loc = loc; ret.original_code = SIZEOF_EXPR; ret.original_type = NULL; if (c_vla_type_p (TREE_TYPE (folded_expr))) @@ -2938,6 +2940,7 @@ c_expr_sizeof_type (location_t loc, struct c_type_ type = groktypename (t, &type_expr, &type_expr_const); ret.value = c_sizeof (loc, type); c_last_sizeof_arg = type; + c_last_sizeof_loc = loc; ret.original_code = SIZEOF_EXPR; ret.original_type = NULL; if ((type_expr || TREE_CODE (ret.value) == INTEGER_CST) Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 247440) +++ gcc/c-family/c.opt (working copy) @@ -702,6 +702,10 @@ Wsized-deallocation C++ ObjC++ Var(warn_sized_deallocation) Warning EnabledBy(Wextra) Warn about missing sized deallocation functions. +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-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. Index: gcc/cp/typeck.c =================================================================== --- gcc/cp/typeck.c (revision 247440) +++ gcc/cp/typeck.c (working copy) @@ -4324,6 +4324,30 @@ cp_build_binary_op (location_t location, break; case TRUNC_DIV_EXPR: + if (TREE_CODE (op0) == SIZEOF_EXPR && TREE_CODE (op1) == SIZEOF_EXPR) + { + tree type0 = TREE_OPERAND (op0, 0); + tree type1 = TREE_OPERAND (op1, 0); + tree first_arg = type0; + if (!TYPE_P (type0)) + type0 = TREE_TYPE (type0); + if (!TYPE_P (type1)) + type1 = TREE_TYPE (type1); + if (POINTER_TYPE_P (type0) && same_type_p (TREE_TYPE (type0), type1) + && !(TREE_CODE (first_arg) == PARM_DECL + && DECL_ARRAY_PARAMETER_P (first_arg) + && warn_sizeof_array_argument) + && (complain & tf_warning)) + if (warning_at (location, OPT_Wsizeof_pointer_div, + "division %<sizeof (%T) / sizeof (%T)%> does " + "not compute the number of array elements", + type0, type1)) + if (DECL_P (first_arg)) + inform (DECL_SOURCE_LOCATION (first_arg), + "first %<sizeof%> operand was declared here"); + } + gcc_fallthrough (); + case CEIL_DIV_EXPR: case FLOOR_DIV_EXPR: case ROUND_DIV_EXPR: Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 247440) +++ gcc/doc/invoke.texi (working copy) @@ -305,7 +305,7 @@ Objective-C and Objective-C++ Dialects}. -Wshift-overflow -Wshift-overflow=@var{n} @gol -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol -Wsign-compare -Wsign-conversion -Wfloat-conversion @gol --Wno-scalar-storage-order @gol +-Wno-scalar-storage-order -Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess -Wsizeof-array-argument @gol -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol -Wstrict-aliasing=n -Wstrict-overflow -Wstrict-overflow=@var{n} @gol @@ -3777,6 +3777,7 @@ Options} and @ref{Objective-C and Objective-C++ Di -Wreturn-type @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol +-Wsizeof-pointer-div @gol -Wsizeof-pointer-memaccess @gol -Wstrict-aliasing @gol -Wstrict-overflow=1 @gol @@ -6027,6 +6028,15 @@ void operator delete[] (void *, std::size_t) noexc or vice versa. Enabled by @option{-Wextra} along with @option{-fsized-deallocation}. +@item -Wsizeof-pointer-div +@opindex Wsizeof-pointer-div +@opindex Wno-sizeof-pointer-div +Warn for suspicious divisions of two sizeof expressions that divide +the pointer size by the element size, which is the usual way to compute +the array size but won't work out correctly with pointers. This warning +warns e.g.@: about @code{sizeof (ptr) / sizeof (ptr[0])} if @code{ptr} is +not an array, but a pointer. This warning is enabled by @option{-Wall}. + @item -Wsizeof-pointer-memaccess @opindex Wsizeof-pointer-memaccess @opindex Wno-sizeof-pointer-memaccess Index: gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c =================================================================== --- gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c (revision 0) +++ gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c (working copy) @@ -0,0 +1,42 @@ +/* Test -Wsizeof-pointer-div warnings. */ +/* { dg-do compile } */ +/* { dg-options "-Wall" } */ + +int +f1 (int *array) +{ + int i; + i = sizeof array / sizeof *array; /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof array / sizeof array[0]; /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(*array); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(array[0]); /* { dg-warning "does not compute the number of array elements" } */ + i += (sizeof(array)) / (sizeof(array[0])); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(int); /* { dg-warning "does not compute the number of array elements" } */ + i += sizeof(array) / sizeof(char); + i += sizeof(*array) / sizeof(char); + i += sizeof(array[0]) / sizeof(char); + return i; +} + +int +f2 (void) +{ + int array[10]; + int i; + i = sizeof array / sizeof *array; + i += sizeof array / sizeof array[0]; + i += sizeof(array) / sizeof(*array); + 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); + i += sizeof(array[0]) / sizeof(char); + return i; +} + +int +f3 (int a[]) +{ + return sizeof a / sizeof *a; /* { dg-warning "Wsizeof-array-argument" } */ +} Index: gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c =================================================================== --- gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c (revision 247440) +++ gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c (working copy) @@ -73,6 +73,15 @@ f1 (void *x, int z) z += bcmp (x, pa2, sizeof (PTA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ z += bcmp (x, pa3, sizeof (PA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (&a), (sizeof (&a))); /* { dg-warning "call is the same expression as the second source; did you mean to remove the addressof" } */ + z += bcmp (x, (pa1), (sizeof (pa1))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa2), (sizeof pa2)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa3), (sizeof (pa3))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa4), (sizeof pa4)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += bcmp (x, (pa1), (sizeof (struct A *)));/* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (pa2), (sizeof (PTA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += bcmp (x, (pa3), (sizeof (PA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + /* These are correct, no warning. */ bzero (&a, sizeof a); bzero (&a, sizeof (a)); Index: gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c =================================================================== --- gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c (revision 247440) +++ gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c (working copy) @@ -150,6 +150,15 @@ f1 (void *x, int z) z += memcmp (x, pa2, sizeof (PTA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ z += memcmp (x, pa3, sizeof (PA)); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (&a), (sizeof (&a))); /* { dg-warning "call is the same expression as the second source; did you mean to remove the addressof" } */ + z += memcmp (x, (pa1), (sizeof (pa1))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa2), (sizeof pa2)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa3), (sizeof (pa3))); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa4), (sizeof pa4)); /* { dg-warning "call is the same expression as the second source; did you mean to dereference it" } */ + z += memcmp (x, (pa1), (sizeof (struct A *)));/* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (pa2), (sizeof (PTA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + z += memcmp (x, (pa3), (sizeof (PA))); /* { dg-warning "call is the same pointer type \[^\n\r\]* as the second source; expected \[^\n\r\]* or an explicit length" } */ + /* These are correct, no warning. */ memset (&a, 0, sizeof a); memset (&a, 0, sizeof (a)); Index: gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse-init-v4hi-1.c (working copy) @@ -17,13 +17,13 @@ check (__m64 x, unsigned short *v, int j) union { __m64 x; - unsigned short i[8]; + unsigned short i[4]; } u; unsigned int i; u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse-init-v4sf-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128 x, float *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.f[i]) Index: gcc/testsuite/gcc.target/i386/sse-set-ps-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse-set-ps-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse-set-ps-1.c (working copy) @@ -23,7 +23,7 @@ test (float *v) u.x = _mm_set_ps (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.f[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse2-init-v16qi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned char *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse2-init-v2di-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned long long *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse2-init-v4si-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned int *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse2-init-v8hi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned short *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse2-set-epi32-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned int *v) u.x = _mm_set_epi32 (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse2-set-epi64x-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned long long *v) u.x = _mm_set_epi64x (v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v16qi-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned char *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v2di-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned long long *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v4sf-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128 x, float *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.f[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse4_1-init-v4si-1.c (working copy) @@ -23,7 +23,7 @@ check (__m128i x, unsigned int *v, int j) u.x = x; - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (i == j) { if (v[i] != u.i[i]) Index: gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-epi32-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned int *v) u.x = _mm_set_epi32 (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-epi64x-1.c (working copy) @@ -23,7 +23,7 @@ test (unsigned long long *v) u.x = _mm_set_epi64x (v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.i[i]) { #ifdef DEBUG Index: gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c =================================================================== --- gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c (revision 247440) +++ gcc/testsuite/gcc.target/i386/sse4_1-set-ps-1.c (working copy) @@ -23,7 +23,7 @@ test (float *v) u.x = _mm_set_ps (v[3], v[2], v[1], v[0]); - for (i = 0; i < sizeof (v) / sizeof (v[0]); i++) + for (i = 0; i < sizeof (u) / sizeof (v[0]); i++) if (v[i] != u.f[i]) { #ifdef DEBUG Index: libgomp/testsuite/libgomp.c/pr39591-2.c =================================================================== --- libgomp/testsuite/libgomp.c/pr39591-2.c (revision 247440) +++ libgomp/testsuite/libgomp.c/pr39591-2.c (working copy) @@ -11,7 +11,7 @@ foo (int *array) #pragma omp task { int j; - for (j = 0; j < sizeof array / sizeof array[0]; j++) + for (j = 0; j < 40; j++) if (array[j] != 0x55555555) #pragma omp atomic err++; Index: libgomp/testsuite/libgomp.c/pr39591-3.c =================================================================== --- libgomp/testsuite/libgomp.c/pr39591-3.c (revision 247440) +++ libgomp/testsuite/libgomp.c/pr39591-3.c (working copy) @@ -11,7 +11,7 @@ foo (int *array) #pragma omp task { int j; - for (j = 0; j < sizeof array / sizeof array[0]; j++) + for (j = 0; j < 40; j++) if (array[j] != 0x55555555) #pragma omp atomic err++; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-05-02 13:38 ` Bernd Edlinger @ 2017-05-03 13:15 ` Jason Merrill 2017-05-12 16:56 ` [PING] " Bernd Edlinger 0 siblings, 1 reply; 13+ messages in thread From: Jason Merrill @ 2017-05-03 13:15 UTC (permalink / raw) To: Bernd Edlinger Cc: Martin Sebor, gcc-patches, Joseph Myers, Jeff Law, Richard Biener, Jakub Jelinek, Marek Polacek On Tue, May 2, 2017 at 9:26 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > On 05/01/17 17:54, Jason Merrill wrote: >> On Fri, Apr 28, 2017 at 1:05 PM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >>> On 04/28/17 17:29, Martin Sebor wrote: >>>> On 04/28/2017 08:12 AM, Bernd Edlinger wrote: >>>>> >>>>> Do you want me to change the %qT format strings to %T ? >>>> >>>> Yes, with the surrounding %< and %> the nested directives should >>>> use the unquoted forms, otherwise the printer would end up quoting >>>> both the whole expression and the type operand. >>>> >>>> FWIW, to help avoid this mistake, I think this might be something >>>> for GCC -Wformat to warn on and the pretty-printer to detect (and >>>> ICE on). >>>> >>> >>> Ah, now I understand. That's pretty advanced. >>> >>> Here is the modified patch with correct quoting of the expression. >>> >>> Bootstrap and reg-testing on x86_64-pc-linux-gnu. >> >>> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. >> >> I think this warning belongs in cp_build_binary_op rather than cp_fold. >> > > Done, as suggested. The pattern in that function is to treat all *_DIV_EXPR the same; I don't think we need to break that pattern with this patch. So please move the new code after the other DIV case labels. With that the C++ changes are OK. Jason ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PING] [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-05-03 13:15 ` Jason Merrill @ 2017-05-12 16:56 ` Bernd Edlinger 2017-06-01 16:03 ` [PING**2] " Bernd Edlinger 0 siblings, 1 reply; 13+ messages in thread From: Bernd Edlinger @ 2017-05-12 16:56 UTC (permalink / raw) To: Jason Merrill Cc: Martin Sebor, gcc-patches, Joseph Myers, Jeff Law, Richard Biener, Jakub Jelinek, Marek Polacek Ping for the C changes. Thanks Bernd. On 05/03/17 15:14, Jason Merrill wrote: > On Tue, May 2, 2017 at 9:26 AM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> On 05/01/17 17:54, Jason Merrill wrote: >>> On Fri, Apr 28, 2017 at 1:05 PM, Bernd Edlinger >>> <bernd.edlinger@hotmail.de> wrote: >>>> On 04/28/17 17:29, Martin Sebor wrote: >>>>> On 04/28/2017 08:12 AM, Bernd Edlinger wrote: >>>>>> >>>>>> Do you want me to change the %qT format strings to %T ? >>>>> >>>>> Yes, with the surrounding %< and %> the nested directives should >>>>> use the unquoted forms, otherwise the printer would end up quoting >>>>> both the whole expression and the type operand. >>>>> >>>>> FWIW, to help avoid this mistake, I think this might be something >>>>> for GCC -Wformat to warn on and the pretty-printer to detect (and >>>>> ICE on). >>>>> >>>> >>>> Ah, now I understand. That's pretty advanced. >>>> >>>> Here is the modified patch with correct quoting of the expression. >>>> >>>> Bootstrap and reg-testing on x86_64-pc-linux-gnu. >>> >>>> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. >>> >>> I think this warning belongs in cp_build_binary_op rather than cp_fold. >>> >> >> Done, as suggested. > > The pattern in that function is to treat all *_DIV_EXPR the same; I > don't think we need to break that pattern with this patch. So please > move the new code after the other DIV case labels. With that the C++ > changes are OK. > > Jason > On 05/03/17 15:14, Jason Merrill wrote: > On Tue, May 2, 2017 at 9:26 AM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> On 05/01/17 17:54, Jason Merrill wrote: >>> On Fri, Apr 28, 2017 at 1:05 PM, Bernd Edlinger >>> <bernd.edlinger@hotmail.de> wrote: >>>> On 04/28/17 17:29, Martin Sebor wrote: >>>>> On 04/28/2017 08:12 AM, Bernd Edlinger wrote: >>>>>> >>>>>> Do you want me to change the %qT format strings to %T ? >>>>> >>>>> Yes, with the surrounding %< and %> the nested directives should >>>>> use the unquoted forms, otherwise the printer would end up quoting >>>>> both the whole expression and the type operand. >>>>> >>>>> FWIW, to help avoid this mistake, I think this might be something >>>>> for GCC -Wformat to warn on and the pretty-printer to detect (and >>>>> ICE on). >>>>> >>>> >>>> Ah, now I understand. That's pretty advanced. >>>> >>>> Here is the modified patch with correct quoting of the expression. >>>> >>>> Bootstrap and reg-testing on x86_64-pc-linux-gnu. >>> >>>> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. >>> >>> I think this warning belongs in cp_build_binary_op rather than cp_fold. >>> >> >> Done, as suggested. > > The pattern in that function is to treat all *_DIV_EXPR the same; I > don't think we need to break that pattern with this patch. So please > move the new code after the other DIV case labels. With that the C++ > changes are OK. > > Jason > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PING**2] [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-05-12 16:56 ` [PING] " Bernd Edlinger @ 2017-06-01 16:03 ` Bernd Edlinger 2017-06-01 22:09 ` Joseph Myers 0 siblings, 1 reply; 13+ messages in thread From: Bernd Edlinger @ 2017-06-01 16:03 UTC (permalink / raw) To: Jason Merrill Cc: Martin Sebor, gcc-patches, Joseph Myers, Jeff Law, Richard Biener, Jakub Jelinek, Marek Polacek Ping... On 05/12/17 18:55, Bernd Edlinger wrote: > Ping for the C changes. > > Thanks > Bernd. > > On 05/03/17 15:14, Jason Merrill wrote: >> On Tue, May 2, 2017 at 9:26 AM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >>> On 05/01/17 17:54, Jason Merrill wrote: >>>> On Fri, Apr 28, 2017 at 1:05 PM, Bernd Edlinger >>>> <bernd.edlinger@hotmail.de> wrote: >>>>> On 04/28/17 17:29, Martin Sebor wrote: >>>>>> On 04/28/2017 08:12 AM, Bernd Edlinger wrote: >>>>>>> >>>>>>> Do you want me to change the %qT format strings to %T ? >>>>>> >>>>>> Yes, with the surrounding %< and %> the nested directives should >>>>>> use the unquoted forms, otherwise the printer would end up quoting >>>>>> both the whole expression and the type operand. >>>>>> >>>>>> FWIW, to help avoid this mistake, I think this might be something >>>>>> for GCC -Wformat to warn on and the pretty-printer to detect (and >>>>>> ICE on). >>>>>> >>>>> >>>>> Ah, now I understand. That's pretty advanced. >>>>> >>>>> Here is the modified patch with correct quoting of the expression. >>>>> >>>>> Bootstrap and reg-testing on x86_64-pc-linux-gnu. >>>> >>>>> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div warning. >>>> >>>> I think this warning belongs in cp_build_binary_op rather than cp_fold. >>>> >>> >>> Done, as suggested. >> >> The pattern in that function is to treat all *_DIV_EXPR the same; I >> don't think we need to break that pattern with this patch. So please >> move the new code after the other DIV case labels. With that the C++ >> changes are OK. >> >> Jason >> > > > On 05/03/17 15:14, Jason Merrill wrote: > > On Tue, May 2, 2017 at 9:26 AM, Bernd Edlinger > > <bernd.edlinger@hotmail.de> wrote: > >> On 05/01/17 17:54, Jason Merrill wrote: > >>> On Fri, Apr 28, 2017 at 1:05 PM, Bernd Edlinger > >>> <bernd.edlinger@hotmail.de> wrote: > >>>> On 04/28/17 17:29, Martin Sebor wrote: > >>>>> On 04/28/2017 08:12 AM, Bernd Edlinger wrote: > >>>>>> > >>>>>> Do you want me to change the %qT format strings to %T ? > >>>>> > >>>>> Yes, with the surrounding %< and %> the nested directives should > >>>>> use the unquoted forms, otherwise the printer would end up quoting > >>>>> both the whole expression and the type operand. > >>>>> > >>>>> FWIW, to help avoid this mistake, I think this might be something > >>>>> for GCC -Wformat to warn on and the pretty-printer to detect (and > >>>>> ICE on). > >>>>> > >>>> > >>>> Ah, now I understand. That's pretty advanced. > >>>> > >>>> Here is the modified patch with correct quoting of the expression. > >>>> > >>>> Bootstrap and reg-testing on x86_64-pc-linux-gnu. > >>> > >>>> * cp-gimplify.c (cp_fold): Implement the -Wsizeof_pointer_div > warning. > >>> > >>> I think this warning belongs in cp_build_binary_op rather than > cp_fold. > >>> > >> > >> Done, as suggested. > > > > The pattern in that function is to treat all *_DIV_EXPR the same; I > > don't think we need to break that pattern with this patch. So please > > move the new code after the other DIV case labels. With that the C++ > > changes are OK. > > > > Jason > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PING**2] [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) 2017-06-01 16:03 ` [PING**2] " Bernd Edlinger @ 2017-06-01 22:09 ` Joseph Myers 0 siblings, 0 replies; 13+ messages in thread From: Joseph Myers @ 2017-06-01 22:09 UTC (permalink / raw) To: Bernd Edlinger Cc: Jason Merrill, Martin Sebor, gcc-patches, Jeff Law, Richard Biener, Jakub Jelinek, Marek Polacek The C changes are OK. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-06-01 22:09 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-20 21:33 [PATCH] Implement a warning for bogus sizeof(pointer) / sizeof(pointer[0]) Bernd Edlinger 2017-04-22 6:51 ` Martin Sebor 2017-04-22 17:50 ` Bernd Edlinger 2017-04-28 3:23 ` Martin Sebor 2017-04-28 14:42 ` Bernd Edlinger 2017-04-28 16:27 ` Martin Sebor 2017-04-28 17:20 ` Bernd Edlinger 2017-05-01 15:54 ` Jason Merrill 2017-05-02 13:38 ` Bernd Edlinger 2017-05-03 13:15 ` Jason Merrill 2017-05-12 16:56 ` [PING] " Bernd Edlinger 2017-06-01 16:03 ` [PING**2] " Bernd Edlinger 2017-06-01 22:09 ` Joseph Myers
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).