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

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

[Manuel López-Ibáñez]

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

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

[Jason Merrill]

On 07/09/2014 10:40 AM, Manuel López-Ibáñez wrote:
>> 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.

I'm planning to address this soon, hopefully this stage 1.

Jason

[PATCH] -Wmemset-transposed-args (PR middle-end/61294, take 2)

[Jakub Jelinek]

On Wed, Jul 09, 2014 at 05:28:34PM -0400, Jason Merrill wrote:
> On 07/09/2014 10:40 AM, Manuel López-Ibáñez wrote:
> >>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.
> 
> I'm planning to address this soon, hopefully this stage 1.

So like this then?  As the testcases show, only literal zero (0, 0L, '\0',
u'\0', U'\0' etc.) alone is now recognized, 0 - 0 or (int) 0 don't count.

On Richard's request I'm stripping the special INTEGER_CSTs early again.
The C FE uses a bitmask on the side (similarly how it already handles
sizeof).

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

	PR middle-end/61294
gcc/c-family/
	* c.opt (Wmemset-transposed-args): New warning.
gcc/c/
	* c-parser.c (c_parser_expr_list): Add new argument literal_zero_mask.
	If non-NULL, call c_parser_check_literal_zero.
	(c_parser_check_literal_zero): New function.
	(c_parser_postfix_expression_after_primary): Adjust
	c_parser_expr_list caller, handle -Wmemset-transposed-args.
gcc/cp/
	* cp-tree.h (LITERAL_ZERO_P): Define.
	* parser.c (cp_parser_parenthesized_expression_list): Add
	want_literal_zero_p argument, if true, for literal zeros
	insert INTEGER_CSTs with LITERAL_ZERO_P flag set.
	(cp_parser_postfix_expression): Adjust
	cp_parser_parenthesized_expression_list caller, handle
	-Wmemset-transposed-args.
	(literal_zeros): New variable.
gcc/
	* doc/invoke.texi (-Wmemset-transposed-args): Document.
gcc/testsuite/
	* c-c++-common/Wmemset-transposed-args1.c: New test.
	* c-c++-common/Wmemset-transposed-args2.c: New test.
	* g++.dg/warn/Wmemset-transposed-args-1.C: New test.

--- gcc/c/c-parser.c.jj	2014-07-09 13:20:33.000000000 +0200
+++ gcc/c/c-parser.c	2014-07-10 14:15:27.221202422 +0200
@@ -1204,7 +1204,8 @@ static struct c_expr c_parser_expression
 static struct c_expr c_parser_expression_conv (c_parser *);
 static vec<tree, va_gc> *c_parser_expr_list (c_parser *, bool, bool,
 					     vec<tree, va_gc> **, location_t *,
-					     tree *, vec<location_t> *);
+					     tree *, vec<location_t> *,
+					     unsigned int * = NULL);
 static void c_parser_omp_construct (c_parser *);
 static void c_parser_omp_threadprivate (c_parser *);
 static void c_parser_omp_barrier (c_parser *);
@@ -7655,6 +7656,7 @@ c_parser_postfix_expression_after_primar
   tree ident, idx;
   location_t sizeof_arg_loc[3];
   tree sizeof_arg[3];
+  unsigned int literal_zero_mask;
   unsigned int i;
   vec<tree, va_gc> *exprlist;
   vec<tree, va_gc> *origtypes = NULL;
@@ -7709,12 +7711,13 @@ c_parser_postfix_expression_after_primar
 	      sizeof_arg[i] = NULL_TREE;
 	      sizeof_arg_loc[i] = UNKNOWN_LOCATION;
 	    }
+	  literal_zero_mask = 0;
 	  if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN))
 	    exprlist = NULL;
 	  else
 	    exprlist = c_parser_expr_list (parser, true, false, &origtypes,
 					   sizeof_arg_loc, sizeof_arg,
-					   &arg_loc);
+					   &arg_loc, &literal_zero_mask);
 	  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN,
 				     "expected %<)%>");
 	  orig_expr = expr;
@@ -7724,6 +7727,18 @@ c_parser_postfix_expression_after_primar
 					      expr.value, exprlist,
 					      sizeof_arg,
 					      sizeof_ptr_memacc_comptypes);
