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

[Richard Biener <richard.guenther@gmail.com>]

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