[RFC] -Wmemset-transposed-args (PR middle-end/61294)
Commit Message
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.
Jakub
Comments
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.
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
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
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
>
@@ -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
@@ -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
@@ -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);
}
@@ -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
@@ -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);
+}
@@ -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> ();
+}