Re: PATCH: warn on memset with constant length 0

[Assar <assar at kth dot se>]

"Manuel López-Ibáñez" <lopezibanez@gmail.com> writes:
> On 15 Dec 2006 08:39:48 -0500, Assar <assar@kth.se> wrote:
> 
> When creating the patch, you should use the -p option of diff, please.
> It helps a lot when reviewing the patch.

I was not able to get my version of svn to send that to diff - I will
need to investigate more why that's the case.

> I am not a maintainer and I cannot approve your patch but I hope you
> find the following comments useful.

Thanks for your feedback.

> > Index: gcc/testsuite/gcc.dg/memset-constant0-3.c
> > ===================================================================
> > --- gcc/testsuite/gcc.dg/memset-constant0-3.c   (revision 0)
> > +++ gcc/testsuite/gcc.dg/memset-constant0-3.c   (revision 0)
> > @@ -0,0 +1,15 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-Wextra -O3" } */
> > +
> 
> Why use Wextra here? Is not Wmemset enabled by Wall ? Nevertheless, I
> think that the appropriate is to use Wmemset directly or even -Wextra
> -Wall -Wmemset all together.

Yeah, I think it should just be -Wmemset.  See patch below.

> Index: gcc/c.opt
> > +Wmemset
> > +C objC C++ ObjC++
> > +Warn about possible bad uses of \"memset\"
> > +
> 
> Why don't you use this instead ?
> 
> Wmemset
> C ObjC C++ ObjC++ Var(warn_memset) Init(-1)
> Warn about possible bad uses of \"memset\"

That makes perfect sense (without the Init(-1) - as you were
suggesting in your second mail).

> With the above changes, I think that the following are unnecessary:

Yeah.

> >  /* Warn about using __null (as NULL in C++) as sentinel.  For code compiled
> >     with GCC this doesn't matter as __null is guaranteed to have the right
> >     size.  */
> > @@ -5827,6 +5831,30 @@
> >    return NULL_TREE;
> >  }
> >
> > +/* Check arguments passed to memset.  */
> > +static void
> > +check_function_memset (tree params)
> > +{
> > +  tree param;
> > +  int param_num;
> > +
> > +  for (param = params, param_num = 1;
> > +       param;
> > +       param_num++, param = TREE_CHAIN (param))
> > +    if (param_num == 3 && integer_zerop (TREE_VALUE (param)))
> > +      warning (OPT_Wextra, "memset called with a constant 0 length");
> > +}
> > +
> 
> You have to use OPT_Wmemset instead of OPT_Wextra.

Good catch.

> Apart from that, is there no way to access the third argument
> directly? That "for" gives me the creeps.

I didn't find any other good examples in the code around there.

A patch with your suggestions incorporated is below.

/assar

gcc/ChangeLog

2006-12-15  Assar  <assar@permabit.com>

	* c-common.h: Add check_builtin_arguments prototype.
	* c-opts.c: Set -Wmemset in -Wall.
	
	* c-common.c: Add check for calling memset with constant third
	argument of 0.

	* c-typeck.c (build_function_call): Call check_builtin_arguments
	to get warnings.

	* c.opt: Add -Wmemset.

	* doc/invoke.texi: Document -Wmemset.

gcc/testsuite/ChangeLog

2006-12-15  Assar Westerlund  <assar@kth.se>

	* gcc.dg/memset-constant0-1.c: Test for "memset with 0" warning.
	* gcc.dg/memset-constant0-2.c: Likewise.
	* gcc.dg/memset-constant0-3.c: Test for not getting "memset with
	0" warning.

gcc/cp/ChangeLog

2006-12-15  Assar  <assar@permabit.com>

	* typeck.c (build_function_call): Call check_builtin_arguments to
	get warnings.
	* call.c (build_over_call): Same.

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 119393)
+++ gcc/doc/invoke.texi	(working copy)
@@ -234,7 +234,7 @@
 -Wno-int-to-pointer-cast @gol
 -Wno-invalid-offsetof  -Winvalid-pch @gol
 -Wlarger-than-@var{len}  -Wunsafe-loop-optimizations  -Wlong-long @gol
--Wmain  -Wmissing-braces  -Wmissing-field-initializers @gol
+-Wmain  -Wmemset  -Wmissing-braces  -Wmissing-field-initializers @gol
 -Wmissing-format-attribute  -Wmissing-include-dirs @gol
 -Wmissing-noreturn @gol
 -Wno-multichar  -Wnonnull  -Wno-overflow @gol
@@ -2507,6 +2507,12 @@
 arguments, two, or three arguments of appropriate types.
 This warning is enabled by @option{-Wall}.
 
+@item -Wmemset
+@opindex -Wmemset
+Warn if the third argument to @samp{memset} is a constant 0.  This is
+usually caused by swapping arguments two and three.
+This warning is enabled by @option{-Wall}.
+
 @item -Wmissing-braces
 @opindex Wmissing-braces
 Warn if an aggregate or union initializer is not fully bracketed.  In