+	  if (warn_memset_transposed_args
+	      && 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");
+
 	  expr.value
 	    = c_build_function_call_vec (expr_loc, arg_loc, expr.value,
 					 exprlist, origtypes);
@@ -7891,6 +7906,36 @@ c_parser_expression_conv (c_parser *pars
   return expr;
 }
 
+/* Helper function of c_parser_expr_list.  Check if IDXth (0 based)
+   argument is a literal zero alone and if so, set it in literal_zero_mask.  */
+
+static inline void
+c_parser_check_literal_zero (c_parser *parser, unsigned *literal_zero_mask,
+			     unsigned int idx)
+{
+  if (idx >= HOST_BITS_PER_INT)
+    return;
+
+  c_token *tok = c_parser_peek_token (parser);
+  switch (tok->type)
+    {
+    case CPP_NUMBER:
+    case CPP_CHAR:
+    case CPP_WCHAR:
+    case CPP_CHAR16:
+    case CPP_CHAR32:
+      /* If a parameter is literal zero alone, remember it
+	 for -Wmemset-transposed-args warning.  */
+      if (integer_zerop (tok->value)
+	  && !TREE_OVERFLOW (tok->value)
+	  && (c_parser_peek_2nd_token (parser)->type == CPP_COMMA
+	      || c_parser_peek_2nd_token (parser)->type == CPP_CLOSE_PAREN))
+	*literal_zero_mask |= 1U << idx;
+    default:
+      break;
+    }
+}
+
 /* Parse a non-empty list of expressions.  If CONVERT_P, convert
    functions and arrays to pointers and lvalues to rvalues.  If
    FOLD_P, fold the expressions.  If LOCATIONS is non-NULL, save the
@@ -7905,7 +7950,8 @@ static vec<tree, va_gc> *
 c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p,
 		    vec<tree, va_gc> **p_orig_types,
 		    location_t *sizeof_arg_loc, tree *sizeof_arg,
-		    vec<location_t> *locations)
+		    vec<location_t> *locations,
+		    unsigned int *literal_zero_mask)
 {
   vec<tree, va_gc> *ret;
   vec<tree, va_gc> *orig_types;
@@ -7923,6 +7969,8 @@ c_parser_expr_list (c_parser *parser, bo
   if (sizeof_arg != NULL
       && c_parser_next_token_is_keyword (parser, RID_SIZEOF))
     cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location;
+  if (literal_zero_mask)
+    c_parser_check_literal_zero (parser, literal_zero_mask, 0);
   expr = c_parser_expr_no_commas (parser, NULL);
   if (convert_p)
     expr = convert_lvalue_to_rvalue (loc, expr, true, true);
@@ -7949,6 +7997,8 @@ c_parser_expr_list (c_parser *parser, bo
 	cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location;
       else
 	cur_sizeof_arg_loc = UNKNOWN_LOCATION;
+      if (literal_zero_mask)
+	c_parser_check_literal_zero (parser, literal_zero_mask, idx + 1);
       expr = c_parser_expr_no_commas (parser, NULL);
       if (convert_p)
 	expr = convert_lvalue_to_rvalue (loc, expr, true, true);
--- gcc/cp/cp-tree.h.jj	2014-07-09 13:20:33.922835883 +0200
+++ gcc/cp/cp-tree.h	2014-07-10 13:16:01.066372163 +0200
@@ -4187,6 +4187,10 @@ more_aggr_init_expr_args_p (const aggr_i
 #define SIZEOF_EXPR_TYPE_P(NODE) \
   TREE_LANG_FLAG_0 (SIZEOF_EXPR_CHECK (NODE))
 
+/* True if INTEGER_CST is a zero literal seen in function argument list.  */
+#define LITERAL_ZERO_P(NODE) \
+  (INTEGER_CST_CHECK (NODE)->base.nothrow_flag)
+
 /* An enumeration of the kind of tags that C++ accepts.  */
 enum tag_types {
   none_type = 0, /* Not a tag type.  */
--- gcc/cp/parser.c.jj	2014-07-09 13:20:33.844836255 +0200
+++ gcc/cp/parser.c	2014-07-10 14:00:37.915749580 +0200
@@ -1929,7 +1929,7 @@ static tree cp_parser_postfix_open_squar
 static tree cp_parser_postfix_dot_deref_expression
   (cp_parser *, enum cpp_ttype, tree, bool, cp_id_kind *, location_t);
 static vec<tree, va_gc> *cp_parser_parenthesized_expression_list
-  (cp_parser *, int, bool, bool, bool *);
+  (cp_parser *, int, bool, bool, bool *, bool = false);
 /* Values for the second parameter of cp_parser_parenthesized_expression_list.  */
 enum { non_attr = 0, normal_attr = 1, id_attr = 2 };
 static void cp_parser_pseudo_destructor_name
@@ -6074,7 +6074,8 @@ cp_parser_postfix_expression (cp_parser
 	    args = (cp_parser_parenthesized_expression_list
 		    (parser, non_attr,
 		     /*cast_p=*/false, /*allow_expansion_p=*/true,
-		     /*non_constant_p=*/NULL));
+		     /*non_constant_p=*/NULL,
+		     /*want_literal_zero_p=*/warn_memset_transposed_args));
 	    if (is_builtin_constant_p)
 	      {
 		parser->integral_constant_expression_p
@@ -6142,6 +6143,30 @@ 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
+		    && integer_zerop ((*args)[2])
+		    && LITERAL_ZERO_P ((*args)[2])
+		    && !(integer_zerop ((*args)[1])
+			 && LITERAL_ZERO_P ((*args)[1])))
+		  warning (OPT_Wmemset_transposed_args,
+			   "%<memset%> used with constant zero length "
+			   "parameter; this could be due to transposed "
+			   "parameters");
+
+		/* Replace LITERAL_ZERO_P INTEGER_CSTs with normal ones
+		   to avoid leaking those into folder and middle-end.  */
+		unsigned int i;
+		tree arg;
+		FOR_EACH_VEC_SAFE_ELT (args, i, arg)
+		  if (TREE_CODE (arg) == INTEGER_CST && LITERAL_ZERO_P (arg))
+		    (*args)[i] = build_int_cst (TREE_TYPE (arg), 0);
+	      }
+
 	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
 	      {
 		tree instance = TREE_OPERAND (postfix_expression, 0);
@@ -6630,6 +6655,10 @@ cp_parser_postfix_dot_deref_expression (
   return postfix_expression;
 }
 
+/* Cache of LITERAL_ZERO_P constants.  */
+
+static GTY(()) tree literal_zeros[itk_none];
+
 /* Parse a parenthesized expression-list.
 
    expression-list:
@@ -6654,14 +6683,18 @@ cp_parser_postfix_dot_deref_expression (
    plain identifier argument, normal_attr for an attribute that wants
    an expression, or non_attr if we aren't parsing an attribute list.  If
    NON_CONSTANT_P is non-NULL, *NON_CONSTANT_P indicates whether or
-   not all of the expressions in the list were constant.  */
+   not all of the expressions in the list were constant.
+   WANT_LITERAL_ZERO_P is true if the caller is interested in
+   LITERAL_ZERO_P INTEGER_CSTs.  FIXME: once we don't fold everything
+   immediately, this can be removed.  */
 
 static vec<tree, va_gc> *
 cp_parser_parenthesized_expression_list (cp_parser* parser,
 					 int is_attribute_list,
 					 bool cast_p,
                                          bool allow_expansion_p,
-					 bool *non_constant_p)
+					 bool *non_constant_p,
+					 bool want_literal_zero_p)
 {
   vec<tree, va_gc> *expression_list;
   bool fold_expr_p = is_attribute_list != non_attr;
@@ -6724,7 +6757,50 @@ cp_parser_parenthesized_expression_list
 		  *non_constant_p = true;
 	      }
 	    else
-	      expr = cp_parser_assignment_expression (parser, cast_p, NULL);
+	      {
+		expr = NULL_TREE;
+		cp_token *tok = cp_lexer_peek_token (parser->lexer);
+		switch (tok->type)
+		  {
+		  case CPP_NUMBER:
+		  case CPP_CHAR:
+		  case CPP_WCHAR:
+		  case CPP_CHAR16:
+		  case CPP_CHAR32:
+		    /* If a parameter is literal zero alone, remember it
+		       for -Wmemset-transposed-args warning.  */
+		    if (integer_zerop (tok->u.value)
+			&& !TREE_OVERFLOW (tok->u.value)
+			&& want_literal_zero_p
+			&& (cp_lexer_peek_nth_token (parser->lexer, 2)->type
+			    == CPP_COMMA
+			    || cp_lexer_peek_nth_token (parser->lexer, 2)->type
+			       == CPP_CLOSE_PAREN))
+		      {
+			unsigned int i;
+			for (i = 0; i < itk_none; ++i)
+			  if (TREE_TYPE (tok->u.value) == integer_types[i])
+			    break;
+			if (i < itk_none && literal_zeros[i])
+			  expr = literal_zeros[i];
+			else
+			  {
+			    expr = copy_node (tok->u.value);
+			    LITERAL_ZERO_P (expr) = 1;
+			    if (i < itk_none)
+			      literal_zeros[i] = expr;
+			  }
+			/* Consume the 0 token (or '\0', 0LL etc.).  */
+			cp_lexer_consume_token (parser->lexer);
+		      }
+		    break;
+		  default:
+		    break;
+		  }
+		if (expr == NULL_TREE)
+		  expr = cp_parser_assignment_expression (parser, cast_p,
+							  NULL);
+	      }
 
 	    if (fold_expr_p)
 	      expr = fold_non_dependent_expr (expr);
