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 master as of 9215310cf13b ("Merge
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net").
Thanks,
Paul
Arnd Bergmann (2):
kbuild: add macro for controlling warnings to linux/compiler.h
disable -Wattribute-alias warning for SYSCALL_DEFINEx()
Paul Burton (1):
Revert "powerpc: fix build failure by disabling attribute-alias
warning in pci_32"
arch/powerpc/kernel/pci_32.c | 4 ---
include/linux/compat.h | 8 ++++-
include/linux/compiler-gcc.h | 66 ++++++++++++++++++++++++++++++++++
include/linux/compiler_types.h | 18 ++++++++++
include/linux/syscalls.h | 4 +++
5 files changed, 95 insertions(+), 5 deletions(-)
--
2.17.1
From: Arnd Bergmann <[email protected]>
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.
[[email protected]:
- 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 <[email protected]>
Signed-off-by: Paul Burton <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Gideon Israel Dsouza <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Stafford Horne <[email protected]>
Cc: Khem Raj <[email protected]>
Cc: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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 */
--
2.17.1
With SYSCALL_DEFINEx() disabling -Wattribute-alias generically, there's
no need to duplicate that for PowerPC's pciconfig_iobase syscall.
This reverts commit 415520373975 ("powerpc: fix build failure by
disabling attribute-alias warning in pci_32").
Signed-off-by: Paul Burton <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Gideon Israel Dsouza <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Stafford Horne <[email protected]>
Cc: Khem Raj <[email protected]>
Cc: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
arch/powerpc/kernel/pci_32.c | 4 ----
1 file changed, 4 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
--
2.17.1
From: Arnd Bergmann <[email protected]>
gcc-8 warns for every single definition of a system call entry
point, e.g.:
include/linux/compat.h:56:18: error: 'compat_sys_rt_sigprocmask' alias between functions of incompatible types 'long int(int, compat_sigset_t *, compat_sigset_t *, compat_size_t)' {aka 'long int(int, struct <anonymous> *, struct <anonymous> *, unsigned int)'} and 'long int(long int, long int, long int, long int)' [-Werror=attribute-alias]
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))\
^~~~~~~~~~
include/linux/compat.h:45:2: note: in expansion of macro 'COMPAT_SYSCALL_DEFINEx'
COMPAT_SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~~~~~~~~
kernel/signal.c:2601:1: note: in expansion of macro 'COMPAT_SYSCALL_DEFINE4'
COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, int, how, compat_sigset_t __user *, nset,
^~~~~~~~~~~~~~~~~~~~~~
include/linux/compat.h:60:18: note: aliased declaration here
asmlinkage long compat_SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))\
^~~~~~~~~~
The new warning seems reasonable in principle, but it doesn't
help us here, since we rely on the type mismatch to sanitize the
system call arguments. After I reported this as GCC PR82435, a new
-Wno-attribute-alias option was added that could be used to turn the
warning off globally on the command line, but I'd prefer to do it a
little more fine-grained.
Interestingly, turning a warning off and on again inside of
a single macro doesn't always work, in this case I had to add
an extra statement inbetween and decided to copy the __SC_TEST
one from the native syscall to the compat syscall macro. See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83256 for more details
about this.
[[email protected]:
- Rebase atop current master.
- Split GCC & version arguments to __diag_ignore() in order to match
changes to the preceding patch.
- Add the comment argument to match the preceding patch.]
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82435
Signed-off-by: Arnd Bergmann <[email protected]>
Signed-off-by: Paul Burton <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Gideon Israel Dsouza <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Stafford Horne <[email protected]>
Cc: Khem Raj <[email protected]>
Cc: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
include/linux/compat.h | 8 +++++++-
include/linux/syscalls.h | 4 ++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index b1a5562b3215..c68acc47da57 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -72,6 +72,9 @@
*/
#ifndef COMPAT_SYSCALL_DEFINEx
#define COMPAT_SYSCALL_DEFINEx(x, name, ...) \
+ __diag_push(); \
+ __diag_ignore(GCC, 8, "-Wattribute-alias", \
+ "Type aliasing is used to sanitize syscall arguments");\
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
__attribute__((alias(__stringify(__se_compat_sys##name)))); \
@@ -80,8 +83,11 @@
asmlinkage long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
asmlinkage long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
{ \
- return __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__));\
+ long ret = __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__));\
+ __MAP(x,__SC_TEST,__VA_ARGS__); \
+ return ret; \
} \
+ __diag_pop(); \
static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
#endif /* COMPAT_SYSCALL_DEFINEx */
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 73810808cdf2..a368a68cb667 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -231,6 +231,9 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
*/
#ifndef __SYSCALL_DEFINEx
#define __SYSCALL_DEFINEx(x, name, ...) \
+ __diag_push(); \
+ __diag_ignore(GCC, 8, "-Wattribute-alias", \
+ "Type aliasing is used to sanitize syscall arguments");\
asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
__attribute__((alias(__stringify(__se_sys##name)))); \
ALLOW_ERROR_INJECTION(sys##name, ERRNO); \
@@ -243,6 +246,7 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
return ret; \
} \
+ __diag_pop(); \
static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
#endif /* __SYSCALL_DEFINEx */
--
2.17.1
On Fri, Jun 15, 2018 at 05:53:19PM -0700, Paul Burton wrote:
> 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 master as of 9215310cf13b ("Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net").
>
> Thanks,
> Paul
>
> Arnd Bergmann (2):
> kbuild: add macro for controlling warnings to linux/compiler.h
> disable -Wattribute-alias warning for SYSCALL_DEFINEx()
>
> Paul Burton (1):
> Revert "powerpc: fix build failure by disabling attribute-alias
> warning in pci_32"
>
> arch/powerpc/kernel/pci_32.c | 4 ---
> include/linux/compat.h | 8 ++++-
> include/linux/compiler-gcc.h | 66 ++++++++++++++++++++++++++++++++++
> include/linux/compiler_types.h | 18 ++++++++++
> include/linux/syscalls.h | 4 +++
> 5 files changed, 95 insertions(+), 5 deletions(-)
Hello Paul,
I tested the series out with the new OpenRISC 9.0.0 port and the
-Wattribute-alias warnings are gone. Thank you.
Using toolchain binaries from:
https://github.com/stffrdhrn/gcc/releases/tag/or1k-9.0.0-20180613
For the series:
Tested-by: Stafford Horne <[email protected]>
-Stafford
Le 16/06/2018 à 02:53, Paul Burton a écrit :
> 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 master as of 9215310cf13b ("Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net").
>
> Thanks,
> Paul
>
> Arnd Bergmann (2):
> kbuild: add macro for controlling warnings to linux/compiler.h
> disable -Wattribute-alias warning for SYSCALL_DEFINEx()
>
> Paul Burton (1):
> Revert "powerpc: fix build failure by disabling attribute-alias
> warning in pci_32"
>
> arch/powerpc/kernel/pci_32.c | 4 ---
> include/linux/compat.h | 8 ++++-
> include/linux/compiler-gcc.h | 66 ++++++++++++++++++++++++++++++++++
> include/linux/compiler_types.h | 18 ++++++++++
> include/linux/syscalls.h | 4 +++
> 5 files changed, 95 insertions(+), 5 deletions(-)
>
Works well, thanks.
You can then also revert 2479bfc9bc600dcce7f932d52dcfa8d677c41f93
("powerpc: Fix build by disabling attribute-alias warning for
SYSCALL_DEFINEx")
Christophe
Le 16/06/2018 à 02:53, Paul Burton a écrit :
> From: Arnd Bergmann <[email protected]>
>
> 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.
>
> [[email protected]:
> - 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 <[email protected]>
> Signed-off-by: Paul Burton <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Gideon Israel Dsouza <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Stafford Horne <[email protected]>
> Cc: Khem Raj <[email protected]>
> Cc: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Tested-by: Christophe Leroy <[email protected]>
> ---
>
> 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 */
>
Le 16/06/2018 à 02:53, Paul Burton a écrit :
> From: Arnd Bergmann <[email protected]>
>
> gcc-8 warns for every single definition of a system call entry
> point, e.g.:
>
> include/linux/compat.h:56:18: error: 'compat_sys_rt_sigprocmask' alias between functions of incompatible types 'long int(int, compat_sigset_t *, compat_sigset_t *, compat_size_t)' {aka 'long int(int, struct <anonymous> *, struct <anonymous> *, unsigned int)'} and 'long int(long int, long int, long int, long int)' [-Werror=attribute-alias]
> asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))\
> ^~~~~~~~~~
> include/linux/compat.h:45:2: note: in expansion of macro 'COMPAT_SYSCALL_DEFINEx'
> COMPAT_SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
> ^~~~~~~~~~~~~~~~~~~~~~
> kernel/signal.c:2601:1: note: in expansion of macro 'COMPAT_SYSCALL_DEFINE4'
> COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, int, how, compat_sigset_t __user *, nset,
> ^~~~~~~~~~~~~~~~~~~~~~
> include/linux/compat.h:60:18: note: aliased declaration here
> asmlinkage long compat_SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))\
> ^~~~~~~~~~
>
> The new warning seems reasonable in principle, but it doesn't
> help us here, since we rely on the type mismatch to sanitize the
> system call arguments. After I reported this as GCC PR82435, a new
> -Wno-attribute-alias option was added that could be used to turn the
> warning off globally on the command line, but I'd prefer to do it a
> little more fine-grained.
>
> Interestingly, turning a warning off and on again inside of
> a single macro doesn't always work, in this case I had to add
> an extra statement inbetween and decided to copy the __SC_TEST
> one from the native syscall to the compat syscall macro. See
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83256 for more details
> about this.
>
> [[email protected]:
> - Rebase atop current master.
> - Split GCC & version arguments to __diag_ignore() in order to match
> changes to the preceding patch.
> - Add the comment argument to match the preceding patch.]
>
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82435
> Signed-off-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Paul Burton <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Gideon Israel Dsouza <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Stafford Horne <[email protected]>
> Cc: Khem Raj <[email protected]>
> Cc: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Tested-by: Christophe Leroy <[email protected]>
> ---
>
> include/linux/compat.h | 8 +++++++-
> include/linux/syscalls.h | 4 ++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index b1a5562b3215..c68acc47da57 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -72,6 +72,9 @@
> */
> #ifndef COMPAT_SYSCALL_DEFINEx
> #define COMPAT_SYSCALL_DEFINEx(x, name, ...) \
> + __diag_push(); \
> + __diag_ignore(GCC, 8, "-Wattribute-alias", \
> + "Type aliasing is used to sanitize syscall arguments");\
> asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
> asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
> __attribute__((alias(__stringify(__se_compat_sys##name)))); \
> @@ -80,8 +83,11 @@
> asmlinkage long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
> asmlinkage long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
> { \
> - return __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__));\
> + long ret = __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__));\
> + __MAP(x,__SC_TEST,__VA_ARGS__); \
> + return ret; \
> } \
> + __diag_pop(); \
> static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
> #endif /* COMPAT_SYSCALL_DEFINEx */
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 73810808cdf2..a368a68cb667 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -231,6 +231,9 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
> */
> #ifndef __SYSCALL_DEFINEx
> #define __SYSCALL_DEFINEx(x, name, ...) \
> + __diag_push(); \
> + __diag_ignore(GCC, 8, "-Wattribute-alias", \
> + "Type aliasing is used to sanitize syscall arguments");\
> asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
> __attribute__((alias(__stringify(__se_sys##name)))); \
> ALLOW_ERROR_INJECTION(sys##name, ERRNO); \
> @@ -243,6 +246,7 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
> __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
> return ret; \
> } \
> + __diag_pop(); \
> static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
> #endif /* __SYSCALL_DEFINEx */
>
>
Le 16/06/2018 à 02:53, Paul Burton a écrit :
> With SYSCALL_DEFINEx() disabling -Wattribute-alias generically, there's
> no need to duplicate that for PowerPC's pciconfig_iobase syscall.
>
> This reverts commit 415520373975 ("powerpc: fix build failure by
> disabling attribute-alias warning in pci_32").
>
> Signed-off-by: Paul Burton <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Gideon Israel Dsouza <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Stafford Horne <[email protected]>
> Cc: Khem Raj <[email protected]>
> Cc: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
Acked-by: Christophe Leroy <[email protected]>
>
> ---
>
> arch/powerpc/kernel/pci_32.c | 4 ----
> 1 file changed, 4 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
>
On Sat, Jun 16, 2018 at 2:53 AM, Paul Burton <[email protected]> wrote:
> 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 master as of 9215310cf13b ("Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net").
>
Sorry I dropped the ball on this earlier, and thanks a lot for picking
it up again! From what I can tell, your version addresses all issues
I was aware of, so we should merge that.
Arnd
Paul Burton <[email protected]> writes:
> With SYSCALL_DEFINEx() disabling -Wattribute-alias generically, there's
> no need to duplicate that for PowerPC's pciconfig_iobase syscall.
>
> This reverts commit 415520373975 ("powerpc: fix build failure by
> disabling attribute-alias warning in pci_32").
>
> Signed-off-by: Paul Burton <[email protected]>
> ---
>
> arch/powerpc/kernel/pci_32.c | 4 ----
> 1 file changed, 4 deletions(-)
Acked-by: Michael Ellerman <[email protected]>
cheers
Hi.
2018-06-16 9:53 GMT+09:00 Paul Burton <[email protected]>:
> From: Arnd Bergmann <[email protected]>
>
> 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.
>
> [[email protected]:
> - 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 <[email protected]>
> Signed-off-by: Paul Burton <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Gideon Israel Dsouza <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Stafford Horne <[email protected]>
> Cc: Khem Raj <[email protected]>
> Cc: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
>
> 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 <[email protected]>:
> > 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
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.
Thanks,
Paul
Arnd Bergmann (2):
kbuild: add macro for controlling warnings to linux/compiler.h
disable -Wattribute-alias warning for SYSCALL_DEFINEx()
Paul Burton (1):
powerpc: Remove -Wattribute-alias pragmas
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 ----
include/linux/compat.h | 8 +++++++-
include/linux/compiler-gcc.h | 27 +++++++++++++++++++++++++++
include/linux/compiler_types.h | 18 ++++++++++++++++++
include/linux/syscalls.h | 4 ++++
11 files changed, 56 insertions(+), 33 deletions(-)
--
2.17.1
From: Arnd Bergmann <[email protected]>
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.
[[email protected]:
- 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.
- Translate severity to GCC-specific pragmas in linux/compiler-gcc.h
rather than using GCC-specific in linux/compiler_types.h.
- Drop all but GCC 8 macros, since we only need to define macros for
versions that we need to introduce pragmas for, and as of this
series that's just GCC 8.
- Capitalize comments in linux/compiler-gcc.h to match the style of
the rest of the file.
- Line up macro definitions with tabs in linux/compiler-gcc.h.]
Signed-off-by: Arnd Bergmann <[email protected]>
Signed-off-by: Paul Burton <[email protected]>
Tested-by: Christophe Leroy <[email protected]>
Tested-by: Stafford Horne <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Gideon Israel Dsouza <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Stafford Horne <[email protected]>
Cc: Khem Raj <[email protected]>
Cc: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Changes in v2:
- Add version argument to fallback __diag_GCC definition.
- Translate severity from generic ignore,warn,error to GCC-specific
pragma content ignored,warning,error in linux/compiler-gcc.h in order
to keep linux/compiler_types.h generic per feedback from Masahiro
Yamada.
- Drop all but GCC 8 macros, since we only need to define macros for
versions that we need to introduce pragmas for, and as of this series
that's just GCC 8.
- Capitalize comments in linux/compiler-gcc.h to match the style of the
rest of the file.
- Line up macro definitions with tabs in linux/compiler-gcc.h.
include/linux/compiler-gcc.h | 27 +++++++++++++++++++++++++++
include/linux/compiler_types.h | 18 ++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index f1a7492a5cc8..5067a90af9c3 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -347,3 +347,30 @@
#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, severity, s) \
+ __diag_GCC_ ## version(__diag_GCC_ ## severity s)
+
+/* Severity used in pragma directives */
+#define __diag_GCC_ignore ignored
+#define __diag_GCC_warn warning
+#define __diag_GCC_error error
+
+/* Compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */
+#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))
+#else
+#define __diag(s)
+#endif
+
+#if GCC_VERSION >= 80000
+#define __diag_GCC_8(s) __diag(s)
+#else
+#define __diag_GCC_8(s)
+#endif
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 6b79a9bba9a7..a8ba6b04152c 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(version, severity, string)
+#endif
+
+#define __diag_push() __diag(push)
+#define __diag_pop() __diag(pop)
+
+#define __diag_ignore(compiler, version, option, comment) \
+ __diag_ ## compiler(version, ignore, option)
+#define __diag_warn(compiler, version, option, comment) \
+ __diag_ ## compiler(version, warn, option)
+#define __diag_error(compiler, version, option, comment) \
+ __diag_ ## compiler(version, error, option)
+
#endif /* __LINUX_COMPILER_TYPES_H */
--
2.17.1
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 <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Gideon Israel Dsouza <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Stafford Horne <[email protected]>
Cc: Khem Raj <[email protected]>
Cc: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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.
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
--
2.17.1
From: Arnd Bergmann <[email protected]>
gcc-8 warns for every single definition of a system call entry
point, e.g.:
include/linux/compat.h:56:18: error: 'compat_sys_rt_sigprocmask' alias between functions of incompatible types 'long int(int, compat_sigset_t *, compat_sigset_t *, compat_size_t)' {aka 'long int(int, struct <anonymous> *, struct <anonymous> *, unsigned int)'} and 'long int(long int, long int, long int, long int)' [-Werror=attribute-alias]
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))\
^~~~~~~~~~
include/linux/compat.h:45:2: note: in expansion of macro 'COMPAT_SYSCALL_DEFINEx'
COMPAT_SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~~~~~~~~
kernel/signal.c:2601:1: note: in expansion of macro 'COMPAT_SYSCALL_DEFINE4'
COMPAT_SYSCALL_DEFINE4(rt_sigprocmask, int, how, compat_sigset_t __user *, nset,
^~~~~~~~~~~~~~~~~~~~~~
include/linux/compat.h:60:18: note: aliased declaration here
asmlinkage long compat_SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__))\
^~~~~~~~~~
The new warning seems reasonable in principle, but it doesn't
help us here, since we rely on the type mismatch to sanitize the
system call arguments. After I reported this as GCC PR82435, a new
-Wno-attribute-alias option was added that could be used to turn the
warning off globally on the command line, but I'd prefer to do it a
little more fine-grained.
Interestingly, turning a warning off and on again inside of
a single macro doesn't always work, in this case I had to add
an extra statement inbetween and decided to copy the __SC_TEST
one from the native syscall to the compat syscall macro. See
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83256 for more details
about this.
[[email protected]:
- Rebase atop current master.
- Split GCC & version arguments to __diag_ignore() in order to match
changes to the preceding patch.
- Add the comment argument to match the preceding patch.]
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82435
Signed-off-by: Arnd Bergmann <[email protected]>
Signed-off-by: Paul Burton <[email protected]>
Tested-by: Christophe Leroy <[email protected]>
Tested-by: Stafford Horne <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Gideon Israel Dsouza <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Stafford Horne <[email protected]>
Cc: Khem Raj <[email protected]>
Cc: He Zhe <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
Changes in v2: None
include/linux/compat.h | 8 +++++++-
include/linux/syscalls.h | 4 ++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h
index b1a5562b3215..c68acc47da57 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -72,6 +72,9 @@
*/
#ifndef COMPAT_SYSCALL_DEFINEx
#define COMPAT_SYSCALL_DEFINEx(x, name, ...) \
+ __diag_push(); \
+ __diag_ignore(GCC, 8, "-Wattribute-alias", \
+ "Type aliasing is used to sanitize syscall arguments");\
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)); \
asmlinkage long compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
__attribute__((alias(__stringify(__se_compat_sys##name)))); \
@@ -80,8 +83,11 @@
asmlinkage long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
asmlinkage long __se_compat_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
{ \
- return __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__));\
+ long ret = __do_compat_sys##name(__MAP(x,__SC_DELOUSE,__VA_ARGS__));\
+ __MAP(x,__SC_TEST,__VA_ARGS__); \
+ return ret; \
} \
+ __diag_pop(); \
static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
#endif /* COMPAT_SYSCALL_DEFINEx */
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 73810808cdf2..a368a68cb667 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -231,6 +231,9 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
*/
#ifndef __SYSCALL_DEFINEx
#define __SYSCALL_DEFINEx(x, name, ...) \
+ __diag_push(); \
+ __diag_ignore(GCC, 8, "-Wattribute-alias", \
+ "Type aliasing is used to sanitize syscall arguments");\
asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
__attribute__((alias(__stringify(__se_sys##name)))); \
ALLOW_ERROR_INJECTION(sys##name, ERRNO); \
@@ -243,6 +246,7 @@ static inline int is_syscall_trace_event(struct trace_event_call *tp_event)
__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
return ret; \
} \
+ __diag_pop(); \
static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
#endif /* __SYSCALL_DEFINEx */
--
2.17.1
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 <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Gideon Israel Dsouza <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Stafford Horne <[email protected]>
> Cc: Khem Raj <[email protected]>
> Cc: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> ---
> 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 <[email protected]>
>
> 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
>
Hi.
2018-06-20 5:14 GMT+09:00 Paul Burton <[email protected]>:
> From: Arnd Bergmann <[email protected]>
>
> 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.
>
> [[email protected]:
> - 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.
> - Translate severity to GCC-specific pragmas in linux/compiler-gcc.h
> rather than using GCC-specific in linux/compiler_types.h.
> - Drop all but GCC 8 macros, since we only need to define macros for
> versions that we need to introduce pragmas for, and as of this
> series that's just GCC 8.
> - Capitalize comments in linux/compiler-gcc.h to match the style of
> the rest of the file.
> - Line up macro definitions with tabs in linux/compiler-gcc.h.]
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Paul Burton <[email protected]>
> Tested-by: Christophe Leroy <[email protected]>
> Tested-by: Stafford Horne <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Masahiro Yamada <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Heiko Carstens <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Gideon Israel Dsouza <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: Benjamin Herrenschmidt <[email protected]>
> Cc: Paul Mackerras <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Stafford Horne <[email protected]>
> Cc: Khem Raj <[email protected]>
> Cc: He Zhe <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> ---
>
> Changes in v2:
> - Add version argument to fallback __diag_GCC definition.
> - Translate severity from generic ignore,warn,error to GCC-specific
> pragma content ignored,warning,error in linux/compiler-gcc.h in order
> to keep linux/compiler_types.h generic per feedback from Masahiro
> Yamada.
> - Drop all but GCC 8 macros, since we only need to define macros for
> versions that we need to introduce pragmas for, and as of this series
> that's just GCC 8.
> - Capitalize comments in linux/compiler-gcc.h to match the style of the
> rest of the file.
> - Line up macro definitions with tabs in linux/compiler-gcc.h.
>
> include/linux/compiler-gcc.h | 27 +++++++++++++++++++++++++++
> include/linux/compiler_types.h | 18 ++++++++++++++++++
> 2 files changed, 45 insertions(+)
>
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index f1a7492a5cc8..5067a90af9c3 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -347,3 +347,30 @@
> #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, severity, s) \
> + __diag_GCC_ ## version(__diag_GCC_ ## severity s)
> +
> +/* Severity used in pragma directives */
> +#define __diag_GCC_ignore ignored
> +#define __diag_GCC_warn warning
> +#define __diag_GCC_error error
> +
> +/* Compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */
> +#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))
> +#else
> +#define __diag(s)
You can omit the #else here
because it is covered by compiler_types.h
> +#endif
> +
> +#if GCC_VERSION >= 80000
> +#define __diag_GCC_8(s) __diag(s)
> +#else
> +#define __diag_GCC_8(s)
> +#endif
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 6b79a9bba9a7..a8ba6b04152c 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(version, severity, string)
> +#endif
> +
> +#define __diag_push() __diag(push)
> +#define __diag_pop() __diag(pop)
> +
> +#define __diag_ignore(compiler, version, option, comment) \
> + __diag_ ## compiler(version, ignore, option)
> +#define __diag_warn(compiler, version, option, comment) \
> + __diag_ ## compiler(version, warn, option)
> +#define __diag_error(compiler, version, option, comment) \
> + __diag_ ## compiler(version, error, option)
> +
> #endif /* __LINUX_COMPILER_TYPES_H */
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
2018-06-20 4:02 GMT+09:00 Paul Burton <[email protected]>:
> 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 <[email protected]>:
>> > 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 [email protected]
> 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 <[email protected]>:
> 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