Index: gcc/testsuite/gcc.dg/memset-constant0-2.c
===================================================================
--- gcc/testsuite/gcc.dg/memset-constant0-2.c	(revision 0)
+++ gcc/testsuite/gcc.dg/memset-constant0-2.c	(revision 0)
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-Wmemset" } */
+
+typedef unsigned size_t;
+void *memset(void *s, int c, size_t n);
+
+void clear(void *p, size_t len)
+{
+  memset(p, len, 0); /* { dg-warning "memset called with a constant 0 length" } */
+}
Index: gcc/testsuite/gcc.dg/memset-constant0-1.c
===================================================================
--- gcc/testsuite/gcc.dg/memset-constant0-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/memset-constant0-1.c	(revision 0)
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-Wmemset" } */
+
+typedef unsigned size_t;
+void *memset(void *s, int c, size_t n);
+
+int main()
+{
+  int foo;
+  memset(&foo, sizeof(foo), 0); /* { dg-warning "memset called with a constant 0 length" } */
+  return foo;
+}
Index: gcc/testsuite/gcc.dg/memset-constant0-3.c
===================================================================
--- gcc/testsuite/gcc.dg/memset-constant0-3.c	(revision 0)
+++ gcc/testsuite/gcc.dg/memset-constant0-3.c	(revision 0)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-Wmemset -O3" } */
+
+typedef unsigned size_t;
+void *memset(void *s, int c, size_t n);
+
+static inline void f(int *a, int size)
+{
+  memset(a, 0, size);
+}
+
+void g(void)
+{
+  f(0, 0);
+}
Index: gcc/cp/typeck.c
===================================================================
--- gcc/cp/typeck.c	(revision 119393)
+++ gcc/cp/typeck.c	(working copy)
@@ -2691,6 +2691,8 @@
   check_function_arguments (TYPE_ATTRIBUTES (fntype), coerced_params,
 			    TYPE_ARG_TYPES (fntype));
 
+  check_builtin_arguments (fndecl, coerced_params);
+
   return build_cxx_call (function, coerced_params);
 }
 
Index: gcc/cp/call.c
===================================================================
--- gcc/cp/call.c	(revision 119393)
+++ gcc/cp/call.c	(working copy)
@@ -4889,6 +4889,8 @@
   check_function_arguments (TYPE_ATTRIBUTES (TREE_TYPE (fn)),
 			    converted_args, TYPE_ARG_TYPES (TREE_TYPE (fn)));
 
+  check_builtin_arguments (fn, converted_args);
+
   /* Avoid actually calling copy constructors and copy assignment operators,
      if possible.  */
 
Index: gcc/c.opt
===================================================================
--- gcc/c.opt	(revision 119393)
+++ gcc/c.opt	(working copy)
@@ -255,6 +255,10 @@
 C ObjC
 Warn about suspicious declarations of \"main\"
 
+Wmemset
+C ObjC C++ ObjC++ Var(warn_memset)
+Warn about possible bad uses of \"memset\"
+
 Wmissing-braces
 C ObjC C++ ObjC++ Var(warn_missing_braces)
 Warn about possibly missing braces around initializers
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 119393)
+++ gcc/c-typeck.c	(working copy)
@@ -2328,6 +2328,8 @@
   check_function_arguments (TYPE_ATTRIBUTES (fntype), coerced_params,
 			    TYPE_ARG_TYPES (fntype));
 
+  check_builtin_arguments (fundecl, coerced_params);
+
   if (require_constant_value)
     {
       result = fold_build3_initializer (CALL_EXPR, TREE_TYPE (fntype),
Index: gcc/c-opts.c
===================================================================
--- gcc/c-opts.c	(revision 119393)
+++ gcc/c-opts.c	(working copy)
@@ -385,6 +385,7 @@
       set_Wunused (value);
       set_Wformat (value);
       set_Wimplicit (value);
+      warn_memset = value;
       warn_char_subscripts = value;
       warn_missing_braces = value;
       warn_parentheses = value;
Index: gcc/c-common.c
===================================================================
--- gcc/c-common.c	(revision 119393)
+++ gcc/c-common.c	(working copy)
@@ -5827,6 +5827,30 @@
   return NULL_TREE;
 }
 
+/* Check arguments passed to memset.  */
+static void
+check_function_memset (tree params)
+{
+  tree param;
+  int param_num;
+
+  for (param = params, param_num = 1;
+       param;
+       param_num++, param = TREE_CHAIN (param))
+    if (param_num == 3 && integer_zerop (TREE_VALUE (param)))
+      warning (OPT_Wmemset, "memset called with a constant 0 length");
+}
+
+void
+check_builtin_arguments (tree fundecl, tree params)
+{
+  if (!fundecl || !DECL_BUILT_IN(fundecl))
+    return;
+
+  if (warn_memset && DECL_FUNCTION_CODE(fundecl) == BUILT_IN_MEMSET)
+    check_function_memset (params);
+}
+
 /* Check for valid arguments being passed to a function.  */
 void
 check_function_arguments (tree attrs, tree params, tree typelist)
Index: gcc/c-common.h
===================================================================
--- gcc/c-common.h	(revision 119393)
+++ gcc/c-common.h	(working copy)
@@ -634,6 +634,7 @@
 extern const char *fname_as_string (int);
 extern tree fname_decl (unsigned, tree);
 
+extern void check_builtin_arguments (tree, tree);
 extern void check_function_arguments (tree, tree, tree);
 extern void check_function_arguments_recurse (void (*)
 					      (void *, tree,