--- gcc/c-family/c.opt.jj	2014-07-09 13:20:33.841836272 +0200
+++ gcc/c-family/c.opt	2014-07-10 13:16:01.162371729 +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/doc/invoke.texi.jj	2014-07-09 13:20:33.567837626 +0200
+++ gcc/doc/invoke.texi	2014-07-10 14:24:23.419458164 +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,18 @@ 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.  The diagnostics
+is only emitted if the third argument is literal zero, if it is some expression
+that is folded to zero, or e.g. a cast of zero to some type etc., it
+is far less likely that user has mistakenly exchanged the arguments and
+no warning is emitted.  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-10 13:16:01.145371805 +0200
+++ gcc/testsuite/c-c++-common/Wmemset-transposed-args1.c	2014-07-10 13:56:16.251092460 +0200
@@ -0,0 +1,31 @@
+/* { 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, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 1, 1 - 1);
+  memset (buf, 1, 0 - 0);
+  memset (buf, 0, 0);
+  memset (buf, '\0', 0);
+  memset (buf, L'\0', 0);
+  memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, (int) 0);
+  memset (buf, sizeof buf, -0);
+}
--- gcc/testsuite/c-c++-common/Wmemset-transposed-args2.c.jj	2014-07-10 13:52:47.425157923 +0200
+++ gcc/testsuite/c-c++-common/Wmemset-transposed-args2.c	2014-07-10 13:54:47.954539662 +0200
@@ -0,0 +1,20 @@
+/* { dg-do compile { target { c || c++11 } } } */
+/* { dg-options "-Wall" } */
+/* { dg-additional-options "-std=gnu99" { target c } } */
+
+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, u'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, U'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, u'\0', 0);
+  memset (buf, U'\0', 0);
+}
--- gcc/testsuite/g++.dg/warn/Wmemset-transposed-args-1.C.jj	2014-07-10 13:16:01.144371809 +0200
+++ gcc/testsuite/g++.dg/warn/Wmemset-transposed-args-1.C	2014-07-10 13:16:01.144371809 +0200
@@ -0,0 +1,74 @@
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+typedef __SIZE_TYPE__ size_t;
+extern "C" void *memset (void *, int, size_t);
+char buf[1024];
+namespace std
+{
+  extern "C" void *memset (void *, int, size_t);
+}
+
+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, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, N);
+  memset (buf, 1, 1 - 1);
+  memset (buf, 1, 0 - 0);
+  memset (buf, 1, N - N);
+  memset (buf, 0, 0);
+  memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, (int) 0);
+  memset (buf, sizeof buf, -0);
+}
+
+template <int N>
+void
+baz ()
+{
+  std::memset (buf, sizeof buf, 0);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, N);
+  std::memset (buf, 1, 1 - 1);
+  std::memset (buf, 1, 0 - 0);
+  std::memset (buf, 1, N - N);
+  std::memset (buf, 0, 0);
+  std::memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, (int) 0);
+  std::memset (buf, sizeof buf, -0);
+}
+
+void
+bar ()
+{
+  foo<0> ();
+  std::memset (buf, sizeof buf, 0);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 1, 1 - 1);
+  std::memset (buf, 1, 0 - 0);
+  std::memset (buf, 0, 0);
+  std::memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, (int) 0);
+  std::memset (buf, sizeof buf, -0);
+}


	Jakub

Re: [PATCH] -Wmemset-transposed-args (PR middle-end/61294, take 2)

[Jason Merrill]

OK.

Jason

Re: [PATCH] -Wmemset-transposed-args (PR middle-end/61294, take 2)

[Gerald Pfeifer]

On Thu, 10 Jul 2014, Jakub Jelinek wrote:
> +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

"calls" (plural), like in the .texi documentation?

"and the second"

(If you want to keep it short, you could say "the third argument is 
constant zero and the second is not".)

> +Warn for suspicious calls to the memset built-in function, if the

Should this be @code{memset} as well?

> +second argument is not zero and third argument is zero.  This warns e.g.@

"the third argument"

> +about @code{memset (buf, sizeof buf, 0);} where most probably
> +@code{memset (buf, 0, sizeof buf);} was meant instead.

I believe this will look better without the semicolons.

Gerald

Re: [PATCH] -Wmemset-transposed-args (PR middle-end/61294, take 2)

[Jakub Jelinek]

On Fri, Jul 11, 2014 at 01:29:12AM +0200, Gerald Pfeifer wrote:
> On Thu, 10 Jul 2014, Jakub Jelinek wrote:
> > +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
> 
> "calls" (plural), like in the .texi documentation?
> 
> "and the second"
> 
> (If you want to keep it short, you could say "the third argument is 
> constant zero and the second is not".)
> 
> > +Warn for suspicious calls to the memset built-in function, if the
> 
> Should this be @code{memset} as well?
> 
> > +second argument is not zero and third argument is zero.  This warns e.g.@
> 
> "the third argument"
> 
> > +about @code{memset (buf, sizeof buf, 0);} where most probably
> > +@code{memset (buf, 0, sizeof buf);} was meant instead.
> 
> I believe this will look better without the semicolons.

So like this?  Also have fixed one omitted line in c-parser.c, this patch
bootstrapped/regtested fine:

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

	PR middle-end/61294
gcc/c-family/
	* c.opt (Wmemset-transposed-args): New warning.
gcc/c/
	* c-parser.c (c_parser_expr_list): Add new argument literal_zero_mask.
	If non-NULL, call c_parser_check_literal_zero.
	(c_parser_check_literal_zero): New function.
	(c_parser_postfix_expression_after_primary): Adjust
	c_parser_expr_list caller, handle -Wmemset-transposed-args.
gcc/cp/
	* cp-tree.h (LITERAL_ZERO_P): Define.
	* parser.c (cp_parser_parenthesized_expression_list): Add
	want_literal_zero_p argument, if true, for literal zeros
	insert INTEGER_CSTs with LITERAL_ZERO_P flag set.
	(cp_parser_postfix_expression): Adjust
	cp_parser_parenthesized_expression_list caller, handle
	-Wmemset-transposed-args.
	(literal_zeros): New variable.
gcc/
	* doc/invoke.texi (-Wmemset-transposed-args): Document.
gcc/testsuite/
	* c-c++-common/Wmemset-transposed-args1.c: New test.
	* c-c++-common/Wmemset-transposed-args2.c: New test.
	* g++.dg/warn/Wmemset-transposed-args-1.C: New test.

--- gcc/c/c-parser.c.jj	2014-07-09 13:20:33.000000000 +0200
+++ gcc/c/c-parser.c	2014-07-11 12:40:05.493107605 +0200
@@ -1204,7 +1204,8 @@ static struct c_expr c_parser_expression
 static struct c_expr c_parser_expression_conv (c_parser *);
 static vec<tree, va_gc> *c_parser_expr_list (c_parser *, bool, bool,
 					     vec<tree, va_gc> **, location_t *,
-					     tree *, vec<location_t> *);
+					     tree *, vec<location_t> *,
+					     unsigned int * = NULL);
 static void c_parser_omp_construct (c_parser *);
 static void c_parser_omp_threadprivate (c_parser *);
 static void c_parser_omp_barrier (c_parser *);
@@ -7655,6 +7656,7 @@ c_parser_postfix_expression_after_primar
   tree ident, idx;
   location_t sizeof_arg_loc[3];
   tree sizeof_arg[3];
+  unsigned int literal_zero_mask;
   unsigned int i;
   vec<tree, va_gc> *exprlist;
   vec<tree, va_gc> *origtypes = NULL;
@@ -7709,12 +7711,13 @@ c_parser_postfix_expression_after_primar
 	      sizeof_arg[i] = NULL_TREE;
 	      sizeof_arg_loc[i] = UNKNOWN_LOCATION;
 	    }
