diff mbox

C, C++: New warning for memset without multiply by elt size

Message ID 571A2776.6060502@t-online.de
State New
Headers show

Commit Message

Bernd Schmidt April 22, 2016, 1:30 p.m. UTC
We had this problem in the C frontend until very recently:

   int array[some_count];
   memset (array, 0, some_count);

which forgets to multiply by sizeof int. The following patch implements 
a new warning option for this.

Bootstrapped and tested (a while ago, will retest) on x86_64-linux. Ok 
for trunk?


Bernd

Comments

Jason Merrill April 22, 2016, 1:57 p.m. UTC | #1
This looks good, but can we move the code into c-common rather than
duplicate it?

Jason


On Fri, Apr 22, 2016 at 9:30 AM, Bernd Schmidt <bernds_cb1@t-online.de> wrote:
> We had this problem in the C frontend until very recently:
>
>   int array[some_count];
>   memset (array, 0, some_count);
>
> which forgets to multiply by sizeof int. The following patch implements a
> new warning option for this.
>
> Bootstrapped and tested (a while ago, will retest) on x86_64-linux. Ok for
> trunk?
>
>
> Bernd
Bernd Schmidt April 22, 2016, 3:24 p.m. UTC | #2
On 04/22/2016 03:57 PM, Jason Merrill wrote:
> This looks good, but can we move the code into c-common rather than
> duplicate it?

Probably not without a fair amount of surgery, since the cdw_ and ds_ 
codes are private to each frontend.


Bernd
Jason Merrill April 22, 2016, 3:31 p.m. UTC | #3
On Fri, Apr 22, 2016 at 11:24 AM, Bernd Schmidt <bernds_cb1@t-online.de> wrote:
> On 04/22/2016 03:57 PM, Jason Merrill wrote:
>> This looks good, but can we move the code into c-common rather than
>> duplicate it?
>
> Probably not without a fair amount of surgery, since the cdw_ and ds_ codes
> are private to each frontend.

I don't see any cdw_ or ds_ in this patch.

Jason
Bernd Schmidt April 22, 2016, 3:57 p.m. UTC | #4
On 04/22/2016 05:31 PM, Jason Merrill wrote:
> On Fri, Apr 22, 2016 at 11:24 AM, Bernd Schmidt <bernds_cb1@t-online.de> wrote:
>> On 04/22/2016 03:57 PM, Jason Merrill wrote:
>>> This looks good, but can we move the code into c-common rather than
>>> duplicate it?
>>
>> Probably not without a fair amount of surgery, since the cdw_ and ds_ codes
>> are private to each frontend.
>
> I don't see any cdw_ or ds_ in this patch.

Ah sorry, that was the other patch. I'm somewhat jetlagged. I'll look 
into this; I do remember there being some differences in the C and C++ 
parts, but it may just be the bit where it looks past a CONST_DECL for C++.


Bernd
diff mbox

Patch

	* doc/invoke.texi (Warning Options): Add -Wmemset-elt-size.
	(-Wmemset-elt-size): New item.
c-family/
	* c.opt (Wmemset-elt-size): New option.
c/
	* c-parser.c (c_parser_postfix_expression_after_primary): Warn for
	memset with element count rather than array size.
cp/
	* parser.c (cp_parser_postfix_expression): Warn for
	memset with element count rather than array size.
testsuite/
	* c-c++-common/memset-array.c: New test.

Index: gcc/c/c-parser.c
===================================================================
--- gcc/c/c-parser.c	(revision 234183)
+++ gcc/c/c-parser.c	(working copy)
@@ -8240,18 +8240,46 @@  c_parser_postfix_expression_after_primar
 					      expr.value, exprlist,
 					      sizeof_arg,
 					      sizeof_ptr_memacc_comptypes);
