| Message ID | 20171205153259.2522368-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 5, 2017 at 7:32 AM, Arnd Bergmann <arnd@arndb.de> wrote: > I have occasionally run into a situation where it would make sense to > control a compiler warning from a source file rather than doing so from > a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...) > helpers. > > The approach here is similar to what glibc uses, using __diag() and > related macros to encapsulate a _Pragma("GCC diagnostic ...") statement > that gets turned into the respective "#pragma GCC diagnostic ..." by > the preprocessor when the macro gets expanded. > > Like glibc, I also have an argument to pass the affected compiler > version, but decided to actually evaluate that one. For now, this > supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7, > GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting > versions is straightforward here. GNU compilers starting with gcc-4.2 > could support it in principle, but "#pragma GCC diagnostic push" > was only added in gcc-4.6, so it seems simpler to not deal with those > at all. The same versions show a large number of warnings already, > so it seems easier to just leave it at that and not do a more > fine-grained control for them. > > The use cases I found so far include: > > - turning off the gcc-8 -Wattribute-alias warning inside of the > SYSCALL_DEFINEx() macro without having to do it globally. > > - Reducing the build time for a simple re-make after a change, > once we move the warnings from ./Makefile and > ./scripts/Makefile.extrawarn into linux/compiler.h > > - More control over the warnings based on other configurations, > using preprocessor syntax instead of Makefile syntax. This should make > it easier for the average developer to understand and change things. > > - Adding an easy way to turn the W=1 option on unconditionally > for a subdirectory or a specific file. This has been requested > by several developers in the past that want to have their subsystems > W=1 clean. > > - Integrating clang better into the build systems. Clang supports > more warnings than GCC, and we probably want to classify them > as default, W=1, W=2 etc, but there are cases in which the > warnings should be classified differently due to excessive false > positives from one or the other compiler. > > - Adding a way to turn the default warnings into errors (e.g. using > a new "make E=0" tag) while not also turning the W=1 warnings into > errors. > > This patch for now just adds the minimal infrastructure in order to > do the first of the list above. As the #pragma GCC diagnostic > takes precedence over command line options, the next step would be > to convert a lot of the individual Makefiles that set nonstandard > options to use __diag() instead. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > I'm marking this RFC for now, as I'd like to get consensus about > whether we want to go down this particular route first, and maybe > if someone can come up with better naming for the macros. I like this. I wonder if it would be a good idea to add an additional argument that forces documentation of the reason for adding a diag marking? Something like: __diag_warn(GCC_7, vla, "No VLAs should be used in this code"); -Kees > --- > include/linux/compiler-gcc.h | 64 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/compiler_types.h | 10 +++++++ > 2 files changed, 74 insertions(+) > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index 2272ded07496..5d595cfdb2c4 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -329,3 +329,67 @@ > * code > */ > #define uninitialized_var(x) x = x > + > +/* > + * turn individual warnings and errors on and off locally, depending > + * on version. > + */ > +#if GCC_VERSION >= 40600 > +#define __diag_str1(s) #s > +#define __diag_str(s) __diag_str1(s) > +#define __diag(s) _Pragma(__diag_str(GCC diagnostic s)) > + > +/* compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */ > +#define __diag_GCC_4_6(s) __diag(s) > +#else > +#define __diag(s) > +#define __diag_GCC_4_6(s) > +#endif > + > +#if GCC_VERSION >= 40700 > +#define __diag_GCC_4_7(s) __diag(s) > +#else > +#define __diag_GCC_4_7(s) > +#endif > + > +#if GCC_VERSION >= 40800 > +#define __diag_GCC_4_8(s) __diag(s) > +#else > +#define __diag_GCC_4_8(s) > +#endif > + > +#if GCC_VERSION >= 40900 > +#define __diag_GCC_4_9(s) __diag(s) > +#else > +#define __diag_GCC_4_9(s) > +#endif > + > +#if GCC_VERSION >= 50000 > +#define __diag_GCC_5(s) __diag(s) > +#else > +#define __diag_GCC_5(s) > +#endif > + > +#if GCC_VERSION >= 60000 > +#define __diag_GCC_6(s) __diag(s) > +#else > +#define __diag_GCC_6(s) > +#endif > + > +#if GCC_VERSION >= 70000 > +#define __diag_GCC_7(s) __diag(s) > +#else > +#define __diag_GCC_7(s) > +#endif > + > +#if GCC_VERSION >= 80000 > +#define __diag_GCC_8(s) __diag(s) > +#else > +#define __diag_GCC_8(s) > +#endif > + > +#if GCC_VERSION >= 90000 > +#define __diag_GCC_9(s) __diag(s) > +#else > +#define __diag_GCC_9(s) > +#endif > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index 6b79a9bba9a7..7e7664d57adb 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -271,4 +271,14 @@ struct ftrace_likely_data { > # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long)) > #endif > > +#ifndef __diag > +#define __diag(string) > +#endif > + > +#define __diag_push() __diag(push) > +#define __diag_ignore(version, option) __diag_ ## version (ignored option) > +#define __diag_warn(version, option) __diag_ ## version (warning option) > +#define __diag_error(version, option) __diag_ ## version (error option) > +#define __diag_pop() __diag(pop) > + > #endif /* __LINUX_COMPILER_TYPES_H */ > -- > 2.9.0 >
On Tue, Dec 5, 2017 at 8:25 PM, Kees Cook <keescook@chromium.org> wrote: > I like this. I wonder if it would be a good idea to add an additional > argument that forces documentation of the reason for adding a diag > marking? Something like: > > __diag_warn(GCC_7, vla, "No VLAs should be used in this code"); This would be similar to what glibc does, it names the (ignore) macro DIAG_IGNORE_NEEDS_COMMENT(), and by convention requires a comment block in front of it. Not sure if it will actually work in the kernel where the reviews are much more scattered across subsystem maintainers, but we could try it. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, 2017-12-06 0:32 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: > I have occasionally run into a situation where it would make sense to > control a compiler warning from a source file rather than doing so from > a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...) > helpers. > > The approach here is similar to what glibc uses, using __diag() and > related macros to encapsulate a _Pragma("GCC diagnostic ...") statement > that gets turned into the respective "#pragma GCC diagnostic ..." by > the preprocessor when the macro gets expanded. > > Like glibc, I also have an argument to pass the affected compiler > version, but decided to actually evaluate that one. For now, this > supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7, > GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting > versions is straightforward here. GNU compilers starting with gcc-4.2 > could support it in principle, but "#pragma GCC diagnostic push" > was only added in gcc-4.6, so it seems simpler to not deal with those > at all. The same versions show a large number of warnings already, > so it seems easier to just leave it at that and not do a more > fine-grained control for them. > > The use cases I found so far include: > > - turning off the gcc-8 -Wattribute-alias warning inside of the > SYSCALL_DEFINEx() macro without having to do it globally. > > - Reducing the build time for a simple re-make after a change, > once we move the warnings from ./Makefile and > ./scripts/Makefile.extrawarn into linux/compiler.h Do you mean, list default warnings in linux/compiler.h like __diag_ignore(GCC_*, "-Wunused-but-set-variable"); __diag_ignore(GCC_*, "-Wunused-const-variable"); instead of KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable) KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable) Is this what you mean? I wonder if we should for all files to include <linux/compiler.h>... > - More control over the warnings based on other configurations, > using preprocessor syntax instead of Makefile syntax. This should make > it easier for the average developer to understand and change things. > > - Adding an easy way to turn the W=1 option on unconditionally > for a subdirectory or a specific file. This has been requested > by several developers in the past that want to have their subsystems > W=1 clean. > > - Integrating clang better into the build systems. Clang supports > more warnings than GCC, and we probably want to classify them > as default, W=1, W=2 etc, but there are cases in which the > warnings should be classified differently due to excessive false > positives from one or the other compiler. > > - Adding a way to turn the default warnings into errors (e.g. using > a new "make E=0" tag) while not also turning the W=1 warnings into > errors. > > This patch for now just adds the minimal infrastructure in order to > do the first of the list above. As the #pragma GCC diagnostic > takes precedence over command line options, the next step would be > to convert a lot of the individual Makefiles that set nonstandard > options to use __diag() instead. GCC manual says: Note that not all diagnostics are modifiable; at the moment only warnings (normally controlled by ‘-W…’) can be controlled, and not all of them. https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas Actually, my compiler does not react to __diag_*(GCC_*, "-Wunused-but-set-variable") at all. Is it possible to replace command line flags? > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > I'm marking this RFC for now, as I'd like to get consensus about > whether we want to go down this particular route first, and maybe > if someone can come up with better naming for the macros. > --- > include/linux/compiler-gcc.h | 64 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/compiler_types.h | 10 +++++++ > 2 files changed, 74 insertions(+) > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index 2272ded07496..5d595cfdb2c4 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -329,3 +329,67 @@ > * code > */ > #define uninitialized_var(x) x = x > + > +/* > + * turn individual warnings and errors on and off locally, depending > + * on version. > + */ > +#if GCC_VERSION >= 40600 > +#define __diag_str1(s) #s > +#define __diag_str(s) __diag_str1(s) > +#define __diag(s) _Pragma(__diag_str(GCC diagnostic s)) > + > +/* compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */ > +#define __diag_GCC_4_6(s) __diag(s) > +#else > +#define __diag(s) > +#define __diag_GCC_4_6(s) > +#endif > + > +#if GCC_VERSION >= 40700 > +#define __diag_GCC_4_7(s) __diag(s) > +#else > +#define __diag_GCC_4_7(s) > +#endif > + > +#if GCC_VERSION >= 40800 > +#define __diag_GCC_4_8(s) __diag(s) > +#else > +#define __diag_GCC_4_8(s) > +#endif > + > +#if GCC_VERSION >= 40900 > +#define __diag_GCC_4_9(s) __diag(s) > +#else > +#define __diag_GCC_4_9(s) > +#endif > + > +#if GCC_VERSION >= 50000 > +#define __diag_GCC_5(s) __diag(s) > +#else > +#define __diag_GCC_5(s) > +#endif > + > +#if GCC_VERSION >= 60000 > +#define __diag_GCC_6(s) __diag(s) > +#else > +#define __diag_GCC_6(s) > +#endif > + > +#if GCC_VERSION >= 70000 > +#define __diag_GCC_7(s) __diag(s) > +#else > +#define __diag_GCC_7(s) > +#endif > + > +#if GCC_VERSION >= 80000 > +#define __diag_GCC_8(s) __diag(s) > +#else > +#define __diag_GCC_8(s) > +#endif > + > +#if GCC_VERSION >= 90000 > +#define __diag_GCC_9(s) __diag(s) > +#else > +#define __diag_GCC_9(s) > +#endif For linux/compiler-intel.h case, these macros are not defined at all, so __diag_ignore(GCC_*, ...) will cause compile error. Clang includes linux/compiler-gcc.h as well, so it should be OK. Interestingly, clang also defines GCC version, but I have no idea the correspondence between gcc version vs. clang version. > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index 6b79a9bba9a7..7e7664d57adb 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -271,4 +271,14 @@ struct ftrace_likely_data { > # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long)) > #endif > > +#ifndef __diag > +#define __diag(string) > +#endif > + > +#define __diag_push() __diag(push) > +#define __diag_ignore(version, option) __diag_ ## version (ignored option) > +#define __diag_warn(version, option) __diag_ ## version (warning option) > +#define __diag_error(version, option) __diag_ ## version (error option) > +#define __diag_pop() __diag(pop) > + > #endif /* __LINUX_COMPILER_TYPES_H */ > -- > 2.9.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 18, 2017 at 10:48 AM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2017-12-06 0:32 GMT+09:00 Arnd Bergmann <arnd@arndb.de>: >> I have occasionally run into a situation where it would make sense to >> control a compiler warning from a source file rather than doing so from >> a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...) >> helpers. >> >> The approach here is similar to what glibc uses, using __diag() and >> related macros to encapsulate a _Pragma("GCC diagnostic ...") statement >> that gets turned into the respective "#pragma GCC diagnostic ..." by >> the preprocessor when the macro gets expanded. >> >> Like glibc, I also have an argument to pass the affected compiler >> version, but decided to actually evaluate that one. For now, this >> supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7, >> GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting >> versions is straightforward here. GNU compilers starting with gcc-4.2 >> could support it in principle, but "#pragma GCC diagnostic push" >> was only added in gcc-4.6, so it seems simpler to not deal with those >> at all. The same versions show a large number of warnings already, >> so it seems easier to just leave it at that and not do a more >> fine-grained control for them. >> >> The use cases I found so far include: >> >> - turning off the gcc-8 -Wattribute-alias warning inside of the >> SYSCALL_DEFINEx() macro without having to do it globally. >> >> - Reducing the build time for a simple re-make after a change, >> once we move the warnings from ./Makefile and >> ./scripts/Makefile.extrawarn into linux/compiler.h > > Do you mean, list default warnings in linux/compiler.h > > like > > __diag_ignore(GCC_*, "-Wunused-but-set-variable"); > __diag_ignore(GCC_*, "-Wunused-const-variable"); > > instead of > > KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable) > KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable) > > Is this what you mean? In principle yes, though my plan was a bit more sophisticated than this: I want to make those definitions specific to both the W= level and the compiler version, so you can say things like KBUILD_WARN(0, GCC_4_6, "-Wall") KBUILD_WARN(0, CLANG5, "-Wall") KBUILD_WARN(1, GCC_4_6, "-Wextra") KBUILD_WARN(1, CLANG5, "-Wextra") KBUILD_WARN(2, CLANG5, "-Wmost") KBUILD_WARN(3, CLANG5, "-Weverything") With the above, "make W=12 E=01" should would lead to -Wall and -Wextra warnings to become errors on all compilers, but also enable -Wmost as warnings rather than errors on clang. > I wonder if we should for all files to include <linux/compiler.h>... I assumed we already did that, but maybe I was wrong here. I see that we include include/linux/kconfig.h everywhere, but I don't see that for compiler.h. This would obviously be a requirement to do first. >> This patch for now just adds the minimal infrastructure in order to >> do the first of the list above. As the #pragma GCC diagnostic >> takes precedence over command line options, the next step would be >> to convert a lot of the individual Makefiles that set nonstandard >> options to use __diag() instead. > > > GCC manual says: > Note that not all diagnostics are modifiable; at the moment only warnings > (normally controlled by ‘-W…’) can be controlled, and not all of them. > > https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas > > > Actually, my compiler does not react > to __diag_*(GCC_*, "-Wunused-but-set-variable") at all. > > Is it possible to replace command line flags? My interpretation was that everything that had a -W option could be controlled with a pragma as well. While we can turn any warning into an error, we cannot turn most errors into warnings. When I experimented with this a couple of months ago, I also saw that "gcc -Q --help=warnings" reports some warnings that are language specific, and it refuses to enable e.g. a fortran specific warning when building a C file. > For linux/compiler-intel.h case, > these macros are not defined at all, > so __diag_ignore(GCC_*, ...) will cause compile error. Right, that would need to be fixed. I think it's ok to just ignore all those statements and just continue with the default warnings. > Clang includes linux/compiler-gcc.h as well, > so it should be OK. > > Interestingly, clang also defines GCC version, > but I have no idea the correspondence between > gcc version vs. clang version. I tried clang-3.9 and clang-5, both of which report compatibility with gcc-4.2.1. It seems that for the most part, clang tries to implement the gcc warning options, but there are always some it does not have. I can probably come up with a list of warning options that all supported versions of either gcc or clang understand, and have those set independent of the version, but for all others require listing the specific version that introduces it. In some cases a warning gets introduced in one version but isn't actually usable until a later version. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" 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/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 2272ded07496..5d595cfdb2c4 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -329,3 +329,67 @@ * code */ #define uninitialized_var(x) x = x + +/* + * turn individual warnings and errors on and off locally, depending + * on version. + */ +#if GCC_VERSION >= 40600 +#define __diag_str1(s) #s +#define __diag_str(s) __diag_str1(s) +#define __diag(s) _Pragma(__diag_str(GCC diagnostic s)) + +/* compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */ +#define __diag_GCC_4_6(s) __diag(s) +#else +#define __diag(s) +#define __diag_GCC_4_6(s) +#endif + +#if GCC_VERSION >= 40700 +#define __diag_GCC_4_7(s) __diag(s) +#else +#define __diag_GCC_4_7(s) +#endif + +#if GCC_VERSION >= 40800 +#define __diag_GCC_4_8(s) __diag(s) +#else +#define __diag_GCC_4_8(s) +#endif + +#if GCC_VERSION >= 40900 +#define __diag_GCC_4_9(s) __diag(s) +#else +#define __diag_GCC_4_9(s) +#endif + +#if GCC_VERSION >= 50000 +#define __diag_GCC_5(s) __diag(s) +#else +#define __diag_GCC_5(s) +#endif + +#if GCC_VERSION >= 60000 +#define __diag_GCC_6(s) __diag(s) +#else +#define __diag_GCC_6(s) +#endif + +#if GCC_VERSION >= 70000 +#define __diag_GCC_7(s) __diag(s) +#else +#define __diag_GCC_7(s) +#endif + +#if GCC_VERSION >= 80000 +#define __diag_GCC_8(s) __diag(s) +#else +#define __diag_GCC_8(s) +#endif + +#if GCC_VERSION >= 90000 +#define __diag_GCC_9(s) __diag(s) +#else +#define __diag_GCC_9(s) +#endif diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 6b79a9bba9a7..7e7664d57adb 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -271,4 +271,14 @@ struct ftrace_likely_data { # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long)) #endif +#ifndef __diag +#define __diag(string) +#endif + +#define __diag_push() __diag(push) +#define __diag_ignore(version, option) __diag_ ## version (ignored option) +#define __diag_warn(version, option) __diag_ ## version (warning option) +#define __diag_error(version, option) __diag_ ## version (error option) +#define __diag_pop() __diag(pop) + #endif /* __LINUX_COMPILER_TYPES_H */
I have occasionally run into a situation where it would make sense to control a compiler warning from a source file rather than doing so from a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...) helpers. The approach here is similar to what glibc uses, using __diag() and related macros to encapsulate a _Pragma("GCC diagnostic ...") statement that gets turned into the respective "#pragma GCC diagnostic ..." by the preprocessor when the macro gets expanded. Like glibc, I also have an argument to pass the affected compiler version, but decided to actually evaluate that one. For now, this supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7, GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting versions is straightforward here. GNU compilers starting with gcc-4.2 could support it in principle, but "#pragma GCC diagnostic push" was only added in gcc-4.6, so it seems simpler to not deal with those at all. The same versions show a large number of warnings already, so it seems easier to just leave it at that and not do a more fine-grained control for them. The use cases I found so far include: - turning off the gcc-8 -Wattribute-alias warning inside of the SYSCALL_DEFINEx() macro without having to do it globally. - Reducing the build time for a simple re-make after a change, once we move the warnings from ./Makefile and ./scripts/Makefile.extrawarn into linux/compiler.h - More control over the warnings based on other configurations, using preprocessor syntax instead of Makefile syntax. This should make it easier for the average developer to understand and change things. - Adding an easy way to turn the W=1 option on unconditionally for a subdirectory or a specific file. This has been requested by several developers in the past that want to have their subsystems W=1 clean. - Integrating clang better into the build systems. Clang supports more warnings than GCC, and we probably want to classify them as default, W=1, W=2 etc, but there are cases in which the warnings should be classified differently due to excessive false positives from one or the other compiler. - Adding a way to turn the default warnings into errors (e.g. using a new "make E=0" tag) while not also turning the W=1 warnings into errors. This patch for now just adds the minimal infrastructure in order to do the first of the list above. As the #pragma GCC diagnostic takes precedence over command line options, the next step would be to convert a lot of the individual Makefiles that set nonstandard options to use __diag() instead. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- I'm marking this RFC for now, as I'd like to get consensus about whether we want to go down this particular route first, and maybe if someone can come up with better naming for the macros. --- include/linux/compiler-gcc.h | 64 ++++++++++++++++++++++++++++++++++++++++++ include/linux/compiler_types.h | 10 +++++++ 2 files changed, 74 insertions(+)