From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 574 invoked by alias); 9 Jul 2014 10:51:40 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 553 invoked by uid 89); 9 Jul 2014 10:51:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wg0-f44.google.com Received: from mail-wg0-f44.google.com (HELO mail-wg0-f44.google.com) (74.125.82.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 09 Jul 2014 10:51:36 +0000 Received: by mail-wg0-f44.google.com with SMTP id k14so7139336wgh.15 for ; Wed, 09 Jul 2014 03:51:32 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.195.12.97 with SMTP id ep1mr30526435wjd.26.1404903092476; Wed, 09 Jul 2014 03:51:32 -0700 (PDT) Received: by 10.195.11.202 with HTTP; Wed, 9 Jul 2014 03:51:32 -0700 (PDT) In-Reply-To: <20140709103255.GS31640@tucnak.redhat.com> References: <20140708125017.GN31640@tucnak.redhat.com> <20140708203151.GP31640@tucnak.redhat.com> <53BC71CE.6000504@redhat.com> <20140709103255.GS31640@tucnak.redhat.com> Date: Wed, 09 Jul 2014 10:51:00 -0000 Message-ID: Subject: Re: [RFC PATCH] -Wmemset-transposed-args (PR middle-end/61294) From: Richard Biener To: Jakub Jelinek Cc: Jason Merrill , "Joseph S. Myers" , "Carlos O'Donell" , Siddhesh Poyarekar , GCC Patches , GNU C Library Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg00620.txt.bz2 On Wed, Jul 9, 2014 at 12:32 PM, Jakub Jelinek 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, > + "% 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, > + "% 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 > +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 > +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