[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
This revision was automatically updated to reflect the committed changes.
Closed by commit rC337470: [Sema] Add a new warning, -Wmemset-transposed-args
(authored by epilk, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D49112?vs=156178=156298#toc
Repository:
rC Clang
https://reviews.llvm.org/D49112
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaChecking.cpp
test/Sema/transpose-memset.c
Index: test/Sema/transpose-memset.c
===
--- test/Sema/transpose-memset.c
+++ test/Sema/transpose-memset.c
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s
+// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s
+
+#define memset(...) __builtin_memset(__VA_ARGS__)
+#define bzero(x,y) __builtin_memset(x, 0, y)
+#define real_bzero(x,y) __builtin_bzero(x,y)
+
+int array[10];
+int *ptr;
+
+int main() {
+ memset(array, sizeof(array), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+ memset(array, sizeof(array), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, sizeof(ptr), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+ memset(ptr, sizeof(*ptr) * 10, 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, 10 * sizeof(int *), 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, 10 * sizeof(int *) + 10, 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, sizeof(char) * sizeof(int *), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(array, sizeof(array), sizeof(array)); // Uh... fine I guess.
+ memset(array, 0, sizeof(array));
+ memset(ptr, 0, sizeof(int *) * 10);
+ memset(array, (int)sizeof(array), (0)); // no warning
+ memset(array, (int)sizeof(array), 32); // no warning
+ memset(array, 32, (0)); // no warning
+
+ bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}}
+ real_bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}}
+}
+
+void macros() {
+#define ZERO 0
+ int array[10];
+ memset(array, 0xff, ZERO); // no warning
+ // Still emit a diagnostic for memsetting a sizeof expression:
+ memset(array, sizeof(array), ZERO); // expected-warning{{'sizeof'}} expected-note{{cast}}
+ bzero(array, ZERO); // no warning
+ real_bzero(array, ZERO); // no warning
+#define NESTED_DONT_DIAG\
+ memset(array, 0xff, ZERO);\
+ real_bzero(array, ZERO);
+
+ NESTED_DONT_DIAG;
+
+#define NESTED_DO_DIAG \
+ memset(array, 0xff, 0); \
+ real_bzero(array, 0)
+
+ NESTED_DO_DIAG; // expected-warning{{'size' argument to memset}} expected-warning{{'size' argument to bzero}} expected-note2{{parenthesize}}
+
+#define FN_MACRO(p) \
+ memset(array, 0xff, p)
+
+ FN_MACRO(ZERO);
+ FN_MACRO(0); // FIXME: should we diagnose this?
+
+ __builtin_memset(array, 0, ZERO); // no warning
+ __builtin_bzero(array, ZERO);
+ __builtin_memset(array, 0, 0); // expected-warning{{'size' argument to memset}} // expected-note{{parenthesize}}
+ __builtin_bzero(array, 0); // expected-warning{{'size' argument to bzero}} // expected-note{{parenthesize}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8629,24 +8629,26 @@
return nullptr;
}
+static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
+ if (const auto *Unary = dyn_cast(E))
+if (Unary->getKind() == UETT_SizeOf)
+ return Unary;
+ return nullptr;
+}
+
/// If E is a sizeof expression, returns its argument expression,
/// otherwise returns NULL.
static const Expr *getSizeOfExprArg(const Expr *E) {
- if (const UnaryExprOrTypeTraitExpr *SizeOf =
- dyn_cast(E))
-if (SizeOf->getKind() == UETT_SizeOf && !SizeOf->isArgumentType())
+ if (const
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM aside from a minor nit.
Comment at: clang/lib/Sema/SemaChecking.cpp:8751
+
+ if (auto *BO = dyn_cast(SizeofExpr)) {
+if (BO->getOpcode() != BO_Mul && BO->getOpcode() != BO_Add)
`const auto *`
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
erik.pilkington added inline comments.
Comment at: clang/lib/Sema/SemaChecking.cpp:7984
+ if (isa(ThirdArg) &&
+ cast(ThirdArg)->getValue() == 0) {
+WarningKind = 0;
Quuxplusone wrote:
> > Suppress the diagnostic in cases like `memset(ptr, 0xff, PADDING)`, when
> > `PADDING` is a macro defined to be `0`.
>
> Would it suffice to treat a literal `0xFF` in the exact same way you treat a
> literal `0`? That is,
> ```
> if (SecondArg is integer literal 0 or 255 || ThirdArg involves sizeof) {
> do nothing
> } else if (ThirdArg is integer literal 0 or 255 || SecondArg involves sizeof)
> {
> diagnose it
> }
> ```
> You should add a test case proving that `memset(x, 0, 0)` doesn't get
> diagnosed (assuming the two `0`s come from two different macro expansions,
> for example).
> My sketch above would fail to diagnose `memset(x, 0, 255)`. I don't know if
> this is a feature or a bug. :) You probably ought to have a test case for
> that, either way, just to demonstrate the expected behavior.
I definitely don't think that ThirdArg being 255 should lead to a diagnostic,
regardless of what SecondArg is. I'm fine with omitting a diagnostic in
`memset(ptr, 255, 0)`, but if the user explicitly spelled out `0` then I would
probably prefer to diagnose. FWIW, for the case I found where we emitted a
false-positive the second argument was 0xd9 or something.
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
erik.pilkington updated this revision to Diff 156178.
erik.pilkington marked 5 inline comments as done.
erik.pilkington added a comment.
This revision is now accepted and ready to land.
In this new patch:
- Add support for __builtin_bzero (-Wsuspicious-bzero), improve diagnostics for
platforms that define bzero to __builtin_memset
- Suppress the diagnostic when the `0` in memset(ptr, something, 0) came from a
macro expansion.
- Address review comments
Thanks!
https://reviews.llvm.org/D49112
Files:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/transpose-memset.c
Index: clang/test/Sema/transpose-memset.c
===
--- /dev/null
+++ clang/test/Sema/transpose-memset.c
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s
+// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s
+
+#define memset(...) __builtin_memset(__VA_ARGS__)
+#define bzero(x,y) __builtin_memset(x, 0, y)
+#define real_bzero(x,y) __builtin_bzero(x,y)
+
+int array[10];
+int *ptr;
+
+int main() {
+ memset(array, sizeof(array), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+ memset(array, sizeof(array), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, sizeof(ptr), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+ memset(ptr, sizeof(*ptr) * 10, 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, 10 * sizeof(int *), 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, 10 * sizeof(int *) + 10, 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, sizeof(char) * sizeof(int *), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(array, sizeof(array), sizeof(array)); // Uh... fine I guess.
+ memset(array, 0, sizeof(array));
+ memset(ptr, 0, sizeof(int *) * 10);
+ memset(array, (int)sizeof(array), (0)); // no warning
+ memset(array, (int)sizeof(array), 32); // no warning
+ memset(array, 32, (0)); // no warning
+
+ bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}}
+ real_bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}}
+}
+
+void macros() {
+#define ZERO 0
+ int array[10];
+ memset(array, 0xff, ZERO); // no warning
+ // Still emit a diagnostic for memsetting a sizeof expression:
+ memset(array, sizeof(array), ZERO); // expected-warning{{'sizeof'}} expected-note{{cast}}
+ bzero(array, ZERO); // no warning
+ real_bzero(array, ZERO); // no warning
+#define NESTED_DONT_DIAG\
+ memset(array, 0xff, ZERO);\
+ real_bzero(array, ZERO);
+
+ NESTED_DONT_DIAG;
+
+#define NESTED_DO_DIAG \
+ memset(array, 0xff, 0); \
+ real_bzero(array, 0)
+
+ NESTED_DO_DIAG; // expected-warning{{'size' argument to memset}} expected-warning{{'size' argument to bzero}} expected-note2{{parenthesize}}
+
+#define FN_MACRO(p) \
+ memset(array, 0xff, p)
+
+ FN_MACRO(ZERO);
+ FN_MACRO(0); // FIXME: should we diagnose this?
+
+ __builtin_memset(array, 0, ZERO); // no warning
+ __builtin_bzero(array, ZERO);
+ __builtin_memset(array, 0, 0); // expected-warning{{'size' argument to memset}} // expected-note{{parenthesize}}
+ __builtin_bzero(array, 0); // expected-warning{{'size' argument to bzero}} // expected-note{{parenthesize}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8629,24 +8629,26 @@
return nullptr;
}
+static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
+ if (const auto *Unary = dyn_cast(E))
+if (Unary->getKind() == UETT_SizeOf)
+ return Unary;
+ return nullptr;
+}
+
/// If E is a sizeof expression, returns its argument expression,
/// otherwise returns
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
aaron.ballman added inline comments.
Comment at: clang/lib/Sema/SemaChecking.cpp:7975-7976
+static void CheckMemsetSizeof(Sema , unsigned BId, const CallExpr *Call) {
+ if (BId != Builtin::BImemset)
+return;
+
erik.pilkington wrote:
> aaron.ballman wrote:
> > This functionality should apply equally to `wmemset()` as well, should it
> > not? The only difference I can think of would be that the type should be
> > cast to `wchar_t` instead of `int` to silence the warning.
> Looks like clang doesn't have a builtin for wmemset, its just defined as a
> normal function. I suppose that we could still try to diagnose calls to
> top-level functions named wmemset, but thats unprecedented and doesn't really
> seem worth the trouble.
I'm fine leaving it off then; I agree it's not worth the trouble.
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
Quuxplusone added inline comments.
Comment at: clang/lib/Sema/SemaChecking.cpp:7984
+ if (isa(ThirdArg) &&
+ cast(ThirdArg)->getValue() == 0) {
+WarningKind = 0;
> Suppress the diagnostic in cases like `memset(ptr, 0xff, PADDING)`, when
> `PADDING` is a macro defined to be `0`.
Would it suffice to treat a literal `0xFF` in the exact same way you treat a
literal `0`? That is,
```
if (SecondArg is integer literal 0 or 255 || ThirdArg involves sizeof) {
do nothing
} else if (ThirdArg is integer literal 0 or 255 || SecondArg involves sizeof) {
diagnose it
}
```
You should add a test case proving that `memset(x, 0, 0)` doesn't get diagnosed
(assuming the two `0`s come from two different macro expansions, for example).
My sketch above would fail to diagnose `memset(x, 0, 255)`. I don't know if
this is a feature or a bug. :) You probably ought to have a test case for
that, either way, just to demonstrate the expected behavior.
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
erik.pilkington planned changes to this revision.
erik.pilkington added a comment.
In https://reviews.llvm.org/D49112#1158793, @thakis wrote:
> lgtm assuming you ran this on some large code base to make sure it doesn't
> have false positives.
Sorry for the delay here, I was running this on some very large internal code
bases. This is overwhelmingly true-positive, but there are 2 improvements I
want to make here before landing this:
- Suppress the diagnostic in cases like `memset(ptr, 0xff, PADDING)`, when
PADDING is a macro defined to be 0.
- Some platforms #define bzero to `__builtin_memset`, so we should recognize
when we're in a macro expansion to bzero and emit a more accurate diagnostic
for `bzero(ary, 0);`. We might as well add support for `__builtin_bzero` too.
Comment at: clang/lib/Sema/SemaChecking.cpp:7975-7976
+static void CheckMemsetSizeof(Sema , unsigned BId, const CallExpr *Call) {
+ if (BId != Builtin::BImemset)
+return;
+
aaron.ballman wrote:
> This functionality should apply equally to `wmemset()` as well, should it
> not? The only difference I can think of would be that the type should be cast
> to `wchar_t` instead of `int` to silence the warning.
Looks like clang doesn't have a builtin for wmemset, its just defined as a
normal function. I suppose that we could still try to diagnose calls to
top-level functions named wmemset, but thats unprecedented and doesn't really
seem worth the trouble.
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
Quuxplusone added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:662
+def note_suspicious_sizeof_memset_silence : Note<
+ "%select{parenthesize the third argument|cast the second argument to 'int'}0
to silence">;
+
aaron.ballman wrote:
> erik.pilkington wrote:
> > Quuxplusone wrote:
> > > If it were my codebase, I'd rather see a cast to `(unsigned char)` than a
> > > cast to `(int)`. (The second argument to memset is supposed to be a
> > > single byte.) Why did you pick `(int)` specifically?
> > I chose `int` because that's the actual type of the second parameter to
> > `memset`, it just gets casted down to `unsigned char` internally. FWIW,
> > either type will suppress the warning. I'm fine with recommending `unsigned
> > char` if you have a strong preference for it.
> My preference is for `int` as well.
Personally I have a strong preference for "any 8-bit type" — the important
thing is that it needs to indicate to the reader that it's intended to be the
single-byte value that `memset` expects. I even want my compiler to diagnose
`memset(x, 0x1234, z)` as a bug!
But I'm not inclined to fight hard, because this is all about the mechanism of
*suppressing* of the warning, and I don't imagine we'll see too many people
trying to suppress it.
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
aaron.ballman added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:662
+def note_suspicious_sizeof_memset_silence : Note<
+ "%select{parenthesize the third argument|cast the second argument to 'int'}0
to silence">;
+
erik.pilkington wrote:
> Quuxplusone wrote:
> > If it were my codebase, I'd rather see a cast to `(unsigned char)` than a
> > cast to `(int)`. (The second argument to memset is supposed to be a single
> > byte.) Why did you pick `(int)` specifically?
> I chose `int` because that's the actual type of the second parameter to
> `memset`, it just gets casted down to `unsigned char` internally. FWIW,
> either type will suppress the warning. I'm fine with recommending `unsigned
> char` if you have a strong preference for it.
My preference is for `int` as well.
Comment at: clang/lib/Sema/SemaChecking.cpp:7975-7976
+static void CheckMemsetSizeof(Sema , unsigned BId, const CallExpr *Call) {
+ if (BId != Builtin::BImemset)
+return;
+
This functionality should apply equally to `wmemset()` as well, should it not?
The only difference I can think of would be that the type should be cast to
`wchar_t` instead of `int` to silence the warning.
Comment at: clang/lib/Sema/SemaChecking.cpp:7997
+
+
+ // If the function is defined as a builtin macro, do not show macro
expansion.
Spurious whitespace.
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.
lgtm assuming you ran this on some large code base to make sure it doesn't have
false positives.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:659
+def warn_suspicious_sizeof_memset : Warning<
+ "%select{'size' argument to memset is '0'|setting buffer to a 'sizeof'
expression}0; did you mean to transpose the last two arguments?">,
+ InGroup;
nit: stay below 80 columns
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
erik.pilkington added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:662
+def note_suspicious_sizeof_memset_silence : Note<
+ "%select{parenthesize the third argument|cast the second argument to 'int'}0
to silence">;
+
Quuxplusone wrote:
> If it were my codebase, I'd rather see a cast to `(unsigned char)` than a
> cast to `(int)`. (The second argument to memset is supposed to be a single
> byte.) Why did you pick `(int)` specifically?
I chose `int` because that's the actual type of the second parameter to
`memset`, it just gets casted down to `unsigned char` internally. FWIW, either
type will suppress the warning. I'm fine with recommending `unsigned char` if
you have a strong preference for it.
Comment at: clang/lib/Sema/SemaChecking.cpp:7962
+ if (auto *BO = dyn_cast(SizeofExpr)) {
+if (BO->getOpcode() != BO_Mul)
+ return false;
Quuxplusone wrote:
> Honestly, if the second argument to `memset` involves `sizeof` *at all*, it's
> probably a bug, isn't it?
>
> memset(, sizeof foo + 10, 0xff);
> memset(, sizeof foo * sizeof bar, 0xff);
>
> Should Clang really go out of its way to silence the warning in these cases?
Sure, that seems reasonable. The new patch makes this check less conservative.
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
erik.pilkington updated this revision to Diff 154833.
erik.pilkington added a comment.
Address @Quuxplusone comments.
https://reviews.llvm.org/D49112
Files:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/transpose-memset.c
Index: clang/test/Sema/transpose-memset.c
===
--- /dev/null
+++ clang/test/Sema/transpose-memset.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s
+// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s
+
+#define memset(...) __builtin_memset(__VA_ARGS__)
+
+int array[10];
+int *ptr;
+
+int main() {
+ memset(array, sizeof(array), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+ memset(array, sizeof(array), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, sizeof(ptr), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+ memset(ptr, sizeof(*ptr) * 10, 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, 10 * sizeof(int *), 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, 10 * sizeof(int *) + 10, 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, sizeof(char) * sizeof(int *), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(array, sizeof(array), sizeof(array)); // Uh... fine I guess.
+ memset(array, 0, sizeof(array));
+ memset(ptr, 0, sizeof(int *) * 10);
+ memset(array, (int)sizeof(array), (0)); // no warning
+ memset(array, (int)sizeof(array), 32); // no warning
+ memset(array, 32, (0)); // no warning
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -7839,24 +7839,26 @@
return nullptr;
}
+static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
+ if (const auto *Unary = dyn_cast(E))
+if (Unary->getKind() == UETT_SizeOf)
+ return Unary;
+ return nullptr;
+}
+
/// If E is a sizeof expression, returns its argument expression,
/// otherwise returns NULL.
static const Expr *getSizeOfExprArg(const Expr *E) {
- if (const UnaryExprOrTypeTraitExpr *SizeOf =
- dyn_cast(E))
-if (SizeOf->getKind() == UETT_SizeOf && !SizeOf->isArgumentType())
+ if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
+if (!SizeOf->isArgumentType())
return SizeOf->getArgumentExpr()->IgnoreParenImpCasts();
-
return nullptr;
}
/// If E is a sizeof expression, returns its argument type.
static QualType getSizeOfArgType(const Expr *E) {
- if (const UnaryExprOrTypeTraitExpr *SizeOf =
- dyn_cast(E))
-if (SizeOf->getKind() == UETT_SizeOf)
- return SizeOf->getTypeOfArgument();
-
+ if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
+return SizeOf->getTypeOfArgument();
return QualType();
}
@@ -7952,6 +7954,56 @@
}
+/// Detect if \c SizeofExpr is likely to calculate the sizeof an object.
+static bool doesExprLikelyComputeSize(const Expr *SizeofExpr) {
+ SizeofExpr = SizeofExpr->IgnoreParenImpCasts();
+
+ if (auto *BO = dyn_cast(SizeofExpr)) {
+if (BO->getOpcode() != BO_Mul && BO->getOpcode() != BO_Add)
+ return false;
+
+return doesExprLikelyComputeSize(BO->getLHS()) ||
+ doesExprLikelyComputeSize(BO->getRHS());
+ }
+
+ return getAsSizeOfExpr(SizeofExpr) != nullptr;
+}
+
+/// Diagnose cases like 'memset(buf, sizeof(buf), 0)', which should have the
+/// last two arguments transposed.
+static void CheckMemsetSizeof(Sema , unsigned BId, const CallExpr *Call) {
+ if (BId != Builtin::BImemset)
+return;
+
+ int WarningKind;
+ SourceLocation DiagLoc;
+
+ // If we're memsetting 0 bytes, then this is likely a programmer error.
+ const Expr *ThirdArg = Call->getArg(2)->IgnoreImpCasts();
+ if (isa(ThirdArg) &&
+ cast(ThirdArg)->getValue() == 0) {
+WarningKind = 0;
+DiagLoc = ThirdArg->getExprLoc();
+ }
+ // If the second argument is a sizeof expression and the third isn't,
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
Quuxplusone added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:662
+def note_suspicious_sizeof_memset_silence : Note<
+ "%select{parenthesize the third argument|cast the second argument to 'int'}0
to silence">;
+
If it were my codebase, I'd rather see a cast to `(unsigned char)` than a cast
to `(int)`. (The second argument to memset is supposed to be a single byte.)
Why did you pick `(int)` specifically?
Comment at: clang/lib/Sema/SemaChecking.cpp:7962
+ if (auto *BO = dyn_cast(SizeofExpr)) {
+if (BO->getOpcode() != BO_Mul)
+ return false;
Honestly, if the second argument to `memset` involves `sizeof` *at all*, it's
probably a bug, isn't it?
memset(, sizeof foo + 10, 0xff);
memset(, sizeof foo * sizeof bar, 0xff);
Should Clang really go out of its way to silence the warning in these cases?
Repository:
rC Clang
https://reviews.llvm.org/D49112
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args
erik.pilkington created this revision.
erik.pilkington added reviewers: rsmith, aaron.ballman, arphaman.
Herald added a subscriber: dexonsmith.
This warning tries to catch programs that incorrectly call memset with the
second and third arguments transposed, ie `memset(ary, sizeof(ary), 0)` instead
of `memset(ary, 0, sizeof(ary))`. This is done by looking at two factors: 1) if
the last argument is `0`, then this is likely a bug (looks this is what GCC's
implementation does) and 2) if the last argument isn't `0`, but the second
argument is a `sizeof`expression. This catches cases like `memset(ary,
sizeof(ary), 0xff)`. I also grouped a couple of related diagnostics that deal
with these c functions into a new group, -Wsuspicious-memaccess.
llvm.org/PR36242
Thanks for taking a look!
Erik
Repository:
rC Clang
https://reviews.llvm.org/D49112
Files:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/test/Sema/transpose-memset.c
Index: clang/test/Sema/transpose-memset.c
===
--- /dev/null
+++ clang/test/Sema/transpose-memset.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -Wmemset-transposed-args -verify %s
+// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s
+
+#define memset(...) __builtin_memset(__VA_ARGS__)
+
+int array[10];
+int *ptr;
+
+int main() {
+ memset(array, sizeof(array), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+ memset(array, sizeof(array), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, sizeof(ptr), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+ memset(ptr, sizeof(*ptr) * 10, 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(ptr, 10 * sizeof(int *), 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+ memset(array, sizeof(array), sizeof(array)); // Uh... fine I guess.
+ memset(array, 0, sizeof(array));
+ memset(ptr, 0, sizeof(int *) * 10);
+ memset(array, (int)sizeof(array), (0)); // no warning
+ memset(array, (int)sizeof(array), 32); // no warning
+ memset(array, 32, (0)); // no warning
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -7839,24 +7839,26 @@
return nullptr;
}
+static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
+ if (const auto *Unary = dyn_cast(E))
+if (Unary->getKind() == UETT_SizeOf)
+ return Unary;
+ return nullptr;
+}
+
/// If E is a sizeof expression, returns its argument expression,
/// otherwise returns NULL.
static const Expr *getSizeOfExprArg(const Expr *E) {
- if (const UnaryExprOrTypeTraitExpr *SizeOf =
- dyn_cast(E))
-if (SizeOf->getKind() == UETT_SizeOf && !SizeOf->isArgumentType())
+ if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
+if (!SizeOf->isArgumentType())
return SizeOf->getArgumentExpr()->IgnoreParenImpCasts();
-
return nullptr;
}
/// If E is a sizeof expression, returns its argument type.
static QualType getSizeOfArgType(const Expr *E) {
- if (const UnaryExprOrTypeTraitExpr *SizeOf =
- dyn_cast(E))
-if (SizeOf->getKind() == UETT_SizeOf)
- return SizeOf->getTypeOfArgument();
-
+ if (const UnaryExprOrTypeTraitExpr *SizeOf = getAsSizeOfExpr(E))
+return SizeOf->getTypeOfArgument();
return QualType();
}
@@ -7952,6 +7954,57 @@
}
+/// Detect if \c E is likely to calculate the sizeof an object.
+static bool doesExprLikelyComputeSize(const Expr *SizeofExpr) {
+ SizeofExpr = SizeofExpr->IgnoreParenImpCasts();
+
+ if (auto *BO = dyn_cast(SizeofExpr)) {
+if (BO->getOpcode() != BO_Mul)
+ return false;
+
+const Expr *LHSSizeof = getAsSizeOfExpr(BO->getLHS()),
+ *RHSSizeof = getAsSizeOfExpr(BO->getRHS());
+return static_cast(LHSSizeof) != static_cast(RHSSizeof);
+ }
+
+ return getAsSizeOfExpr(SizeofExpr) != nullptr;
+}
+
+/// Diagnose cases like 'memset(buf, sizeof(buf), 0)', which should have the
+/// last two arguments transposed.
+static void CheckMemsetSizeof(Sema , unsigned BId, const CallExpr *Call) {
+ if (BId != Builtin::BImemset)
+return;
+
+ int WarningKind;
+ SourceLocation DiagLoc;
+
+ // If we're memsetting 0 bytes, then this