+	  literal_zero_mask = 0;
 	  if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN))
 	    exprlist = NULL;
 	  else
 	    exprlist = c_parser_expr_list (parser, true, false, &origtypes,
 					   sizeof_arg_loc, sizeof_arg,
-					   &arg_loc);
+					   &arg_loc, &literal_zero_mask);
 	  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN,
 				     "expected %<)%>");
 	  orig_expr = expr;
@@ -7724,6 +7727,19 @@ 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
+	      && 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");
+
 	  expr.value
 	    = c_build_function_call_vec (expr_loc, arg_loc, expr.value,
 					 exprlist, origtypes);
@@ -7891,6 +7907,36 @@ c_parser_expression_conv (c_parser *pars
   return expr;
 }
 
+/* Helper function of c_parser_expr_list.  Check if IDXth (0 based)
+   argument is a literal zero alone and if so, set it in literal_zero_mask.  */
+
+static inline void
+c_parser_check_literal_zero (c_parser *parser, unsigned *literal_zero_mask,
+			     unsigned int idx)
+{
+  if (idx >= HOST_BITS_PER_INT)
+    return;
+
+  c_token *tok = c_parser_peek_token (parser);
+  switch (tok->type)
+    {
+    case CPP_NUMBER:
+    case CPP_CHAR:
+    case CPP_WCHAR:
+    case CPP_CHAR16:
+    case CPP_CHAR32:
+      /* If a parameter is literal zero alone, remember it
+	 for -Wmemset-transposed-args warning.  */
+      if (integer_zerop (tok->value)
+	  && !TREE_OVERFLOW (tok->value)
+	  && (c_parser_peek_2nd_token (parser)->type == CPP_COMMA
+	      || c_parser_peek_2nd_token (parser)->type == CPP_CLOSE_PAREN))
+	*literal_zero_mask |= 1U << idx;
+    default:
+      break;
+    }
+}
+
 /* Parse a non-empty list of expressions.  If CONVERT_P, convert
    functions and arrays to pointers and lvalues to rvalues.  If
    FOLD_P, fold the expressions.  If LOCATIONS is non-NULL, save the
@@ -7905,7 +7951,8 @@ static vec<tree, va_gc> *
 c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p,
 		    vec<tree, va_gc> **p_orig_types,
 		    location_t *sizeof_arg_loc, tree *sizeof_arg,
-		    vec<location_t> *locations)
+		    vec<location_t> *locations,
+		    unsigned int *literal_zero_mask)
 {
   vec<tree, va_gc> *ret;
   vec<tree, va_gc> *orig_types;
@@ -7923,6 +7970,8 @@ c_parser_expr_list (c_parser *parser, bo
   if (sizeof_arg != NULL
       && c_parser_next_token_is_keyword (parser, RID_SIZEOF))
     cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location;
+  if (literal_zero_mask)
+    c_parser_check_literal_zero (parser, literal_zero_mask, 0);
   expr = c_parser_expr_no_commas (parser, NULL);
   if (convert_p)
     expr = convert_lvalue_to_rvalue (loc, expr, true, true);
@@ -7949,6 +7998,8 @@ c_parser_expr_list (c_parser *parser, bo
 	cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location;
       else
 	cur_sizeof_arg_loc = UNKNOWN_LOCATION;
+      if (literal_zero_mask)
+	c_parser_check_literal_zero (parser, literal_zero_mask, idx + 1);
       expr = c_parser_expr_no_commas (parser, NULL);
       if (convert_p)
 	expr = convert_lvalue_to_rvalue (loc, expr, true, true);
--- gcc/cp/cp-tree.h.jj	2014-07-09 13:20:33.922835883 +0200
+++ gcc/cp/cp-tree.h	2014-07-10 13:16:01.066372163 +0200
@@ -4187,6 +4187,10 @@ more_aggr_init_expr_args_p (const aggr_i
 #define SIZEOF_EXPR_TYPE_P(NODE) \
   TREE_LANG_FLAG_0 (SIZEOF_EXPR_CHECK (NODE))
 
+/* True if INTEGER_CST is a zero literal seen in function argument list.  */
+#define LITERAL_ZERO_P(NODE) \
+  (INTEGER_CST_CHECK (NODE)->base.nothrow_flag)
+
 /* An enumeration of the kind of tags that C++ accepts.  */
 enum tag_types {
   none_type = 0, /* Not a tag type.  */
--- gcc/cp/parser.c.jj	2014-07-09 13:20:33.844836255 +0200
+++ gcc/cp/parser.c	2014-07-10 14:00:37.915749580 +0200
@@ -1929,7 +1929,7 @@ static tree cp_parser_postfix_open_squar
 static tree cp_parser_postfix_dot_deref_expression
   (cp_parser *, enum cpp_ttype, tree, bool, cp_id_kind *, location_t);
 static vec<tree, va_gc> *cp_parser_parenthesized_expression_list
-  (cp_parser *, int, bool, bool, bool *);
+  (cp_parser *, int, bool, bool, bool *, bool = false);
 /* Values for the second parameter of cp_parser_parenthesized_expression_list.  */
 enum { non_attr = 0, normal_attr = 1, id_attr = 2 };
 static void cp_parser_pseudo_destructor_name
@@ -6074,7 +6074,8 @@ cp_parser_postfix_expression (cp_parser
 	    args = (cp_parser_parenthesized_expression_list
 		    (parser, non_attr,
 		     /*cast_p=*/false, /*allow_expansion_p=*/true,
-		     /*non_constant_p=*/NULL));
+		     /*non_constant_p=*/NULL,
+		     /*want_literal_zero_p=*/warn_memset_transposed_args));
 	    if (is_builtin_constant_p)
 	      {
 		parser->integral_constant_expression_p
@@ -6142,6 +6143,30 @@ 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
+		    && integer_zerop ((*args)[2])
+		    && LITERAL_ZERO_P ((*args)[2])
+		    && !(integer_zerop ((*args)[1])
+			 && LITERAL_ZERO_P ((*args)[1])))
+		  warning (OPT_Wmemset_transposed_args,
+			   "%<memset%> used with constant zero length "
+			   "parameter; this could be due to transposed "
+			   "parameters");
+
+		/* Replace LITERAL_ZERO_P INTEGER_CSTs with normal ones
+		   to avoid leaking those into folder and middle-end.  */
+		unsigned int i;
+		tree arg;
+		FOR_EACH_VEC_SAFE_ELT (args, i, arg)
+		  if (TREE_CODE (arg) == INTEGER_CST && LITERAL_ZERO_P (arg))
+		    (*args)[i] = build_int_cst (TREE_TYPE (arg), 0);
+	      }
+
 	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
 	      {
 		tree instance = TREE_OPERAND (postfix_expression, 0);
@@ -6630,6 +6655,10 @@ cp_parser_postfix_dot_deref_expression (
   return postfix_expression;
 }
 
+/* Cache of LITERAL_ZERO_P constants.  */
+
+static GTY(()) tree literal_zeros[itk_none];
+
 /* Parse a parenthesized expression-list.
 
    expression-list:
@@ -6654,14 +6683,18 @@ cp_parser_postfix_dot_deref_expression (
    plain identifier argument, normal_attr for an attribute that wants
    an expression, or non_attr if we aren't parsing an attribute list.  If
    NON_CONSTANT_P is non-NULL, *NON_CONSTANT_P indicates whether or
-   not all of the expressions in the list were constant.  */
+   not all of the expressions in the list were constant.
+   WANT_LITERAL_ZERO_P is true if the caller is interested in
+   LITERAL_ZERO_P INTEGER_CSTs.  FIXME: once we don't fold everything
+   immediately, this can be removed.  */
 
 static vec<tree, va_gc> *
 cp_parser_parenthesized_expression_list (cp_parser* parser,
 					 int is_attribute_list,
 					 bool cast_p,
                                          bool allow_expansion_p,
-					 bool *non_constant_p)
+					 bool *non_constant_p,
+					 bool want_literal_zero_p)
 {
   vec<tree, va_gc> *expression_list;
   bool fold_expr_p = is_attribute_list != non_attr;
@@ -6724,7 +6757,50 @@ cp_parser_parenthesized_expression_list
 		  *non_constant_p = true;
 	      }
 	    else
