| Message ID | 20180407095038.jlfcpdez3fup3wfy@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Apr 06, 2018 at 11:50:38PM -1000, Joey Pabalinas wrote: > Recent changes to the min()/max() macros in include/linux/kernel.h > have added a lot of noise to sparse output; this is due to the > *huge* number of new sizeof(void) warnings, most of which can > largely be ignored. > > Add the -Wsizeof_void flag to enable/disable these warnings on > demand; the warning itself has been disabled by default to reduce the > large influx of noise which was inadvertently added by commit > 3c8ba0d61d04ced9f8 (kernel.h: Retain constant expression output > for max()/min()). > > Update the manpage to document the new flag. Thanks. It looks good to me. I've moved the s/gcc/GCC/ change in a separate commit (cfr. git://github.com/lucvoo/sparse-dev.git sizeof-void ). I've also seen that for GCC this warning is enabled by -Wpointer-arith. I think we should do the same. Mind to respin with this change? There is another thing that I think should be changed: taking the size of a void is quite weird/counter-intuitive and is a constraint violation in standard C. So, I think the flag should be enabled by default but the kernel should have -Wno-pointer-arith in CHECKFLAGS since it do pointer arithmetic on void (and a few places take the size of a function too). Cheers, -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 07, 2018 at 05:31:40PM +0200, Luc Van Oostenryck wrote: > I've moved the s/gcc/GCC/ change in a separate commit > (cfr. git://github.com/lucvoo/sparse-dev.git sizeof-void ). Sure, splitting that change SGTM. > I've also seen that for GCC this warning is enabled by -Wpointer-arith. > I think we should do the same. Mind to respin with this change? You mean change the flag name to -Wpointer-arith? I actually think that's a great idea. Consistency with GCC flags would be good. > There is another thing that I think should be changed: > taking the size of a void is quite weird/counter-intuitive and is > a constraint violation in standard C. So, I think the flag should > be enabled by default but the kernel should have -Wno-pointer-arith > in CHECKFLAGS since it do pointer arithmetic on void (and a few > places take the size of a function too). Well, although Sparse is a general semantic parser I was under the impression that it was mostly used in the kernel. That is what I had in mind when disabling it by default. OTOH, I can see the benefit in doing it your way; anyone else have any opinions on if -Wno-pointer-arith would be better made default in CHECKFLAGS leave this warning on by default in Sparse? I'm spinning up a new patch now; will send the V2 it once we decide on if it should be disabled by default or not.
On Sat, Apr 07, 2018 at 10:24:40AM -1000, Joey Pabalinas wrote: > I'm spinning up a new patch now; will send the V2 it once we decide > on if it should be disabled by default or not. I'll actually just send a tentative V2 now and split the _Bool change completely; not really related at all on second thought.
On Sat, Apr 07, 2018 at 10:24:40AM -1000, Joey Pabalinas wrote: > On Sat, Apr 07, 2018 at 05:31:40PM +0200, Luc Van Oostenryck wrote: > > I've moved the s/gcc/GCC/ change in a separate commit > > (cfr. git://github.com/lucvoo/sparse-dev.git sizeof-void ). > > Sure, splitting that change SGTM. > > > I've also seen that for GCC this warning is enabled by -Wpointer-arith. > > I think we should do the same. Mind to respin with this change? > > You mean change the flag name to -Wpointer-arith? I actually think > that's a great idea. Consistency with GCC flags would be good. > > > There is another thing that I think should be changed: > > taking the size of a void is quite weird/counter-intuitive and is > > a constraint violation in standard C. So, I think the flag should > > be enabled by default but the kernel should have -Wno-pointer-arith > > in CHECKFLAGS since it do pointer arithmetic on void (and a few > > places take the size of a function too). > > Well, although Sparse is a general semantic parser I was under the > impression that it was mostly used in the kernel. That is what I had > in mind when disabling it by default. OTOH, I can see the benefit in > doing it your way; anyone else have any opinions on if -Wno-pointer-arith > would be better made default in CHECKFLAGS leave this warning on > by default in Sparse? It's true that sparse is mainly used for the Linux kernel but as a checker it's normal that it's more strict than GCC. For non-kernel usage I think it's better to leave the warning enabled and adapt CHECKFLAGS. > I'm spinning up a new patch now; will send the V2 it once we decide > on if it should be disabled by default or not. Yes, fine. Cheers, -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/evaluate.c b/evaluate.c index b96696d3a51396800a..33e5fcabc4e755e0b7 100644 --- a/evaluate.c +++ b/evaluate.c @@ -2169,7 +2169,8 @@ static struct symbol *evaluate_sizeof(struct expression *expr) size = type->bit_size; if (size < 0 && is_void_type(type)) { - warning(expr->pos, "expression using sizeof(void)"); + if (Wsizeof_void) + warning(expr->pos, "expression using sizeof(void)"); size = bits_in_char; } diff --git a/lib.c b/lib.c index 645132a892107512a1..35b41ad5becddf6b59 100644 --- a/lib.c +++ b/lib.c @@ -245,6 +245,7 @@ int Wptr_subtraction_blows = 0; int Wreturn_void = 0; int Wshadow = 0; int Wsizeof_bool = 0; +int Wsizeof_void = 0; int Wtautological_compare = 0; int Wtransparent_union = 0; int Wtypesign = 0; @@ -536,6 +537,7 @@ static const struct warning { { "return-void", &Wreturn_void }, { "shadow", &Wshadow }, { "sizeof-bool", &Wsizeof_bool }, + { "sizeof-void", &Wsizeof_void }, { "sparse-error", &Wsparse_error }, { "tautological-compare", &Wtautological_compare }, { "transparent-union", &Wtransparent_union }, diff --git a/lib.h b/lib.h index a9b70b07686801305c..517f933e97d504f2bb 100644 --- a/lib.h +++ b/lib.h @@ -138,6 +138,7 @@ extern int Wptr_subtraction_blows; extern int Wreturn_void; extern int Wshadow; extern int Wsizeof_bool; +extern int Wsizeof_void; extern int Wtautological_compare; extern int Wtransparent_union; extern int Wtypesign; diff --git a/sparse.1 b/sparse.1 index e183204de623efd022..09eecf9cbb06b9acc4 100644 --- a/sparse.1 +++ b/sparse.1 @@ -323,7 +323,25 @@ Sparse does not issue these warnings by default. .B \-Wsizeof-bool Warn when checking the sizeof a _Bool. -C99 does not specify the sizeof a _Bool. gcc uses 1. +C99 does not specify the sizeof a _Bool. GCC uses \fI1\fR. + +Sparse does not issue these warnings by default. +. +.TP +.B \-Wsizeof-void +Warn when checking the sizeof a void. + +C99 does not allow the sizeof operator to be applied to incomplete types +such as void. GCC allows \fBsizeof(void)\fR as an extension and uses \fI1\fR. + +Although non-standard, \fBsizeof(void)\fR is often useful when the intent +is to operate on an expression without evaluating it, e.g. in the following +integer constant expression predicate: + +.nf +#define __is_constexpr(x) \\ + (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8))) +.fi Sparse does not issue these warnings by default. .
Recent changes to the min()/max() macros in include/linux/kernel.h have added a lot of noise to sparse output; this is due to the *huge* number of new sizeof(void) warnings, most of which can largely be ignored. Add the -Wsizeof_void flag to enable/disable these warnings on demand; the warning itself has been disabled by default to reduce the large influx of noise which was inadvertently added by commit 3c8ba0d61d04ced9f8 (kernel.h: Retain constant expression output for max()/min()). Update the manpage to document the new flag. CC: Kees Cook <keescook@chromium.org> CC: Linus Torvalds <torvalds@linux-foundation.org> CC: Martin Uecker <Martin.Uecker@med.uni-goettingen.de> CC: Al Viro <viro@ZenIV.linux.org.uk> CC: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> CC: Christopher Li <sparse@chrisli.org> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com> 4 files changed, 24 insertions(+), 2 deletions(-)