This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [RFC PATCH] -Wmemset-transposed-args (PR middle-end/61294)


I don't think we want to warn about e.g. 1-1, only about literal 0.

-------- Original Message --------
 From: Jakub Jelinek <jakub@redhat.com>
 Sent: Tue, Jul 8, 2014 05:50 AM
 To: Joseph S. Myers <joseph@codesourcery.com>; Jason Merrill <jason@redhat.com>; Carlos O'Donell <carlos@redhat.com>; Siddhesh Poyarekar <siddhesh@redhat.com>
 CC: gcc-patches@gcc.gnu.org; libc-alpha@sourceware.org
 Subject: [RFC PATCH] -Wmemset-transposed-args (PR middle-end/61294)

Hi!

This is an attempt to move the warning about transposed memset arguments
from the glibc headers to gcc FEs.  The problem with the warning in glibc
is that it uses __builtin_constant_p and e.g. jump threading very often
makes the warning trigger even on code where it is very unlikely a user
swapped arguments.  See e.g.
https://gcc.gnu.org/PR51744
https://gcc.gnu.org/PR56977
https://gcc.gnu.org/PR61294
https://bugzilla.redhat.com/452219
https://bugs.kde.org/show_bug.cgi?id=311098
https://bugzilla.mozilla.org/show_bug.cgi?id=581227
and many others.  Thus, I'd like to warn in the FEs instead, and
once we have a GCC release with that warning in, disable the glibc
bits/string3.h:
  if (__builtin_constant_p (__len) && __len == 0
      && (!__builtin_constant_p (__ch) || __ch != 0))
    {
      __warn_memset_zero_len ();
      return __dest;
    }
warning for GCC versions with that new warning in.

Any thoughts on this?

If you are ok with it, either we can add it only for 4.10/5.0 and
later only, or perhaps 4.9.2 too, or even 4.9.1.  For -D_FORTIFY_SOURCE=2
built code with glibc it shouldn't make a difference (other than having
fewer false positives), but for other non-fortified -Wall compilation
it would make a difference (introducing new warnings), so perhaps
doing it only for 4.10/5.0+ is best.

2014-07-08  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/61294
gcc/c-family/
	* c.opt (Wmemset-transposed-args): New warning.
gcc/c/
	* c-typeck.c (c_build_function_call_vec): Handle
	-Wmemset-transposed-args.
gcc/cp/
	* semantics.c (finish_call_expr): Handle -Wmemset-transposed-args.
gcc/
	* doc/invoke.texi (-Wmemset-transposed-args): Document.
gcc/testsuite/
	* c-c++-common/Wmemset-transposed-args1.c: New test.
	* g++.dg/warn/Wmemset-transposed-args-1.C: New test.

--- gcc/c-family/c.opt.jj	2014-07-07 10:39:43.000000000 +0200
+++ gcc/c-family/c.opt	2014-07-08 13:12:07.755536537 +0200
@@ -518,6 +518,10 @@ Wmain
 LangEnabledBy(C ObjC C++ ObjC++,Wpedantic, 2, 0)
 ;
 
+Wmemset-transposed-args
+C ObjC C++ ObjC++ Var(warn_memset_transposed_args) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn about suspicious call to memset where the third argument is constant zero and second is not zero
+
 Wmissing-braces
 C ObjC C++ ObjC++ Var(warn_missing_braces) Warning LangEnabledBy(C ObjC,Wall)
 Warn about possibly missing braces around initializers