-	      expr = cp_parser_assignment_expression (parser, cast_p, NULL);
+	      {
+		expr = NULL_TREE;
+		cp_token *tok = cp_lexer_peek_token (parser->lexer);
+		switch (tok->type)
+		  {
+		  case CPP_NUMBER:
+		  case CPP_CHAR:
+		  case CPP_WCHAR:
+		  case CPP_CHAR16:
+		  case CPP_CHAR32:
+		    /* If a parameter is literal zero alone, remember it
+		       for -Wmemset-transposed-args warning.  */
+		    if (integer_zerop (tok->u.value)
+			&& !TREE_OVERFLOW (tok->u.value)
+			&& want_literal_zero_p
+			&& (cp_lexer_peek_nth_token (parser->lexer, 2)->type
+			    == CPP_COMMA
+			    || cp_lexer_peek_nth_token (parser->lexer, 2)->type
+			       == CPP_CLOSE_PAREN))
+		      {
+			unsigned int i;
+			for (i = 0; i < itk_none; ++i)
+			  if (TREE_TYPE (tok->u.value) == integer_types[i])
+			    break;
+			if (i < itk_none && literal_zeros[i])
+			  expr = literal_zeros[i];
+			else
+			  {
+			    expr = copy_node (tok->u.value);
+			    LITERAL_ZERO_P (expr) = 1;
+			    if (i < itk_none)
+			      literal_zeros[i] = expr;
+			  }
+			/* Consume the 0 token (or '\0', 0LL etc.).  */
+			cp_lexer_consume_token (parser->lexer);
+		      }
+		    break;
+		  default:
+		    break;
+		  }
+		if (expr == NULL_TREE)
+		  expr = cp_parser_assignment_expression (parser, cast_p,
+							  NULL);
+	      }
 
 	    if (fold_expr_p)
 	      expr = fold_non_dependent_expr (expr);
--- gcc/c-family/c.opt.jj	2014-07-09 13:20:33.841836272 +0200
+++ gcc/c-family/c.opt	2014-07-10 13:16:01.162371729 +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 calls to memset where the third argument is constant literal zero and the second is not
+
 Wmissing-braces
 C ObjC C++ ObjC++ Var(warn_missing_braces) Warning LangEnabledBy(C ObjC,Wall)
 Warn about possibly missing braces around initializers
--- gcc/doc/invoke.texi.jj	2014-07-09 13:20:33.567837626 +0200
+++ gcc/doc/invoke.texi	2014-07-10 14:24:23.419458164 +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,18 @@ 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 @code{memset} built-in function, if the
+second argument is not zero and the 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.  The diagnostics
+is only emitted if the third argument is literal zero, if it is some expression
+that is folded to zero, or e.g. a cast of zero to some type etc., it
+is far less likely that user has mistakenly exchanged the arguments and
+no warning is emitted.  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-10 13:16:01.145371805 +0200
+++ gcc/testsuite/c-c++-common/Wmemset-transposed-args1.c	2014-07-10 13:56:16.251092460 +0200
@@ -0,0 +1,31 @@
+/* { 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, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 1, 1 - 1);
+  memset (buf, 1, 0 - 0);
+  memset (buf, 0, 0);
+  memset (buf, '\0', 0);
+  memset (buf, L'\0', 0);
+  memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, (int) 0);
+  memset (buf, sizeof buf, -0);
+}
--- gcc/testsuite/c-c++-common/Wmemset-transposed-args2.c.jj	2014-07-10 13:52:47.425157923 +0200
+++ gcc/testsuite/c-c++-common/Wmemset-transposed-args2.c	2014-07-10 13:54:47.954539662 +0200
@@ -0,0 +1,20 @@
+/* { dg-do compile { target { c || c++11 } } } */
+/* { dg-options "-Wall" } */
+/* { dg-additional-options "-std=gnu99" { target c } } */
+
+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, u'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, U'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, u'\0', 0);
+  memset (buf, U'\0', 0);
+}
--- gcc/testsuite/g++.dg/warn/Wmemset-transposed-args-1.C.jj	2014-07-10 13:16:01.144371809 +0200
+++ gcc/testsuite/g++.dg/warn/Wmemset-transposed-args-1.C	2014-07-10 13:16:01.144371809 +0200
@@ -0,0 +1,74 @@
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+typedef __SIZE_TYPE__ size_t;
+extern "C" void *memset (void *, int, size_t);
+char buf[1024];
+namespace std
+{
+  extern "C" void *memset (void *, int, size_t);
+}
+
+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, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, N);
+  memset (buf, 1, 1 - 1);
+  memset (buf, 1, 0 - 0);
+  memset (buf, 1, N - N);
+  memset (buf, 0, 0);
+  memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, (int) 0);
+  memset (buf, sizeof buf, -0);
+}
+
+template <int N>
+void
+baz ()
+{
+  std::memset (buf, sizeof buf, 0);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, N);
+  std::memset (buf, 1, 1 - 1);
+  std::memset (buf, 1, 0 - 0);
+  std::memset (buf, 1, N - N);
+  std::memset (buf, 0, 0);
+  std::memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, (int) 0);
+  std::memset (buf, sizeof buf, -0);
+}
+
+void
+bar ()
+{
+  foo<0> ();
+  std::memset (buf, sizeof buf, 0);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 1, 1 - 1);
+  std::memset (buf, 1, 0 - 0);
+  std::memset (buf, 0, 0);
+  std::memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, (int) 0);
+  std::memset (buf, sizeof buf, -0);
+}


	Jakub

Re: [PATCH] -Wmemset-transposed-args (PR middle-end/61294, take 2)

[Gerald Pfeifer]

Hi Jakub,

On Fri, 11 Jul 2014, Jakub Jelinek wrote:
> So like this?  Also have fixed one omitted line in c-parser.c, 
> this patch bootstrapped/regtested fine:

just in time for GCC 5.1 RC1 :-) a small update on top of yours.

Committed.

Gerald

2015-04-12  Gerald Pfeifer  <gerald@pfeifer.com>
 
	* doc/invoke.texi (-Wmemset-transposed-args): Break a long
	sentence.  Improve grammar.

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 222021)
+++ doc/invoke.texi	(working copy)
@@ -4909,10 +4909,10 @@
 second argument is not zero and the 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.  The diagnostics