-	  if (warn_memset_transposed_args
-	      && TREE_CODE (expr.value) == FUNCTION_DECL
+	  if (TREE_CODE (expr.value) == FUNCTION_DECL
 	      && DECL_BUILT_IN_CLASS (expr.value) == BUILT_IN_NORMAL
 	      && DECL_FUNCTION_CODE (expr.value) == BUILT_IN_MEMSET
-	      && vec_safe_length (exprlist) == 3
-	      && integer_zerop ((*exprlist)[2])
-	      && (literal_zero_mask & (1 << 2)) != 0
-	      && (!integer_zerop ((*exprlist)[1])
-		  || (literal_zero_mask & (1 << 1)) == 0))
-	    warning_at (expr_loc, OPT_Wmemset_transposed_args,
-			"%<memset%> used with constant zero length parameter; "
-			"this could be due to transposed parameters");
+	      && vec_safe_length (exprlist) == 3)
+	    {
+	      if (warn_memset_transposed_args
+		  && integer_zerop ((*exprlist)[2])
+		  && (literal_zero_mask & (1 << 2)) != 0
+		  && (!integer_zerop ((*exprlist)[1])
+		      || (literal_zero_mask & (1 << 1)) == 0))
+		warning_at (expr_loc, OPT_Wmemset_transposed_args,
+			    "%<memset%> used with constant zero length "
+			    "parameter; this could be due to transposed "
+			    "parameters");
+	      if (warn_memset_elt_size
+		  && TREE_CODE ((*exprlist)[2]) == INTEGER_CST)
+		{
+		  tree ptr = (*exprlist)[0];
+		  STRIP_NOPS (ptr);
+		  if (TREE_CODE (ptr) == ADDR_EXPR)
+		    ptr = TREE_OPERAND (ptr, 0);
+		  tree type = TREE_TYPE (ptr);
+		  if (TREE_CODE (type) == ARRAY_TYPE)
+		    {
+		      tree elt_type = TREE_TYPE (type);
+		      tree domain = TYPE_DOMAIN (type);
+		      if (!integer_onep (TYPE_SIZE_UNIT (elt_type))
+			  && TYPE_MAXVAL (domain)
+			  && TYPE_MINVAL (domain)
+			  && integer_zerop (TYPE_MINVAL (domain))
+			  && integer_onep (fold_build2 (MINUS_EXPR, domain,
+							(*exprlist)[2],
+							TYPE_MAXVAL (domain))))
+		      warning_at (expr_loc, OPT_Wmemset_elt_size,
+				  "%<memset%> used with length equal to "
+				  "number of elements without multiplication "
+				  "with element size");
+		    }
+		}
+	    }
 
 	  start = expr.get_start ();
 	  finish = parser->tokens_buf[0].get_finish ();
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 234183)
+++ gcc/cp/parser.c	(working copy)
@@ -6829,20 +6829,48 @@  cp_parser_postfix_expression (cp_parser
 		  }
 	      }
 
-	    if (warn_memset_transposed_args)
+	    if (TREE_CODE (postfix_expression) == FUNCTION_DECL
+		&& DECL_BUILT_IN_CLASS (postfix_expression) == BUILT_IN_NORMAL
+		&& DECL_FUNCTION_CODE (postfix_expression) == BUILT_IN_MEMSET
+		&& vec_safe_length (args) == 3)
 	      {
-		if (TREE_CODE (postfix_expression) == FUNCTION_DECL
-		    && DECL_BUILT_IN_CLASS (postfix_expression) == BUILT_IN_NORMAL
-		    && DECL_FUNCTION_CODE (postfix_expression) == BUILT_IN_MEMSET
-		    && vec_safe_length (args) == 3
-		    && TREE_CODE ((*args)[2]) == INTEGER_CST
-		    && integer_zerop ((*args)[2])
-		    && !(TREE_CODE ((*args)[1]) == INTEGER_CST
-			 && integer_zerop ((*args)[1])))
+		tree arg1 = (*args)[1];
+		tree arg2 = (*args)[2];
+		if (TREE_CODE (arg2) == CONST_DECL)
+		  arg2 = DECL_INITIAL (arg2);
+
+		if (warn_memset_transposed_args
+		    && integer_zerop (arg2)
+		    && !(TREE_CODE (arg1) == INTEGER_CST
+			 && integer_zerop (arg1)))
 		  warning (OPT_Wmemset_transposed_args,
 			   "%<memset%> used with constant zero length "
 			   "parameter; this could be due to transposed "
 			   "parameters");
+		if (warn_memset_elt_size && TREE_CODE (arg2) == INTEGER_CST)
+		  {
+		    tree ptr = (*args)[0];
+		    STRIP_NOPS (ptr);
+		    if (TREE_CODE (ptr) == ADDR_EXPR)
+		      ptr = TREE_OPERAND (ptr, 0);
+		    tree type = TREE_TYPE (ptr);
+		    if (TREE_CODE (type) == ARRAY_TYPE)
+		      {
+			tree elt_type = TREE_TYPE (type);
+			tree domain = TYPE_DOMAIN (type);
+			if (!integer_onep (TYPE_SIZE_UNIT (elt_type))
+			    && TYPE_MAXVAL (domain)
+			    && TYPE_MINVAL (domain)
+			    && integer_zerop (TYPE_MINVAL (domain))
+			    && integer_onep (fold_build2 (MINUS_EXPR, domain,
+							  arg2,
+							  TYPE_MAXVAL (domain))))
+			  warning (OPT_Wmemset_elt_size,
+				   "%<memset%> used with length equal to "
+				   "number of elements without multiplication "
+				   "with element size");
+		      }
+		  }
 	      }
 
 	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
