public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Re: [RFC PATCH] -Wmemset-transposed-args (PR middle-end/61294)
@ 2014-07-09 14:40 Manuel López-Ibáñez
  2014-07-09 21:28 ` Jason Merrill
  0 siblings, 1 reply; 17+ messages in thread
From: Manuel López-Ibáñez @ 2014-07-09 14:40 UTC (permalink / raw)
  To: Gcc Patch List
  Cc: Richard Biener, Jason Merrill, Jakub Jelinek, Joseph S. Myers,
	Siddhesh Poyarekar, Carlos O'Donell, GNU C Library

> All of these warnings (-Wsizeof-pointer-memaccess, -Wsizeof-array-argument
> and -Wmemset-transposed-args) are implemented in a hackish way, because we
> fold everything too early.  Perhaps for such analysis we want a FOLDED_EXPR
> which would have arguments what it has been folded to and the original tree,
> for the purposes of code generation the first argument would be used and
> the second one only for the analysis.  We don't have that many spots where
> we need the original trees to be analyzed yet for it to be worth it though
> IMHO.

But if we keep adding hacks around it, there will never be progress
and the person(s) who take the challenge of properly fixing this will
not only have to deal with the task itself but also with all the ugly
and obscure hacks added year after year.

Is it worth the trouble?

Well, apart from those warnings that you mention, we have several bugs
about it: See PR32643, PR60090.

It prevents other desirable improvements:
https://gcc.gnu.org/ml/gcc-patches/2012-09/msg01222.html

It is mentioned as one of the "pros" of Clang vs. GCC:
http://clang.llvm.org/comparison.html#gcc

One of the main reasons why Clang was developed:
https://www.youtube.com/watch?v=029YXzHtRy0#t=20m52s

One of the reasons why Google switched to Clang:
https://www.youtube.com/watch?v=NURiiQatBXA#t=4m09s

And there has been a lot of work in this direction in the C FE that
was never translated to the C++ FE:
https://gcc.gnu.org/ml/gcc-patches/2008-10/msg01061.html

Unfortunately, this is not the kind of work that a GSOC student or a
volunteer can do on its free time. It requires a lot of experience and
a continuous focused effort.

Cheers,

Manuel.

^ permalink raw reply	[flat|nested] 17+ messages in thread
* [RFC PATCH] -Wmemset-transposed-args (PR middle-end/61294)
@ 2014-07-08 12:50 Jakub Jelinek
  2014-07-08 19:25 ` Jason Merrill
  2014-07-08 19:34 ` Carlos O'Donell
  0 siblings, 2 replies; 17+ messages in thread
From: Jakub Jelinek @ 2014-07-08 12:50 UTC (permalink / raw)
  To: Joseph S. Myers, Jason Merrill, Carlos O'Donell, Siddhesh Poyarekar
  Cc: gcc-patches, libc-alpha

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

^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-04-12 19:30 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-09 14:40 [RFC PATCH] -Wmemset-transposed-args (PR middle-end/61294) Manuel López-Ibáñez
2014-07-09 21:28 ` Jason Merrill
2014-07-10 12:52   ` [PATCH] -Wmemset-transposed-args (PR middle-end/61294, take 2) Jakub Jelinek
2014-07-10 17:57     ` Jason Merrill
2014-07-10 23:29     ` Gerald Pfeifer
2014-07-11 20:19       ` Jakub Jelinek
2015-04-12 19:30         ` Gerald Pfeifer
  -- strict thread matches above, loose matches on Subject: below --
2014-07-08 12:50 [RFC PATCH] -Wmemset-transposed-args (PR middle-end/61294) Jakub Jelinek
2014-07-08 19:25 ` Jason Merrill
2014-07-08 19:38   ` Carlos O'Donell
2014-07-08 20:32   ` Jakub Jelinek
2014-07-08 22:33     ` Jason Merrill
2014-07-09 10:26       ` Richard Biener
2014-07-09 10:33         ` Jakub Jelinek
2014-07-09 10:51           ` Richard Biener
2014-07-09 11:09             ` Jakub Jelinek
2014-07-08 19:34 ` Carlos O'Donell

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).