From patchwork Tue Jul 8 19:24:52 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jason Merrill X-Patchwork-Id: 1956 Received: (qmail 12335 invoked by alias); 8 Jul 2014 19:25:04 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 12314 invoked by uid 89); 8 Jul 2014 19:25:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=unavailable version=3.3.2 X-HELO: mx5-phx2.redhat.com Date: Tue, 8 Jul 2014 15:24:52 -0400 (EDT) Subject: RE: [RFC PATCH] -Wmemset-transposed-args (PR middle-end/61294) Message-ID: From: Jason Merrill To: Jakub Jelinek , "Joseph S. Myers" , "Carlos O'Donell" , Siddhesh Poyarekar Cc: gcc-patches@gcc.gnu.org, libc-alpha@sourceware.org MIME-Version: 1.0 In-Reply-To: <20140708125017.GN31640@tucnak.redhat.com> References: <20140708125017.GN31640@tucnak.redhat.com> I don't think we want to warn about e.g. 1-1, only about literal 0. -------- Original Message -------- From: Jakub Jelinek Sent: Tue, Jul 8, 2014 05:50 AM To: Joseph S. Myers ; Jason Merrill ; Carlos O'Donell ; Siddhesh Poyarekar 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 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. Jakub --- 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, + "% 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 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 +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> (); +}