Index: gcc/testsuite/c-c++-common/memset-array.c
===================================================================
--- gcc/testsuite/c-c++-common/memset-array.c	(revision 0)
+++ gcc/testsuite/c-c++-common/memset-array.c	(working copy)
@@ -0,0 +1,36 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wmemset-elt-size" } */
+enum a {
+  a_1,
+  a_2,
+  a_n
+};
+int t1[20];
+int t2[a_n];
+
+struct s
+{
+  int t[20];
+};
+
+void foo (struct s *s)
+{
+  __builtin_memset (t1, 0, 20); /* { dg-warning "element size" } */
+  __builtin_memset (t2, 0, a_n); /* { dg-warning "element size" } */
+  __builtin_memset (s->t, 0, 20); /* { dg-warning "element size" } */
+}
+
+char u1[20];
+char u2[a_n];
+
+struct s2
+{
+  char u[20];
+};
+
+void bar (struct s2 *s)
+{
+  __builtin_memset (u1, 0, 20);
+  __builtin_memset (u2, 0, a_n);
+  __builtin_memset (s->u, 0, 20);
+}
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 234183)
+++ gcc/doc/invoke.texi	(working copy)
@@ -273,7 +273,7 @@  Objective-C and Objective-C++ Dialects}.
 -Wno-int-to-pointer-cast -Winvalid-memory-model -Wno-invalid-offsetof @gol
 -Winvalid-pch -Wlarger-than=@var{len} @gol
 -Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol
--Wmain -Wmaybe-uninitialized -Wmemset-transposed-args @gol
+-Wmain -Wmaybe-uninitialized -Wmemset-elt-size -Wmemset-transposed-args @gol
 -Wmisleading-indentation -Wmissing-braces @gol
 -Wmissing-field-initializers -Wmissing-include-dirs @gol
 -Wno-multichar -Wnonnull -Wnonnull-compare @gol
@@ -3547,6 +3547,7 @@  Options} and @ref{Objective-C and Object
 -Wlogical-not-parentheses
 -Wmain @r{(only for C/ObjC and unless} @option{-ffreestanding}@r{)}  @gol
 -Wmaybe-uninitialized @gol
+-Wmemset-elt-size @gol
 -Wmemset-transposed-args @gol
 -Wmisleading-indentation @r{(only for C/C++)} @gol
 -Wmissing-braces @r{(only for C/ObjC)} @gol
@@ -5248,6 +5249,15 @@  Warn when the @code{sizeof} operator is
 declared as an array in a function definition.  This warning is enabled by
 default for C and C++ programs.
 
+@item -Wmemset-elt-size
+@opindex Wmemset-elt-size
+@opindex Wno-memset-elt-size
+Warn for suspicious calls to the @code{memset} built-in function, if the
+first argument references an array, and the third argument is a number
+equal to the number of elements, but not equal to the size of the array
+in memory.  This indicates that the user has omitted a multiplication by
+the element size.  This warning is enabled by @option{-Wall}.
+
 @item -Wmemset-transposed-args
 @opindex Wmemset-transposed-args
 @opindex Wno-memset-transposed-args
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 234183)
+++ gcc/c-family/c.opt	(working copy)
@@ -561,6 +561,10 @@  Wmemset-transposed-args
 C ObjC C++ ObjC++ Var(warn_memset_transposed_args) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
 Warn about suspicious calls to memset where the third argument is constant literal zero and the second is not.
 
+Wmemset-elt-size
+C ObjC C++ ObjC++ Var(warn_memset_elt_size) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn about suspicious calls to memset where the third argument contains the number of elements not multiplied by the element size.
+
 Wmisleading-indentation
 C C++ Common Var(warn_misleading_indentation) Warning LangEnabledBy(C C++,Wall)
 Warn when the indentation of the code does not reflect the block structure.