--- gcc/c/c-typeck.c.jj	2014-07-07 10:39:43.000000000 +0200
+++ gcc/c/c-typeck.c	2014-07-08 13:22:36.846564329 +0200
@@ -2987,6 +2987,16 @@ c_build_function_call_vec (location_t lo
   /* Convert anything with function type to a pointer-to-function.  */
   if (TREE_CODE (function) == FUNCTION_DECL)
     {
+      if (warn_memset_transposed_args
+	  && DECL_BUILT_IN_CLASS (function) == BUILT_IN_NORMAL
+	  && DECL_FUNCTION_CODE (function) == BUILT_IN_MEMSET
+	  && vec_safe_length (params) == 3
+	  && integer_zerop ((*params)[2])
+	  && !integer_zerop ((*params)[1]))
+	warning_at (loc, OPT_Wmemset_transposed_args,
+		    "%<memset%> used with constant zero length parameter; "
+		    "this could be due to transposed parameters");
+
       /* Implement type-directed function overloading for builtins.
 	 resolve_overloaded_builtin and targetm.resolve_overloaded_builtin
 	 handle all the type checking.  The result is a complete expression
--- gcc/cp/semantics.c.jj	2014-07-02 09:04:13.000000000 +0200
+++ gcc/cp/semantics.c	2014-07-08 14:02:45.936782580 +0200
@@ -2361,6 +2361,18 @@ finish_call_expr (tree fn, vec<tree, va_
 		 sizeof_arg, same_type_ignoring_top_level_qualifiers_p);
 	    }
 
+	  if (warn_memset_transposed_args
+	      && !processing_template_decl
+	      && TREE_CODE (fn) == FUNCTION_DECL
+	      && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL
+	      && DECL_FUNCTION_CODE (fn) == BUILT_IN_MEMSET
+	      && vec_safe_length (*args) == 3
+	      && integer_zerop ((**args)[2])
+	      && !integer_zerop ((**args)[1]))
+	    warning (OPT_Wmemset_transposed_args,
+		     "%<memset%> used with constant zero length parameter; "
+		     "this could be due to transposed parameters");
+
 	  /* A call to a namespace-scope function.  */
 	  result = build_new_function_call (fn, args, koenig_p, complain);
 	}
--- gcc/doc/invoke.texi.jj	2014-07-08 11:36:14.000000000 +0200
+++ gcc/doc/invoke.texi	2014-07-08 14:20:09.932799699 +0200
@@ -257,8 +257,8 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-int-to-pointer-cast -Wno-invalid-offsetof @gol
 -Winvalid-pch -Wlarger-than=@var{len}  -Wunsafe-loop-optimizations @gol
 -Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol
--Wmain -Wmaybe-uninitialized -Wmissing-braces  -Wmissing-field-initializers @gol
--Wmissing-include-dirs @gol
+-Wmain -Wmaybe-uninitialized -Wmemset-transposed-args  -Wmissing-braces @gol
+-Wmissing-field-initializers -Wmissing-include-dirs @gol
 -Wno-multichar  -Wnonnull  -Wno-overflow -Wopenmp-simd @gol
 -Woverlength-strings  -Wpacked  -Wpacked-bitfield-compat  -Wpadded @gol
 -Wparentheses  -Wpedantic-ms-format -Wno-pedantic-ms-format @gol
@@ -4683,6 +4683,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-transposed-args
+@opindex Wmemset-transposed-args
+@opindex Wno-memset-transposed-args
+Warn for suspicious calls to the memset built-in function, if the
+second argument is not zero and third argument is zero.  This warns e.g.@
+about @code{memset (buf, sizeof buf, 0);} where most probably
+@code{memset (buf, 0, sizeof buf);} was meant instead.  This warning is
+enabled by @option{-Wall}.
+
 @item -Waddress
 @opindex Waddress
 @opindex Wno-address
--- gcc/testsuite/c-c++-common/Wmemset-transposed-args1.c.jj	2014-07-08 13:46:19.381765644 +0200
+++ gcc/testsuite/c-c++-common/Wmemset-transposed-args1.c	2014-07-08 13:46:14.868798956 +0200
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+extern
+#ifdef __cplusplus
+"C"
+#endif
+void *memset (void *, int, size_t);
+char buf[1024];
+
+void
+foo ()
+{
+  memset (buf, sizeof buf, 0);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 1, 1 - 1);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 0, 0);
+  memset (buf, 1 - 1, 0);
+}
--- gcc/testsuite/g++.dg/warn/Wmemset-transposed-args-1.C.jj	2014-07-08 13:50:22.685624346 +0200
+++ gcc/testsuite/g++.dg/warn/Wmemset-transposed-args-1.C	2014-07-08 13:51:59.837968475 +0200
@@ -0,0 +1,25 @@
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+typedef __SIZE_TYPE__ size_t;
+extern "C" void *memset (void *, int, size_t);
+char buf[1024];
+
+template <int N>
+void
+foo ()
+{
+  memset (buf, sizeof buf, 0);	// { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" }
+  memset (buf, sizeof buf, '\0'); // { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" }
+  memset (buf, sizeof buf, N);	// { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" }
+  memset (buf, 1, 1 - 1);	// { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" }
+  memset (buf, 1, N - N);	// { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" }
+  memset (buf, 0, 0);
+  memset (buf, 1 - 1, 0);
+}
+
+void
+bar ()
+{
+  foo<0> ();
+}

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]