| Message ID | 20180616005323.7938-2-paul.burton@mips.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] kbuild: add macro for controlling warnings to linux/compiler.h | expand |
Le 16/06/2018 à 02:53, Paul Burton a écrit : > From: 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 > > - 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. > > [paul.burton@mips.com: > - Rebase atop current master. > - Add __diag_GCC, or more generally __diag_<compiler>, abstraction to > avoid code outside of linux/compiler-gcc.h needing to duplicate > knowledge about different GCC versions. > - Add a comment argument to __diag_{ignore,warn,error} which isn't > used in the expansion of the macros but serves to push people to > document the reason for using them - per feedback from Kees Cook.] > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Paul Burton <paul.burton@mips.com> > Cc: Michal Marek <michal.lkml@markovi.net> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Douglas Anderson <dianders@chromium.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Matthew Wilcox <matthew@wil.cx> > Cc: Matthias Kaehlcke <mka@chromium.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Josh Poimboeuf <jpoimboe@redhat.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Gideon Israel Dsouza <gidisrael@gmail.com> > Cc: Christophe Leroy <christophe.leroy@c-s.fr> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Stafford Horne <shorne@gmail.com> > Cc: Khem Raj <raj.khem@gmail.com> > Cc: He Zhe <zhe.he@windriver.com> > Cc: linux-kbuild@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-mips@linux-mips.org > Cc: linuxppc-dev@lists.ozlabs.org Tested-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > > include/linux/compiler-gcc.h | 66 ++++++++++++++++++++++++++++++++++ > include/linux/compiler_types.h | 18 ++++++++++ > 2 files changed, 84 insertions(+) > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index f1a7492a5cc8..aba64a2912d8 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -347,3 +347,69 @@ > #if GCC_VERSION >= 50100 > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > #endif > + > +/* > + * turn individual warnings and errors on and off locally, depending > + * on version. > + */ > +#define __diag_GCC(version, s) __diag_GCC_ ## version(s) > + > +#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..313a2ad884e0 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -271,4 +271,22 @@ 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 > + > +#ifndef __diag_GCC > +#define __diag_GCC(string) > +#endif > + > +#define __diag_push() __diag(push) > +#define __diag_pop() __diag(pop) > + > +#define __diag_ignore(compiler, version, option, comment) \ > + __diag_ ## compiler(version, ignored option) > +#define __diag_warn(compiler, version, option, comment) \ > + __diag_ ## compiler(version, warning option) > +#define __diag_error(compiler, version, option, comment) \ > + __diag_ ## compiler(version, error option) > + > #endif /* __LINUX_COMPILER_TYPES_H */ >
Hi. 2018-06-16 9:53 GMT+09:00 Paul Burton <paul.burton@mips.com>: > From: 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 > > - 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. > > [paul.burton@mips.com: > - Rebase atop current master. > - Add __diag_GCC, or more generally __diag_<compiler>, abstraction to > avoid code outside of linux/compiler-gcc.h needing to duplicate > knowledge about different GCC versions. > - Add a comment argument to __diag_{ignore,warn,error} which isn't > used in the expansion of the macros but serves to push people to > document the reason for using them - per feedback from Kees Cook.] > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Paul Burton <paul.burton@mips.com> > Cc: Michal Marek <michal.lkml@markovi.net> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Douglas Anderson <dianders@chromium.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Matthew Wilcox <matthew@wil.cx> > Cc: Matthias Kaehlcke <mka@chromium.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Josh Poimboeuf <jpoimboe@redhat.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Gideon Israel Dsouza <gidisrael@gmail.com> > Cc: Christophe Leroy <christophe.leroy@c-s.fr> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Stafford Horne <shorne@gmail.com> > Cc: Khem Raj <raj.khem@gmail.com> > Cc: He Zhe <zhe.he@windriver.com> > Cc: linux-kbuild@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-mips@linux-mips.org > Cc: linuxppc-dev@lists.ozlabs.org > --- > > include/linux/compiler-gcc.h | 66 ++++++++++++++++++++++++++++++++++ > include/linux/compiler_types.h | 18 ++++++++++ > 2 files changed, 84 insertions(+) > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index f1a7492a5cc8..aba64a2912d8 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -347,3 +347,69 @@ > #if GCC_VERSION >= 50100 > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > #endif > + > +/* > + * turn individual warnings and errors on and off locally, depending > + * on version. > + */ > +#define __diag_GCC(version, s) __diag_GCC_ ## version(s) > + > +#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 Hmm, we would have to add this for every release. > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index 6b79a9bba9a7..313a2ad884e0 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -271,4 +271,22 @@ 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 > + > +#ifndef __diag_GCC > +#define __diag_GCC(string) > +#endif __diag_GCC() takes two arguments, so it should be: #ifndef __diag_GCC #define __diag_GCC(version, s) #endif Otherwise, this would cause warning like this: arch/arm64/kernel/sys.c:40:1: error: macro "__diag_GCC" passed 2 arguments, but takes just 1 SYSCALL_DEFINE1(arm64_personality, unsigned int, personality) ^~~~~~~~~~ > +#define __diag_push() __diag(push) > +#define __diag_pop() __diag(pop) > + > +#define __diag_ignore(compiler, version, option, comment) \ > + __diag_ ## compiler(version, ignored option) > +#define __diag_warn(compiler, version, option, comment) \ > + __diag_ ## compiler(version, warning option) > +#define __diag_error(compiler, version, option, comment) \ > + __diag_ ## compiler(version, error option) > + To me, it looks like this is putting GCC/Clang specific things in the common file, <linux/compiler_types.h> . All compilers must use "ignored", "warning", "error", not allowed to use "ignore". I also wonder if we could avoid proliferating __diag_GCC_*. > #endif /* __LINUX_COMPILER_TYPES_H */ > -- > 2.17.1 > I attached a bit different implementation below. I used -Wno-pragmas to avoid unknown option warnings. diff --git a/Makefile b/Makefile index ca2af1a..d610d81 100644 --- a/Makefile +++ b/Makefile @@ -817,6 +817,8 @@ KBUILD_CFLAGS += $(call cc-option,-Werror=designated-init) # change __FILE__ to the relative path from the srctree KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=) +KBUILD_CFLAGS += $(call cc-option,-Wno-pragmas) + # use the deterministic mode of AR if available KBUILD_ARFLAGS := $(call ar-option,D) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index f1a7492..3f9c1cc 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -3,6 +3,8 @@ #error "Please don't include <linux/compiler-gcc.h> directly, include <linux/compiler.h> instead." #endif +#include <linux/stringify.h> + /* * Common definitions for all gcc versions go here. */ @@ -259,6 +261,16 @@ */ #define __visible __attribute__((externally_visible)) +/* turn individual warnings and errors on and off locally */ +#define __diag_gcc(s) _Pragma(__stringify(GCC diagnostic s)) + +#define __diag_push() __diag_gcc(push) +#define __diag_pop() __diag_gcc(pop) + +#define __diag_ignore(option, comment) __diag_gcc(ignored __stringify(option)) +#define __diag_warn(option, comment) __diag_gcc(warning __stringify(option)) +#define __diag_error(option, comment) __diag_gcc(error __stringify(option)) + #endif /* GCC_VERSION >= 40600 */ diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index 6b79a9b..32e354f 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -271,4 +271,24 @@ 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_push +#define __diag_push() +#endif + +#ifndef __diag_pop +#define __diag_pop() +#endif + +#ifndef __diag_ignore +#define __diag_ignore(option, comment) +#endif + +#ifndef __diag_warn +#define __diag_warn(option, comment) +#endif + +#ifndef __diag_error +#define __diag_error(option, comment) +#endif + #endif /* __LINUX_COMPILER_TYPES_H */ Usage is __diag_push(); __diag_ignore(-Wattribute-alias, "Type aliasing is used to sanitize syscall arguments"); ... __diag_pop(); Comments, ideas are appreciated. -- Best Regards Masahiro Yamada
Hi Masahiro, On Wed, Jun 20, 2018 at 02:34:35AM +0900, Masahiro Yamada wrote: > 2018-06-16 9:53 GMT+09:00 Paul Burton <paul.burton@mips.com>: > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > > index f1a7492a5cc8..aba64a2912d8 100644 > > --- a/include/linux/compiler-gcc.h > > +++ b/include/linux/compiler-gcc.h > > @@ -347,3 +347,69 @@ > > #if GCC_VERSION >= 50100 > > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 > > #endif > > + > > +/* > > + * turn individual warnings and errors on and off locally, depending > > + * on version. > > + */ > > +#define __diag_GCC(version, s) __diag_GCC_ ## version(s) > > + > > +#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 > > > Hmm, we would have to add this for every release. Well, strictly speaking only ones that we need to modify diags for - ie. in this series we could get away with only adding the GCC 8 macro if we wanted. > > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > > index 6b79a9bba9a7..313a2ad884e0 100644 > > --- a/include/linux/compiler_types.h > > +++ b/include/linux/compiler_types.h > > @@ -271,4 +271,22 @@ 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 > > + > > +#ifndef __diag_GCC > > +#define __diag_GCC(string) > > +#endif > > __diag_GCC() takes two arguments, > so it should be: > > #ifndef __diag_GCC > #define __diag_GCC(version, s) > #endif > > > Otherwise, this would cause warning like this: > > > arch/arm64/kernel/sys.c:40:1: error: macro "__diag_GCC" passed 2 > arguments, but takes just 1 > SYSCALL_DEFINE1(arm64_personality, unsigned int, personality) > ^~~~~~~~~~ Yes, good catch. > > +#define __diag_push() __diag(push) > > +#define __diag_pop() __diag(pop) > > + > > +#define __diag_ignore(compiler, version, option, comment) \ > > + __diag_ ## compiler(version, ignored option) > > +#define __diag_warn(compiler, version, option, comment) \ > > + __diag_ ## compiler(version, warning option) > > +#define __diag_error(compiler, version, option, comment) \ > > + __diag_ ## compiler(version, error option) > > + > > To me, it looks like this is putting GCC/Clang specific things > in the common file, <linux/compiler_types.h> . > > All compilers must use "ignored", "warning", "error", > not allowed to use "ignore". We could move that to linux/compiler-gcc.h pretty easily. > I also wonder if we could avoid proliferating __diag_GCC_*. My thought is that it's unlikely we'll ever support enough different compilers for it to become problematic to list the ones we modify warnings for in linux/compiler_types.h. > I attached a bit different implementation below. > > I used -Wno-pragmas to avoid unknown option warnings. That doesn't seem very clean to me because it will hide typos or other mistakes. One advantage of Arnd's patch is that by specifying the compiler & version we only attempt to use pragmas that are appropriate so we don't need to ignore unknown ones. > Usage is > > __diag_push(); > __diag_ignore(-Wattribute-alias, > "Type aliasing is used to sanitize syscall arguments"); > ... > __diag_pop(); > > Comments, ideas are appreciated. By removing the compiler & version arguments you're enforcing that if we ignore a warning for one compiler we must ignore it for all, regardless of whether it's problematic. This essentially presumes that warnings with the same name will behave the same across compilers, which feels worse to me than having users of this explicitly state which compilers need the pragmas. Thanks, Paul
Le 19/06/2018 à 22:14, Paul Burton a écrit : > With SYSCALL_DEFINEx() disabling -Wattribute-alias generically, there's > no need to duplicate that for PowerPC syscalls. > > This reverts commit 415520373975 ("powerpc: fix build failure by > disabling attribute-alias warning in pci_32") and commit 2479bfc9bc60 > ("powerpc: Fix build by disabling attribute-alias warning for > SYSCALL_DEFINEx"). > > Signed-off-by: Paul Burton <paul.burton@mips.com> > Cc: Michal Marek <michal.lkml@markovi.net> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com> > Cc: Douglas Anderson <dianders@chromium.org> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Matthew Wilcox <matthew@wil.cx> > Cc: Matthias Kaehlcke <mka@chromium.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Josh Poimboeuf <jpoimboe@redhat.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Gideon Israel Dsouza <gidisrael@gmail.com> > Cc: Christophe Leroy <christophe.leroy@c-s.fr> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Cc: Paul Mackerras <paulus@samba.org> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Stafford Horne <shorne@gmail.com> > Cc: Khem Raj <raj.khem@gmail.com> > Cc: He Zhe <zhe.he@windriver.com> > Cc: linux-kbuild@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-mips@linux-mips.org > Cc: linuxppc-dev@lists.ozlabs.org > > --- > Michael & Christophe, I didn't add your acks here yet since it changed > to include the second revert that Christophe pointed out I'd missed & I > didn't want to presume your acks extended to that. Looks ok to me Acked-by: Christophe Leroy <christophe.leroy@c-s.fr> > > Changes in v2: > - Also revert 2479bfc9bc60 ("powerpc: Fix build by disabling > attribute-alias warning for SYSCALL_DEFINEx"). > - Change subject now that it's not just a simple one-commit revert. > > arch/powerpc/kernel/pci_32.c | 4 ---- > arch/powerpc/kernel/pci_64.c | 4 ---- > arch/powerpc/kernel/rtas.c | 4 ---- > arch/powerpc/kernel/signal_32.c | 8 -------- > arch/powerpc/kernel/signal_64.c | 4 ---- > arch/powerpc/kernel/syscalls.c | 4 ---- > arch/powerpc/mm/subpage-prot.c | 4 ---- > 7 files changed, 32 deletions(-) > > diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c > index 4f861055a852..d63b488d34d7 100644 > --- a/arch/powerpc/kernel/pci_32.c > +++ b/arch/powerpc/kernel/pci_32.c > @@ -285,9 +285,6 @@ pci_bus_to_hose(int bus) > * Note that the returned IO or memory base is a physical address > */ > > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wpragmas" > -#pragma GCC diagnostic ignored "-Wattribute-alias" > SYSCALL_DEFINE3(pciconfig_iobase, long, which, > unsigned long, bus, unsigned long, devfn) > { > @@ -313,4 +310,3 @@ SYSCALL_DEFINE3(pciconfig_iobase, long, which, > > return result; > } > -#pragma GCC diagnostic pop > diff --git a/arch/powerpc/kernel/pci_64.c b/arch/powerpc/kernel/pci_64.c > index 812171c09f42..dff28f903512 100644 > --- a/arch/powerpc/kernel/pci_64.c > +++ b/arch/powerpc/kernel/pci_64.c > @@ -203,9 +203,6 @@ void pcibios_setup_phb_io_space(struct pci_controller *hose) > #define IOBASE_ISA_IO 3 > #define IOBASE_ISA_MEM 4 > > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wpragmas" > -#pragma GCC diagnostic ignored "-Wattribute-alias" > SYSCALL_DEFINE3(pciconfig_iobase, long, which, unsigned long, in_bus, > unsigned long, in_devfn) > { > @@ -259,7 +256,6 @@ SYSCALL_DEFINE3(pciconfig_iobase, long, which, unsigned long, in_bus, > > return -EOPNOTSUPP; > } > -#pragma GCC diagnostic pop > > #ifdef CONFIG_NUMA > int pcibus_to_node(struct pci_bus *bus) > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index 7fb9f83dcde8..8afd146bc9c7 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -1051,9 +1051,6 @@ struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log, > } > > /* We assume to be passed big endian arguments */ > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wpragmas" > -#pragma GCC diagnostic ignored "-Wattribute-alias" > SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > { > struct rtas_args args; > @@ -1140,7 +1137,6 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs) > > return 0; > } > -#pragma GCC diagnostic pop > > /* > * Call early during boot, before mem init, to retrieve the RTAS > diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c > index 5eedbb282d42..e6474a45cef5 100644 > --- a/arch/powerpc/kernel/signal_32.c > +++ b/arch/powerpc/kernel/signal_32.c > @@ -1038,9 +1038,6 @@ static int do_setcontext_tm(struct ucontext __user *ucp, > } > #endif > > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wpragmas" > -#pragma GCC diagnostic ignored "-Wattribute-alias" > #ifdef CONFIG_PPC64 > COMPAT_SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx, > struct ucontext __user *, new_ctx, int, ctx_size) > @@ -1134,7 +1131,6 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx, > set_thread_flag(TIF_RESTOREALL); > return 0; > } > -#pragma GCC diagnostic pop > > #ifdef CONFIG_PPC64 > COMPAT_SYSCALL_DEFINE0(rt_sigreturn) > @@ -1231,9 +1227,6 @@ SYSCALL_DEFINE0(rt_sigreturn) > return 0; > } > > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wpragmas" > -#pragma GCC diagnostic ignored "-Wattribute-alias" > #ifdef CONFIG_PPC32 > SYSCALL_DEFINE3(debug_setcontext, struct ucontext __user *, ctx, > int, ndbg, struct sig_dbg_op __user *, dbg) > @@ -1337,7 +1330,6 @@ SYSCALL_DEFINE3(debug_setcontext, struct ucontext __user *, ctx, > return 0; > } > #endif > -#pragma GCC diagnostic pop > > /* > * OK, we're invoking a handler > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > index d42b60020389..83d51bf586c7 100644 > --- a/arch/powerpc/kernel/signal_64.c > +++ b/arch/powerpc/kernel/signal_64.c > @@ -625,9 +625,6 @@ static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp) > /* > * Handle {get,set,swap}_context operations > */ > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wpragmas" > -#pragma GCC diagnostic ignored "-Wattribute-alias" > SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx, > struct ucontext __user *, new_ctx, long, ctx_size) > { > @@ -693,7 +690,6 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx, > set_thread_flag(TIF_RESTOREALL); > return 0; > } > -#pragma GCC diagnostic pop > > > /* > diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c > index 083fa06962fd..466216506eb2 100644 > --- a/arch/powerpc/kernel/syscalls.c > +++ b/arch/powerpc/kernel/syscalls.c > @@ -62,9 +62,6 @@ static inline long do_mmap2(unsigned long addr, size_t len, > return ret; > } > > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wpragmas" > -#pragma GCC diagnostic ignored "-Wattribute-alias" > SYSCALL_DEFINE6(mmap2, unsigned long, addr, size_t, len, > unsigned long, prot, unsigned long, flags, > unsigned long, fd, unsigned long, pgoff) > @@ -78,7 +75,6 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len, > { > return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT); > } > -#pragma GCC diagnostic pop > > #ifdef CONFIG_PPC32 > /* > diff --git a/arch/powerpc/mm/subpage-prot.c b/arch/powerpc/mm/subpage-prot.c > index 75cb646a79c3..9d16ee251fc0 100644 > --- a/arch/powerpc/mm/subpage-prot.c > +++ b/arch/powerpc/mm/subpage-prot.c > @@ -186,9 +186,6 @@ static void subpage_mark_vma_nohuge(struct mm_struct *mm, unsigned long addr, > * in a 2-bit field won't allow writes to a page that is otherwise > * write-protected. > */ > -#pragma GCC diagnostic push > -#pragma GCC diagnostic ignored "-Wpragmas" > -#pragma GCC diagnostic ignored "-Wattribute-alias" > SYSCALL_DEFINE3(subpage_prot, unsigned long, addr, > unsigned long, len, u32 __user *, map) > { > @@ -272,4 +269,3 @@ SYSCALL_DEFINE3(subpage_prot, unsigned long, addr, > up_write(&mm->mmap_sem); > return err; > } > -#pragma GCC diagnostic pop >
2018-06-20 4:02 GMT+09:00 Paul Burton <paul.burton@mips.com>: > Hi Masahiro, > > On Wed, Jun 20, 2018 at 02:34:35AM +0900, Masahiro Yamada wrote: >> 2018-06-16 9:53 GMT+09:00 Paul Burton <paul.burton@mips.com>: >> > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h >> > index f1a7492a5cc8..aba64a2912d8 100644 >> > --- a/include/linux/compiler-gcc.h >> > +++ b/include/linux/compiler-gcc.h >> > @@ -347,3 +347,69 @@ >> > #if GCC_VERSION >= 50100 >> > #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 >> > #endif >> > + >> > +/* >> > + * turn individual warnings and errors on and off locally, depending >> > + * on version. >> > + */ >> > +#define __diag_GCC(version, s) __diag_GCC_ ## version(s) >> > + >> > +#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 >> >> >> Hmm, we would have to add this for every release. > > Well, strictly speaking only ones that we need to modify diags for - ie. > in this series we could get away with only adding the GCC 8 macro if we > wanted. > >> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h >> > index 6b79a9bba9a7..313a2ad884e0 100644 >> > --- a/include/linux/compiler_types.h >> > +++ b/include/linux/compiler_types.h >> > @@ -271,4 +271,22 @@ 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 >> > + >> > +#ifndef __diag_GCC >> > +#define __diag_GCC(string) >> > +#endif >> >> __diag_GCC() takes two arguments, >> so it should be: >> >> #ifndef __diag_GCC >> #define __diag_GCC(version, s) >> #endif >> >> >> Otherwise, this would cause warning like this: >> >> >> arch/arm64/kernel/sys.c:40:1: error: macro "__diag_GCC" passed 2 >> arguments, but takes just 1 >> SYSCALL_DEFINE1(arm64_personality, unsigned int, personality) >> ^~~~~~~~~~ > > Yes, good catch. > >> > +#define __diag_push() __diag(push) >> > +#define __diag_pop() __diag(pop) >> > + >> > +#define __diag_ignore(compiler, version, option, comment) \ >> > + __diag_ ## compiler(version, ignored option) >> > +#define __diag_warn(compiler, version, option, comment) \ >> > + __diag_ ## compiler(version, warning option) >> > +#define __diag_error(compiler, version, option, comment) \ >> > + __diag_ ## compiler(version, error option) >> > + >> >> To me, it looks like this is putting GCC/Clang specific things >> in the common file, <linux/compiler_types.h> . >> >> All compilers must use "ignored", "warning", "error", >> not allowed to use "ignore". > > We could move that to linux/compiler-gcc.h pretty easily. > >> I also wonder if we could avoid proliferating __diag_GCC_*. > > My thought is that it's unlikely we'll ever support enough different > compilers for it to become problematic to list the ones we modify > warnings for in linux/compiler_types.h. > >> I attached a bit different implementation below. >> >> I used -Wno-pragmas to avoid unknown option warnings. > > That doesn't seem very clean to me because it will hide typos or other > mistakes. One advantage of Arnd's patch is that by specifying the > compiler & version we only attempt to use pragmas that are appropriate > so we don't need to ignore unknown ones. > >> Usage is >> >> __diag_push(); >> __diag_ignore(-Wattribute-alias, >> "Type aliasing is used to sanitize syscall arguments"); >> ... >> __diag_pop(); >> >> Comments, ideas are appreciated. > > By removing the compiler & version arguments you're enforcing that if we > ignore a warning for one compiler we must ignore it for all, regardless > of whether it's problematic. This essentially presumes that warnings > with the same name will behave the same across compilers, which feels > worse to me than having users of this explicitly state which compilers > need the pragmas. Fair enough. V2 is good except one nit. (I left a comment in it) I can fix it up locally if it is tedious to re-spin, though. > Thanks, > Paul > -- > 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 -- Best Regards Masahiro Yamada
Hi Masahiro, On Thu, Jun 21, 2018 at 08:21:01AM +0900, Masahiro Yamada wrote: > V2 is good except one nit. > (I left a comment in it) Thanks, and yes I agree with your comment that the GCC<4.6 __diag() definition can be removed. > I can fix it up locally if it is tedious to re-spin, though. If you wouldn't mind that'd be great :) Thanks, Paul
2018-06-20 5:14 GMT+09:00 Paul Burton <paul.burton@mips.com>: > This series introduces infrastructure allowing compiler diagnostics to > be disabled or their severity modified for specific pieces of code, with > suitable abstractions to prevent that code from becoming tied to a > specific compiler. > > This infrastructure is then used to disable the -Wattribute-alias > warning around syscall definitions, which rely on type mismatches to > sanitize arguments. > > Finally PowerPC-specific #pragma's are removed now that the generic code > is handling this. > > The series takes Arnd's RFC patches & addresses the review comments they > received. The most notable effect of this series to to avoid warnings & > build failures caused by -Wattribute-alias when compiling the kernel > with GCC 8. > > Applies cleanly atop v4.18-rc1. Series, applied to linux-kbuild/fixes. (since we need to fix warnings from GCC 8.1) Thanks! -- Best Regards Masahiro Yamada
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index f1a7492a5cc8..aba64a2912d8 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -347,3 +347,69 @@ #if GCC_VERSION >= 50100 #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 #endif + +/* + * turn individual warnings and errors on and off locally, depending + * on version. + */ +#define __diag_GCC(version, s) __diag_GCC_ ## version(s) + +#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..313a2ad884e0 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -271,4 +271,22 @@ 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 + +#ifndef __diag_GCC +#define __diag_GCC(string) +#endif + +#define __diag_push() __diag(push) +#define __diag_pop() __diag(pop) + +#define __diag_ignore(compiler, version, option, comment) \ + __diag_ ## compiler(version, ignored option) +#define __diag_warn(compiler, version, option, comment) \ + __diag_ ## compiler(version, warning option) +#define __diag_error(compiler, version, option, comment) \ + __diag_ ## compiler(version, error option) + #endif /* __LINUX_COMPILER_TYPES_H */