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

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.



	Jakub

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

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