-is only emitted if the third argument is literal zero, if it is some expression
-that is folded to zero, or e.g. a cast of zero to some type etc., it
-is far less likely that user has mistakenly exchanged the arguments and
-no warning is emitted.  This warning is enabled by @option{-Wall}.
+is only emitted if the third argument is literal zero.  If it is some
+expression that is folded to zero, a cast of zero to some type, etc., 
+it is far less likely that the user has mistakenly exchanged the arguments 
+and no warning is emitted.  This warning is enabled by @option{-Wall}.
 
 @item -Waddress
 @opindex Waddress

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

[Jakub Jelinek]

On Wed, Jul 09, 2014 at 12:51:32PM +0200, Richard Biener wrote:
> At least it shouldn't (they are not required to be shared and usually are not
> if they've gone a transition from TREE_OVERFLOW to !TREE_OVERFLOW).
> 
> Well, still feels ugly to me - but it's Jasons call in the end.

Another possibility is to keep that info on the side, something e.g.
the C FE already does.  There, c_parser_expr_list fills for the first 3
arguments (if requested by the caller) info about whether the argument used
to be originally a sizeof, and then the caller can look at this info.
For the literal zeros, it can be stored as a bitmask in a single char.

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.

	Jakub

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

[Richard Biener]

On Wed, Jul 9, 2014 at 12:32 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jul 09, 2014 at 12:26:09PM +0200, Richard Biener wrote:
>> > I suppose we could use an INTEGER_CST distinct from the one in
>> > TYPE_CACHED_VALUES for raw 0, with a TREE_LANG_FLAG set.
>>
>> Ick.  (please no - at least make sure it doesn't survive anywhere to the
>> middle-end, like fold or gimple).
>
> Why?  I don't think the middle-end ever compares INTEGER_CSTs for equality.

At least it shouldn't (they are not required to be shared and usually are not
if they've gone a transition from TREE_OVERFLOW to !TREE_OVERFLOW).

Well, still feels ugly to me - but it's Jasons call in the end.

Richard.

> Here is my WIP on Jason's suggestion (untested of course other than the
> testcase), the C++ FE is changed to do that, the C FE is not.
>
> Sure, I could add another bool argument to
> cp_parser_parenthesized_expression_list whether the caller expects to see
> the literal zeros, and replace them with normal INTEGER_CSTs after the test.
>
> --- gcc/c/c-typeck.c.jj 2014-07-08 17:38:09.612266656 +0200
> +++ gcc/c/c-typeck.c    2014-07-09 10:41:38.931018088 +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/cp-tree.h.jj 2014-07-07 10:39:44.000000000 +0200
> +++ gcc/cp/cp-tree.h    2014-07-09 11:48:12.534274257 +0200
> @@ -4187,6 +4187,10 @@ more_aggr_init_expr_args_p (const aggr_i
>  #define SIZEOF_EXPR_TYPE_P(NODE) \
>    TREE_LANG_FLAG_0 (SIZEOF_EXPR_CHECK (NODE))
>
> +/* True if INTEGER_CST is a zero literal seen in function argument list.  */
> +#define LITERAL_ZERO_P(NODE) \
> +  (INTEGER_CST_CHECK (NODE)->base.nothrow_flag)
> +
>  /* An enumeration of the kind of tags that C++ accepts.  */
>  enum tag_types {
>    none_type = 0, /* Not a tag type.  */
> --- gcc/cp/parser.c.jj  2014-07-09 10:41:12.000000000 +0200
> +++ gcc/cp/parser.c     2014-07-09 11:36:40.195693369 +0200
> @@ -6142,6 +6142,19 @@ cp_parser_postfix_expression (cp_parser
>                   }
>               }
>
> +         if (warn_memset_transposed_args
> +             && 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
> +             && integer_zerop ((*args)[2])
> +             && LITERAL_ZERO_P ((*args)[2])
> +             && !(integer_zerop ((*args)[1])
> +                  && LITERAL_ZERO_P ((*args)[1])))
> +           warning (OPT_Wmemset_transposed_args,
> +                    "%<memset%> used with constant zero length parameter; "
> +                    "this could be due to transposed parameters");
> +
>             if (TREE_CODE (postfix_expression) == COMPONENT_REF)
>               {
>                 tree instance = TREE_OPERAND (postfix_expression, 0);
> @@ -6630,6 +6643,10 @@ cp_parser_postfix_dot_deref_expression (
>    return postfix_expression;
>  }
>
> +/* Cache of LITERAL_ZERO_P constants.  */
> +
> +static GTY(()) tree literal_zeros[itk_none];
> +
>  /* Parse a parenthesized expression-list.
>
>     expression-list:
> @@ -6724,7 +6741,50 @@ cp_parser_parenthesized_expression_list
>                   *non_constant_p = true;
>               }
>             else
> -             expr = cp_parser_assignment_expression (parser, cast_p, NULL);
> +             {
> +               expr = NULL_TREE;
> +               cp_token *tok = cp_lexer_peek_token (parser->lexer);
> +               switch (tok->type)
> +                 {
> +                 case CPP_NUMBER:
> +                 case CPP_CHAR:
> +                 case CPP_WCHAR:
> +                 case CPP_CHAR16:
> +                 case CPP_CHAR32:
> +                   /* If a parameter is literal zero alone, remember it
> +                      for -Wmemset-transposed-args warning.  */
> +                   if (integer_zerop (tok->u.value)
> +                       && !TREE_OVERFLOW (tok->u.value)
> +                       && is_attribute_list == non_attr
> +                       && (cp_lexer_peek_nth_token (parser->lexer, 2)->type
> +                           == CPP_COMMA
> +                           || cp_lexer_peek_nth_token (parser->lexer, 2)->type
> +                              == CPP_CLOSE_PAREN))
> +                     {
> +                       unsigned int i;
> +                       for (i = 0; i < itk_none; ++i)
> +                         if (TREE_TYPE (tok->u.value) == integer_types[i])
> +                           break;
> +                       if (i < itk_none && literal_zeros[i])
> +                         expr = literal_zeros[i];
> +                       else
> +                         {
> +                           expr = copy_node (tok->u.value);
> +                           LITERAL_ZERO_P (expr) = 1;
> +                           if (i < itk_none)
> +                             literal_zeros[i] = expr;
> +                         }
> +                       /* Consume the 0 token (or '\0', 0LL etc.).  */
> +                       cp_lexer_consume_token (parser->lexer);
> +                     }
> +                   break;
> +                 default:
> +                   break;
> +                 }
> +               if (expr == NULL_TREE)
> +                 expr = cp_parser_assignment_expression (parser, cast_p,
> +                                                         NULL);
> +             }
>
>             if (fold_expr_p)
>               expr = fold_non_dependent_expr (expr);
> --- gcc/doc/invoke.texi.jj      2014-07-08 17:38:09.301268037 +0200
> +++ gcc/doc/invoke.texi 2014-07-09 10:41:38.950017994 +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/g++.dg/warn/Wmemset-transposed-args-1.C.jj    2014-07-09 10:41:38.969017900 +0200
> +++ gcc/testsuite/g++.dg/warn/Wmemset-transposed-args-1.C       2014-07-09 12:12:40.850993778 +0200
> @@ -0,0 +1,74 @@
> +// { dg-do compile }
> +// { dg-options "-Wall" }
> +
> +typedef __SIZE_TYPE__ size_t;
> +extern "C" void *memset (void *, int, size_t);
> +char buf[1024];
> +namespace std
> +{
> +  extern "C" void *memset (void *, int, size_t);
> +}
> +
> +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, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, N);
> +  memset (buf, 1, 1 - 1);
> +  memset (buf, 1, 0 - 0);
> +  memset (buf, 1, N - N);
> +  memset (buf, 0, 0);
> +  memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, 0L);        /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, (int) 0);
> +  memset (buf, sizeof buf, -0);
> +}
> +
> +template <int N>
> +void
> +baz ()
> +{
> +  std::memset (buf, sizeof buf, 0);    /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, N);
> +  std::memset (buf, 1, 1 - 1);
> +  std::memset (buf, 1, 0 - 0);
> +  std::memset (buf, 1, N - N);
> +  std::memset (buf, 0, 0);
> +  std::memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, 0L);   /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, (int) 0);
> +  std::memset (buf, sizeof buf, -0);
> +}
> +
> +void
> +bar ()
> +{
> +  foo<0> ();
> +  std::memset (buf, sizeof buf, 0);    /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, 1, 1 - 1);
> +  std::memset (buf, 1, 0 - 0);
> +  std::memset (buf, 0, 0);
> +  std::memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, 0L);   /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  std::memset (buf, sizeof buf, (int) 0);
> +  std::memset (buf, sizeof buf, -0);
> +}
> --- gcc/testsuite/c-c++-common/Wmemset-transposed-args1.c.jj    2014-07-09 10:41:38.969017900 +0200
> +++ gcc/testsuite/c-c++-common/Wmemset-transposed-args1.c       2014-07-09 11:51:12.000000000 +0200
> @@ -0,0 +1,29 @@
> +/* { 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, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, 1, 1 - 1);
> +  memset (buf, 1, 0 - 0);
> +  memset (buf, 0, 0);
> +  memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, 0L);        /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
> +  memset (buf, sizeof buf, (int) 0);
> +  memset (buf, sizeof buf, -0);
> +}
> --- gcc/c-family/c.opt.jj       2014-07-08 17:38:09.249268240 +0200
> +++ gcc/c-family/c.opt  2014-07-09 10:41:38.909018201 +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
>
>
>         Jakub

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

[Jakub Jelinek]

On Wed, Jul 09, 2014 at 12:26:09PM +0200, Richard Biener wrote:
> > I suppose we could use an INTEGER_CST distinct from the one in
> > TYPE_CACHED_VALUES for raw 0, with a TREE_LANG_FLAG set.
> 
> Ick.  (please no - at least make sure it doesn't survive anywhere to the
> middle-end, like fold or gimple).

Why?  I don't think the middle-end ever compares INTEGER_CSTs for equality.

Here is my WIP on Jason's suggestion (untested of course other than the
testcase), the C++ FE is changed to do that, the C FE is not.

Sure, I could add another bool argument to
cp_parser_parenthesized_expression_list whether the caller expects to see
the literal zeros, and replace them with normal INTEGER_CSTs after the test.

--- gcc/c/c-typeck.c.jj	2014-07-08 17:38:09.612266656 +0200
+++ gcc/c/c-typeck.c	2014-07-09 10:41:38.931018088 +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/cp-tree.h.jj	2014-07-07 10:39:44.000000000 +0200
+++ gcc/cp/cp-tree.h	2014-07-09 11:48:12.534274257 +0200
@@ -4187,6 +4187,10 @@ more_aggr_init_expr_args_p (const aggr_i
 #define SIZEOF_EXPR_TYPE_P(NODE) \
   TREE_LANG_FLAG_0 (SIZEOF_EXPR_CHECK (NODE))
 
+/* True if INTEGER_CST is a zero literal seen in function argument list.  */
+#define LITERAL_ZERO_P(NODE) \
+  (INTEGER_CST_CHECK (NODE)->base.nothrow_flag)
+
 /* An enumeration of the kind of tags that C++ accepts.  */
 enum tag_types {
   none_type = 0, /* Not a tag type.  */
--- gcc/cp/parser.c.jj	2014-07-09 10:41:12.000000000 +0200
+++ gcc/cp/parser.c	2014-07-09 11:36:40.195693369 +0200
@@ -6142,6 +6142,19 @@ cp_parser_postfix_expression (cp_parser
 		  }
 	      }
 
