On Mon, Sep 14, 2020 at 08:39:36PM +0000, Joseph Myers wrote:
> On Mon, 14 Sep 2020, Marek Polacek via Gcc-patches wrote:
>
> > so I followed suit. In the C++ FE this was rather easy, because
> > finish_parenthesized_expr already set TREE_NO_WARNING. In the C FE
> > it was trickier; I've added a NOP_EXPR to discern between the non-()
> > and () versions.
>
> This sort of thing is normally handled for C via original_code in c_expr.
> I suppose that doesn't work in this case because the code dealing with
> parenthesized expressions has a special case for sizeof:
>
> if (expr.original_code != C_MAYBE_CONST_EXPR
> && expr.original_code != SIZEOF_EXPR)
> expr.original_code = ERROR_MARK;
>
> Handling this in some way via c_expr seems better to me than generating
> NOP_EXPR. I suppose you could invent a PAREN_SIZEOF_EXPR used by (sizeof
> foo) and ((sizeof foo)) etc. as an original_code setting (and handled the
> same as SIZEOF_EXPR by whatever other warnings look for SIZEOF_EXPR
> there), or else add fields to c_expr to allow more such information to be
> tracked there.
I went with the last option. But there was a snag: c_expr doesn't have
a default constructor, so the new member would remain uninitialized.
Instead of initializing .parenthesized_p everywhere where we initialize
.original_{type,type}, I only initialize .parenthesized_p when creating
a SIZEOF_EXPR.
It would seem appropriate to add a default constructor/NSDMIs for c_expr,
and then clean up the codebase so that we don't initialize c_expr's fields
manually. Would you accept such a patch? It's clearly out of scope for
this patch though. Such a change would also mean that c_expr is no longer
a trivial type, meaning that we can't use memset on it, and that we couldn't
skip its initialization like we do e.g. in c_parser_alignof_expression.
Thanks for the review.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >8 --
This patch implements a new warning, -Wsizeof-array-div. It warns about
code like
int arr[10];
sizeof (arr) / sizeof (short);
where we have a division of two sizeof expressions, where the first
argument is an array, and the second sizeof does not equal the size
of the array element. See e.g. <https://www.viva64.com/en/examples/v706/>.
Clang makes it possible to suppress the warning by parenthesizing the
second sizeof like this:
sizeof (arr) / (sizeof (short));
so I followed suit. In the C++ FE this was rather easy, because
finish_parenthesized_expr already set TREE_NO_WARNING. In the C FE
I've added a parenthesized_p member to c_expr to discern between the
non-() and () versions.
This warning is enabled by -Wall. An example of the output:
x.c:5:23: warning: expression does not compute the number of elements in this
array; element type is ‘int’, not ‘short int’ [-Wsizeof-array-div]
5 | return sizeof (arr) / sizeof (short);
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
x.c:5:25: note: add parentheses around ‘sizeof (short int)’ to silence this
warning
5 | return sizeof (arr) / sizeof (short);
| ^~~~~~~~~~~~~~
| ( )
x.c:4:7: note: array ‘arr’ declared here
4 | int arr[10];
| ^~~
gcc/c-family/ChangeLog:
PR c++/91741
* c-common.h (maybe_warn_sizeof_array_div): Declare.
* c-warn.c (sizeof_pointer_memaccess_warning): Unwrap NOP_EXPRs.
(maybe_warn_sizeof_array_div): New function.
* c.opt (Wsizeof-array-div): New option.
gcc/c/ChangeLog:
PR c++/91741
* c-parser.c (c_parser_binary_expression): Implement -Wsizeof-array-div.
(c_parser_postfix_expression): Set expr.parenthesized_p.
* c-tree.h (struct c_expr): Add parenthesized_p member.
(char_type_p): Declare.
* c-typeck.c (c_expr_sizeof_expr): Initialize ret.parenthesized_p.
(c_expr_sizeof_type): Likewise.
(char_type_p): No longer static.
gcc/cp/ChangeLog:
PR c++/91741
* typeck.c (cp_build_binary_op): Implement -Wsizeof-array-div.
gcc/ChangeLog:
PR c++/91741
* doc/invoke.texi: Document -Wsizeof-array-div.
gcc/testsuite/ChangeLog:
PR c++/91741
* c-c++-common/Wsizeof-pointer-div.c: Add dg-warning.
* c-c++-common/Wsizeof-array-div1.c: New test.
* g++.dg/warn/Wsizeof-array-div1.C: New test.
* g++.dg/warn/Wsizeof-array-div2.C: New test.
---
gcc/c-family/c-common.h | 1 +
gcc/c-family/c-warn.c | 47 ++++++++++++++++
gcc/c-family/c.opt | 5 ++
gcc/c/c-parser.c | 33 ++++++-----
gcc/c/c-tree.h | 6 ++
gcc/c/c-typeck.c | 4 +-
gcc/cp/typeck.c | 10 +++-
gcc/doc/invoke.texi | 19 +++++++
.../c-c++-common/Wsizeof-array-div1.c | 56 +++++++++++++++++++
.../c-c++-common/Wsizeof-pointer-div.c | 2 +-
.../g++.dg/warn/Wsizeof-array-div1.C | 37 ++++++++++++
.../g++.dg/warn/Wsizeof-array-div2.C | 15 +++++
12 files changed, 218 insertions(+), 17 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/Wsizeof-array-div1.c
create mode 100644 gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C
create mode 100644 gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 4fc64bc4aa6..443aaa2ca9c 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1321,6 +1321,7 @@ extern void c_do_switch_warnings (splay_tree, location_t,
tree, tree, bool);
extern void warn_for_omitted_condop (location_t, tree);
extern bool warn_for_restrict (unsigned, tree *, unsigned);
extern void warn_for_address_or_pointer_of_packed_member (tree, tree);
+extern void maybe_warn_sizeof_array_div (location_t, tree, tree, tree, tree);
/* Places where an lvalue, or modifiable lvalue, may be required.
Used to select diagnostic messages in lvalue_error and
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index c32d8228b5c..6fdada5f9b7 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -3099,3 +3099,50 @@ warn_for_address_or_pointer_of_packed_member (tree type,
tree rhs)
check_and_warn_address_or_pointer_of_packed_member (type, rhs);
}
+
+/* Warn about divisions of two sizeof operators when the first one is applied
+ to an array and the divisor does not equal the size of the array element.
+ For instance:
+
+ sizeof (ARR) / sizeof (OP)
+
+ ARR is the array argument of the first sizeof, ARR_TYPE is its ARRAY_TYPE.
+ OP1 is the whole second SIZEOF_EXPR, or its argument; TYPE1 is the type
+ of the second argument. */
+
+void
+maybe_warn_sizeof_array_div (location_t loc, tree arr, tree arr_type,
+ tree op1, tree type1)
+{
+ tree elt_type = TREE_TYPE (arr_type);
+
+ if (!warn_sizeof_array_div
+ /* Don't warn on multidimensional arrays. */
+ || TREE_CODE (elt_type) == ARRAY_TYPE)
+ return;
+
+ if (!tree_int_cst_equal (TYPE_SIZE (elt_type), TYPE_SIZE (type1)))
+ {
+ auto_diagnostic_group d;
+ if (warning_at (loc, OPT_Wsizeof_array_div,
+ "expression does not compute the number of "
+ "elements in this array; element type is "
+ "%qT, not %qT", elt_type, type1))
+ {
+ if (EXPR_HAS_LOCATION (op1))
+ {
+ location_t op1_loc = EXPR_LOCATION (op1);
+ gcc_rich_location richloc (op1_loc);
+ richloc.add_fixit_insert_before (op1_loc, "(");
+ richloc.add_fixit_insert_after (op1_loc, ")");
+ inform (&richloc, "add parentheses around %qE to "
+ "silence this warning", op1);
+ }
+ else
+ inform (loc, "add parentheses around the second %<sizeof%> "
+ "to silence this warning");
+ if (DECL_P (arr))
+ inform (DECL_SOURCE_LOCATION (arr), "array %qD declared here", arr);
+ }
+ }
+}
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index c1d8fd336e8..9ab44550140 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -799,6 +799,11 @@ Wsizeof-pointer-div
C ObjC C++ ObjC++ Var(warn_sizeof_pointer_div) Warning LangEnabledBy(C ObjC
C++ ObjC++,Wall)
Warn about suspicious divisions of two sizeof expressions that don't work
correctly with pointers.
+Wsizeof-array-div
+C ObjC C++ ObjC++ Var(warn_sizeof_array_div) Warning LangEnabledBy(C ObjC C++
ObjC++,Wall)
+Warn about divisions of two sizeof operators when the first one is applied
+to an array and the divisor does not equal the size of the array element.
+
Wsizeof-pointer-memaccess
C ObjC C++ ObjC++ Var(warn_sizeof_pointer_memaccess) Warning LangEnabledBy(C
ObjC C++ ObjC++,Wall)
Warn about suspicious length parameters to certain string functions if the
argument uses sizeof.
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index a8bc301ffad..0510a9b9fdc 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -7888,7 +7888,7 @@ c_parser_binary_expression (c_parser *parser, struct
c_expr *after,
c_inhibit_evaluation_warnings -= (stack[sp - 1].expr.value \
== truthvalue_true_node); \
break; \
- case TRUNC_DIV_EXPR: \
+ case TRUNC_DIV_EXPR: \
if (stack[sp - 1].expr.original_code == SIZEOF_EXPR \
&& stack[sp].expr.original_code == SIZEOF_EXPR) \
{ \
@@ -7904,18 +7904,23 @@ c_parser_binary_expression (c_parser *parser, struct
c_expr *after,
&& !(TREE_CODE (first_arg) == PARM_DECL \
&& C_ARRAY_PARAMETER (first_arg) \
&& warn_sizeof_array_argument)) \
- { \
- auto_diagnostic_group d;
\
- if (warning_at (stack[sp].loc, OPT_Wsizeof_pointer_div, \
- "division %<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"); \
- } \
- } \
+ { \
+ auto_diagnostic_group d; \
+ 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"); \
+ } \
+ else if (TREE_CODE (type0) == ARRAY_TYPE \
+ && !char_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (type0))) \
+ && !stack[sp].expr.parenthesized_p) \
+ maybe_warn_sizeof_array_div (stack[sp].loc, first_arg, type0, \
+ stack[sp].sizeof_arg, type1); \
+ } \
break; \
default: \
break; \
@@ -9168,6 +9173,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 == SIZEOF_EXPR)
+ expr.parenthesized_p = true;
if (expr.original_code != C_MAYBE_CONST_EXPR
&& expr.original_code != SIZEOF_EXPR)
expr.original_code = ERROR_MARK;
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index 10938cf0857..4ef235fca41 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -147,6 +147,11 @@ struct c_expr
etc), so we stash a copy here. */
source_range src_range;
+ /* True iff the sizeof expression was enclosed in parentheses.
+ NB: This member is currently only initialized when .original_code
+ is a SIZEOF_EXPR. ??? Add a default constructor to this class. */
+ bool parenthesized_p;
+
/* Access to the first and last locations within the source spelling
of this expression. */
location_t get_start () const { return src_range.m_start; }
@@ -664,6 +669,7 @@ extern location_t c_last_sizeof_loc;
extern struct c_switch *c_switch_stack;
+extern bool char_type_p (tree);
extern tree c_objc_common_truthvalue_conversion (location_t, tree);
extern tree require_complete_type (location_t, tree);
extern bool same_translation_unit_p (const_tree, const_tree);
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index bb27099bfe1..ac8690d2fae 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -2938,6 +2938,7 @@ c_expr_sizeof_expr (location_t loc, struct c_expr expr)
c_last_sizeof_loc = loc;
ret.original_code = SIZEOF_EXPR;
ret.original_type = NULL;
+ ret.parenthesized_p = false;
if (c_vla_type_p (TREE_TYPE (folded_expr)))
{
/* sizeof is evaluated when given a vla (C99 6.5.3.4p2). */
@@ -2968,6 +2969,7 @@ c_expr_sizeof_type (location_t loc, struct c_type_name *t)
c_last_sizeof_loc = loc;
ret.original_code = SIZEOF_EXPR;
ret.original_type = NULL;
+ ret.parenthesized_p = false;
if ((type_expr || TREE_CODE (ret.value) == INTEGER_CST)
&& c_vla_type_p (type))
{
@@ -3719,7 +3721,7 @@ parser_build_unary_op (location_t loc, enum tree_code
code, struct c_expr arg)
/* Returns true if TYPE is a character type, *not* including wchar_t. */
-static bool
+bool
char_type_p (tree type)
{
return (type == char_type_node
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 9166156a5d5..7705f15c25f 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -4706,14 +4706,13 @@ cp_build_binary_op (const op_location_t &location,
{
tree type0 = TREE_OPERAND (op0, 0);
tree type1 = TREE_OPERAND (op1, 0);
- tree first_arg = type0;
+ tree first_arg = tree_strip_any_location_wrapper (type0);
if (!TYPE_P (type0))
type0 = TREE_TYPE (type0);
if (!TYPE_P (type1))
type1 = TREE_TYPE (type1);
if (INDIRECT_TYPE_P (type0) && same_type_p (TREE_TYPE (type0), type1))
{
- STRIP_ANY_LOCATION_WRAPPER (first_arg);
if (!(TREE_CODE (first_arg) == PARM_DECL
&& DECL_ARRAY_PARAMETER_P (first_arg)
&& warn_sizeof_array_argument)
@@ -4729,6 +4728,13 @@ cp_build_binary_op (const op_location_t &location,
"first %<sizeof%> operand was declared here");
}
}
+ else if (TREE_CODE (type0) == ARRAY_TYPE
+ && !char_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (type0)))
+ /* Set by finish_parenthesized_expr. */
+ && !TREE_NO_WARNING (op1)
+ && (complain & tf_warning))
+ maybe_warn_sizeof_array_div (location, first_arg, type0,
+ op1, non_reference (type1));
}
if ((code0 == INTEGER_TYPE || code0 == REAL_TYPE
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 6d9ff2c3362..97fef872a54 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -362,6 +362,7 @@ Objective-C and Objective-C++ Dialects}.
-Wno-shift-overflow -Wshift-overflow=@var{n} @gol
-Wsign-compare -Wsign-conversion @gol
-Wno-sizeof-array-argument @gol
+-Wsizeof-array-div @gol
-Wsizeof-pointer-div -Wsizeof-pointer-memaccess @gol
-Wstack-protector -Wstack-usage=@var{byte-size} -Wstrict-aliasing @gol
-Wstrict-aliasing=n -Wstrict-overflow -Wstrict-overflow=@var{n} @gol
@@ -5255,6 +5256,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect
Options}.
-Wreturn-type @gol
-Wsequence-point @gol
-Wsign-compare @r{(only in C++)} @gol
+-Wsizeof-array-div @gol
-Wsizeof-pointer-div @gol
-Wsizeof-pointer-memaccess @gol
-Wstrict-aliasing @gol
@@ -7947,6 +7949,23 @@ real to lower precision real values. This option is
also enabled by
@opindex Wscalar-storage-order
Do not warn on suspicious constructs involving reverse scalar storage order.
+@item -Wsizeof-array-div
+@opindex Wsizeof-array-div
+@opindex Wno-sizeof-array-div
+Warn about divisions of two sizeof operators when the first one is applied
+to an array and the divisor does not equal the size of the array element.
+In such a case, the computation will not yield the number of elements in the
+array, which is likely what the user intended. This warning warns e.g. about
+@smallexample
+int fn ()
+@{
+ int arr[10];
+ return sizeof (arr) / sizeof (short);
+@}
+@end smallexample
+
+This warning is enabled by @option{-Wall}.
+
@item -Wsizeof-pointer-div
@opindex Wsizeof-pointer-div
@opindex Wno-sizeof-pointer-div
diff --git a/gcc/testsuite/c-c++-common/Wsizeof-array-div1.c
b/gcc/testsuite/c-c++-common/Wsizeof-array-div1.c
new file mode 100644
index 00000000000..84d9a730cba
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wsizeof-array-div1.c
@@ -0,0 +1,56 @@
+/* PR c++/91741 */
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+typedef int T;
+
+int
+fn (int ap[])
+{
+ int arr[10];
+ int *arr2[10];
+ int *p = &arr[0];
+ int r = 0;
+
+ r += sizeof (arr) / sizeof (*arr);
+ r += sizeof (arr) / sizeof (p); /* { dg-warning "expression does not
compute" } */
+ r += sizeof (arr) / sizeof p; /* { dg-warning "expression does not compute"
} */
+ r += sizeof (arr) / (sizeof p);
+ r += sizeof (arr) / (sizeof (p));
+ r += sizeof (arr2) / sizeof p;
+ r += sizeof (arr2) / sizeof (int); /* { dg-warning "expression does not
compute" } */
+ r += sizeof (arr2) / sizeof (int *);
+ r += sizeof (arr2) / sizeof (short *);
+ r += sizeof (arr) / sizeof (int);
+ r += sizeof (arr) / sizeof (unsigned int);
+ r += sizeof (arr) / sizeof (T);
+ r += sizeof (arr) / sizeof (short); /* { dg-warning "expression does not
compute" } */
+ r += sizeof (arr) / (sizeof (short));
+
+ r += sizeof (ap) / sizeof (char); /* { dg-warning ".sizeof. on array
function parameter" } */
+
+ const char arr3[] = "foo";
+ r += sizeof (arr3) / sizeof(char);
+ r += sizeof (arr3) / sizeof(int);
+ r += sizeof (arr3) / sizeof (*arr3);
+
+ int arr4[5][5];
+ r += sizeof (arr4) / sizeof (arr4[0]);
+ r += sizeof (arr4) / sizeof (*arr4);
+ r += sizeof (arr4) / sizeof (**arr4);
+ r += sizeof (arr4) / sizeof (int *);
+ r += sizeof (arr4) / sizeof (int);
+ r += sizeof (arr4) / sizeof (short int);
+
+ T arr5[10];
+ r += sizeof (arr5) / sizeof (T);
+ r += sizeof (arr5) / sizeof (int);
+ r += sizeof (arr5) / sizeof (short); /* { dg-warning "expression does not
compute" } */
+
+ double arr6[10];
+ r += sizeof (arr6) / sizeof (double);
+ r += sizeof (arr6) / sizeof (float); /* { dg-warning "expression does not
compute" } */
+ r += sizeof (arr6) / sizeof (*arr6);
+
+ return r;
+}
diff --git a/gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c
b/gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c
index 83116183902..e9bad1fa420 100644
--- a/gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c
+++ b/gcc/testsuite/c-c++-common/Wsizeof-pointer-div.c
@@ -29,7 +29,7 @@ f2 (void)
i += sizeof(array) / sizeof(array[0]);
i += (sizeof(array)) / (sizeof(array[0]));
i += sizeof(array) / sizeof(int);
- i += sizeof(array) / sizeof(char);
+ i += sizeof(array) / sizeof(char); /* { dg-warning "expression
does not compute" } */
i += sizeof(*array) / sizeof(char);
i += sizeof(array[0]) / sizeof(char);
return i;
diff --git a/gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C
b/gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C
new file mode 100644
index 00000000000..da220cd57ba
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wsizeof-array-div1.C
@@ -0,0 +1,37 @@
+// PR c++/91741
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wall" }
+
+int
+fn1 ()
+{
+ int arr[10];
+ return sizeof (arr) / sizeof (decltype(arr[0]));
+}
+
+template<typename T, int N>
+int fn2 (T (&arr)[N])
+{
+ return sizeof (arr) / sizeof (T);
+}
+
+template<typename T, int N>
+int fn3 (T (&arr)[N])
+{
+ return sizeof (arr) / sizeof (bool); // { dg-warning "expression does not
compute" }
+}
+
+template<typename U, int N, typename T>
+int fn4 (T (&arr)[N])
+{
+ return sizeof (arr) / sizeof (U); // { dg-warning "expression does not
compute" }
+}
+
+void
+fn ()
+{
+ int arr[10];
+ fn2 (arr);
+ fn3 (arr);
+ fn4<short> (arr);
+}
diff --git a/gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C
b/gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C
new file mode 100644
index 00000000000..7962c23522c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wsizeof-array-div2.C
@@ -0,0 +1,15 @@
+// PR c++/91741
+// { dg-do compile }
+// { dg-options "-Wall" }
+// From <https://www.viva64.com/en/examples/v706/>.
+
+const int kBaudrates[] = { 50, 75, 110 };
+
+void
+foo ()
+{
+ for(int i = sizeof(kBaudrates) / sizeof(char*); // { dg-warning "expression
does not compute" }
+ --i >= 0;)
+ {
+ }
+}
base-commit: 0620f4d79e270f1a455a7ec099504d44dc6180e6
--
2.26.2