+	  if (warn_memset_transposed_args
+	      && 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
+	      && integer_zerop ((*args)[2])
+	      && LITERAL_ZERO_P ((*args)[2])
+	      && !(integer_zerop ((*args)[1])
+		   && LITERAL_ZERO_P ((*args)[1])))
+	    warning (OPT_Wmemset_transposed_args,
+		     "%<memset%> used with constant zero length parameter; "
+		     "this could be due to transposed parameters");
+
 	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
 	      {
 		tree instance = TREE_OPERAND (postfix_expression, 0);
@@ -6630,6 +6643,10 @@ cp_parser_postfix_dot_deref_expression (
   return postfix_expression;
 }
 
+/* Cache of LITERAL_ZERO_P constants.  */
+
+static GTY(()) tree literal_zeros[itk_none];
+
 /* Parse a parenthesized expression-list.
 
    expression-list:
@@ -6724,7 +6741,50 @@ cp_parser_parenthesized_expression_list
 		  *non_constant_p = true;
 	      }
 	    else
-	      expr = cp_parser_assignment_expression (parser, cast_p, NULL);
+	      {
+		expr = NULL_TREE;
+		cp_token *tok = cp_lexer_peek_token (parser->lexer);
+		switch (tok->type)
+		  {
+		  case CPP_NUMBER:
+		  case CPP_CHAR:
+		  case CPP_WCHAR:
+		  case CPP_CHAR16:
+		  case CPP_CHAR32:
+		    /* If a parameter is literal zero alone, remember it
+		       for -Wmemset-transposed-args warning.  */
+		    if (integer_zerop (tok->u.value)
+			&& !TREE_OVERFLOW (tok->u.value)
+			&& is_attribute_list == non_attr
+			&& (cp_lexer_peek_nth_token (parser->lexer, 2)->type
+			    == CPP_COMMA
+			    || cp_lexer_peek_nth_token (parser->lexer, 2)->type
+			       == CPP_CLOSE_PAREN))
+		      {
+			unsigned int i;
+			for (i = 0; i < itk_none; ++i)
+			  if (TREE_TYPE (tok->u.value) == integer_types[i])
+			    break;
+			if (i < itk_none && literal_zeros[i])
+			  expr = literal_zeros[i];
+			else
+			  {
+			    expr = copy_node (tok->u.value);
+			    LITERAL_ZERO_P (expr) = 1;
+			    if (i < itk_none)
+			      literal_zeros[i] = expr;
+			  }
+			/* Consume the 0 token (or '\0', 0LL etc.).  */
+			cp_lexer_consume_token (parser->lexer);
+		      }
+		    break;
+		  default:
+		    break;
+		  }
+		if (expr == NULL_TREE)
+		  expr = cp_parser_assignment_expression (parser, cast_p,
+							  NULL);
+	      }
 
 	    if (fold_expr_p)
 	      expr = fold_non_dependent_expr (expr);
--- gcc/doc/invoke.texi.jj	2014-07-08 17:38:09.301268037 +0200
+++ gcc/doc/invoke.texi	2014-07-09 10:41:38.950017994 +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/g++.dg/warn/Wmemset-transposed-args-1.C.jj	2014-07-09 10:41:38.969017900 +0200
+++ gcc/testsuite/g++.dg/warn/Wmemset-transposed-args-1.C	2014-07-09 12:12:40.850993778 +0200
@@ -0,0 +1,74 @@
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+typedef __SIZE_TYPE__ size_t;
+extern "C" void *memset (void *, int, size_t);
+char buf[1024];
+namespace std
+{
+  extern "C" void *memset (void *, int, size_t);
+}
+
+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, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, N);
+  memset (buf, 1, 1 - 1);
+  memset (buf, 1, 0 - 0);
+  memset (buf, 1, N - N);
+  memset (buf, 0, 0);
+  memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, (int) 0);
+  memset (buf, sizeof buf, -0);
+}
+
+template <int N>
+void
+baz ()
+{
+  std::memset (buf, sizeof buf, 0);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, N);
+  std::memset (buf, 1, 1 - 1);
+  std::memset (buf, 1, 0 - 0);
+  std::memset (buf, 1, N - N);
+  std::memset (buf, 0, 0);
+  std::memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, (int) 0);
+  std::memset (buf, sizeof buf, -0);
+}
+
+void
+bar ()
+{
+  foo<0> ();
+  std::memset (buf, sizeof buf, 0);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, '\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 1, 1 - 1);
+  std::memset (buf, 1, 0 - 0);
+  std::memset (buf, 0, 0);
+  std::memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  std::memset (buf, sizeof buf, (int) 0);
+  std::memset (buf, sizeof buf, -0);
+}
--- gcc/testsuite/c-c++-common/Wmemset-transposed-args1.c.jj	2014-07-09 10:41:38.969017900 +0200
+++ gcc/testsuite/c-c++-common/Wmemset-transposed-args1.c	2014-07-09 11:51:12.000000000 +0200
@@ -0,0 +1,29 @@
+/* { 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, sizeof buf, L'\0'); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 1, 1 - 1);
+  memset (buf, 1, 0 - 0);
+  memset (buf, 0, 0);
+  memset (buf, 1 - 1, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, 0 - 0, 0); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0L);	/* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0UL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0LL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, 0ULL); /* { dg-warning ".memset. used with constant zero length parameter; this could be due to transposed parameters" } */
+  memset (buf, sizeof buf, (int) 0);
+  memset (buf, sizeof buf, -0);
+}
--- gcc/c-family/c.opt.jj	2014-07-08 17:38:09.249268240 +0200
+++ gcc/c-family/c.opt	2014-07-09 10:41:38.909018201 +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


	Jakub

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

[Richard Biener]

On Wed, Jul 9, 2014 at 12:33 AM, Jason Merrill <jason@redhat.com> wrote:
> On 07/08/2014 12:38 PM, Carlos O'Donell wrote:
>>
>> What rationale would you give for not warning on 1-1?
>
>
> Because it's not likely to be a case of argument transposition; it's more
> likely to be an expression that just happens to evaluate to 0, which is fine
> as a length argument to memset.
>
>
> On 07/08/2014 01:31 PM, Jakub Jelinek wrote:
>>
>> On Tue, Jul 08, 2014 at 03:24:52PM -0400, Jason Merrill wrote:
>>>
>>> I don't think we want to warn about e.g. 1-1, only about literal 0.
>>
>>
>> Well, at least literal 0 and '\0'.
>
>
> Right, I consider '\0' to be a literal 0.
>
>
>> But in the C++ FE there isn't something like that.  Do you think we
>> shouldn't warn even if e.g. the last argument is a template parameter
>> that turns out to be 0, so warn only during parsing and check for literal
>> 0 and not warn again during instantiation?
>
>
> Yes, that's what I think.
>
>
>> Any suggestions how to find out
>> if it was literal 0 or something that folded to 0 in the C++ FE?
>
>
> I suppose we could use an INTEGER_CST distinct from the one in
> TYPE_CACHED_VALUES for raw 0, with a TREE_LANG_FLAG set.

Ick.  (please no - at least make sure it doesn't survive anywhere to the
middle-end, like fold or gimple).

Richard.

> Jason
>

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

[Jason Merrill]

On 07/08/2014 12:38 PM, Carlos O'Donell wrote:
> What rationale would you give for not warning on 1-1?

Because it's not likely to be a case of argument transposition; it's 
more likely to be an expression that just happens to evaluate to 0, 
which is fine as a length argument to memset.

On 07/08/2014 01:31 PM, Jakub Jelinek wrote:
> On Tue, Jul 08, 2014 at 03:24:52PM -0400, Jason Merrill wrote:
>> I don't think we want to warn about e.g. 1-1, only about literal 0.
>
> Well, at least literal 0 and '\0'.

Right, I consider '\0' to be a literal 0.

> But in the C++ FE there isn't something like that.  Do you think we
> shouldn't warn even if e.g. the last argument is a template parameter
> that turns out to be 0, so warn only during parsing and check for literal
> 0 and not warn again during instantiation?

Yes, that's what I think.

> Any suggestions how to find out
> if it was literal 0 or something that folded to 0 in the C++ FE?

I suppose we could use an INTEGER_CST distinct from the one in 
TYPE_CACHED_VALUES for raw 0, with a TREE_LANG_FLAG set.

Jason

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

[Jakub Jelinek]

On Tue, Jul 08, 2014 at 03:24:52PM -0400, Jason Merrill wrote:
> I don't think we want to warn about e.g. 1-1, only about literal 0.

Well, at least literal 0 and '\0'.  In any case, it seems both the C and C++
FEs fold the arguments too early, already during the parsing of the argument
list.  In the C FE, there is original_code in the c_expr struct, so perhaps
I could somehow propagate it to the caller for the first few arguments
and test that original_code is INTEGER_CST in addition to integer_zerop
to check for literal 0.
But in the C++ FE there isn't something like that.  Do you think we
shouldn't warn even if e.g. the last argument is a template parameter
that turns out to be 0, so warn only during parsing and check for literal
0 and not warn again during instantiation?  Any suggestions how to find out
if it was literal 0 or something that folded to 0 in the C++ FE?

	Jakub

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

[Carlos O'Donell]

On 07/08/2014 03:24 PM, Jason Merrill wrote:
> I don't think we want to warn about e.g. 1-1, only about literal 0.

What rationale would you give for not warning on 1-1?

Cheers,
Carlos.

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

[Carlos O'Donell]

On 07/08/2014 08:50 AM, Jakub Jelinek wrote:
> 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.

This seems like a sensible solution to fixing the false positives
caused by jump threading (I haven't looked into that in detail,
just briefly).

I would prefer we enable this for 4.10/5.0+ if only to avoid the
fallout (new warnings) that would happen for the distributions.

We can fix the glibc header once the fix is in gcc, unless someone
objects to the fix itself.

Cheers,
Carlos.

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

[Jason Merrill]

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

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

[Jakub Jelinek]

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