[1/2,RFC] kbuild: add macro for controlling warnings to linux/compiler.h

I have occasionally run into a situation where it would make sense to
control a compiler warning from a source file rather than doing so from
a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...)
helpers.

The approach here is similar to what glibc uses, using __diag() and
related macros to encapsulate a _Pragma("GCC diagnostic ...") statement
that gets turned into the respective "#pragma GCC diagnostic ..." by
the preprocessor when the macro gets expanded.

Like glibc, I also have an argument to pass the affected compiler
version, but decided to actually evaluate that one. For now, this
supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7,
GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting
versions is straightforward here. GNU compilers starting with gcc-4.2
could support it in principle, but "#pragma GCC diagnostic push"
was only added in gcc-4.6, so it seems simpler to not deal with those
at all. The same versions show a large number of warnings already,
so it seems easier to just leave it at that and not do a more
fine-grained control for them.

The use cases I found so far include:

- turning off the gcc-8 -Wattribute-alias warning inside of the
  SYSCALL_DEFINEx() macro without having to do it globally.

- Reducing the build time for a simple re-make after a change,
  once we move the warnings from ./Makefile and
  ./scripts/Makefile.extrawarn into linux/compiler.h

- More control over the warnings based on other configurations,
  using preprocessor syntax instead of Makefile syntax. This should make
  it easier for the average developer to understand and change things.

- Adding an easy way to turn the W=1 option on unconditionally
  for a subdirectory or a specific file. This has been requested
  by several developers in the past that want to have their subsystems
  W=1 clean.

- Integrating clang better into the build systems. Clang supports
  more warnings than GCC, and we probably want to classify them
  as default, W=1, W=2 etc, but there are cases in which the
  warnings should be classified differently due to excessive false
  positives from one or the other compiler.

- Adding a way to turn the default warnings into errors (e.g. using
  a new "make E=0" tag) while not also turning the W=1 warnings into
  errors.

This patch for now just adds the minimal infrastructure in order to
do the first of the list above. As the #pragma GCC diagnostic
takes precedence over command line options, the next step would be
to convert a lot of the individual Makefiles that set nonstandard
options to use __diag() instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
I'm marking this RFC for now, as I'd like to get consensus about
whether we want to go down this particular route first, and maybe
if someone can come up with better naming for the macros.
---
 include/linux/compiler-gcc.h   | 64 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/compiler_types.h | 10 +++++++
 2 files changed, 74 insertions(+)

-- 
2.9.0

On Tue, Dec 5, 2017 at 7:32 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> I have occasionally run into a situation where it would make sense to

> control a compiler warning from a source file rather than doing so from

> a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...)

> helpers.

>

> The approach here is similar to what glibc uses, using __diag() and

> related macros to encapsulate a _Pragma("GCC diagnostic ...") statement

> that gets turned into the respective "#pragma GCC diagnostic ..." by

> the preprocessor when the macro gets expanded.

>

> Like glibc, I also have an argument to pass the affected compiler

> version, but decided to actually evaluate that one. For now, this

> supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7,

> GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting

> versions is straightforward here. GNU compilers starting with gcc-4.2

> could support it in principle, but "#pragma GCC diagnostic push"

> was only added in gcc-4.6, so it seems simpler to not deal with those

> at all. The same versions show a large number of warnings already,

> so it seems easier to just leave it at that and not do a more

> fine-grained control for them.

>

> The use cases I found so far include:

>

> - turning off the gcc-8 -Wattribute-alias warning inside of the

>   SYSCALL_DEFINEx() macro without having to do it globally.

>

> - Reducing the build time for a simple re-make after a change,

>   once we move the warnings from ./Makefile and

>   ./scripts/Makefile.extrawarn into linux/compiler.h

>

> - More control over the warnings based on other configurations,

>   using preprocessor syntax instead of Makefile syntax. This should make

>   it easier for the average developer to understand and change things.

>

> - Adding an easy way to turn the W=1 option on unconditionally

>   for a subdirectory or a specific file. This has been requested

>   by several developers in the past that want to have their subsystems

>   W=1 clean.

>

> - Integrating clang better into the build systems. Clang supports

>   more warnings than GCC, and we probably want to classify them

>   as default, W=1, W=2 etc, but there are cases in which the

>   warnings should be classified differently due to excessive false

>   positives from one or the other compiler.

>

> - Adding a way to turn the default warnings into errors (e.g. using

>   a new "make E=0" tag) while not also turning the W=1 warnings into

>   errors.

>

> This patch for now just adds the minimal infrastructure in order to

> do the first of the list above. As the #pragma GCC diagnostic

> takes precedence over command line options, the next step would be

> to convert a lot of the individual Makefiles that set nonstandard

> options to use __diag() instead.

>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

> I'm marking this RFC for now, as I'd like to get consensus about

> whether we want to go down this particular route first, and maybe

> if someone can come up with better naming for the macros.


I like this. I wonder if it would be a good idea to add an additional
argument that forces documentation of the reason for adding a diag
marking? Something like:

__diag_warn(GCC_7, vla, "No VLAs should be used in this code");

-Kees

> ---

>  include/linux/compiler-gcc.h   | 64 ++++++++++++++++++++++++++++++++++++++++++

>  include/linux/compiler_types.h | 10 +++++++

>  2 files changed, 74 insertions(+)

>

> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h

> index 2272ded07496..5d595cfdb2c4 100644

> --- a/include/linux/compiler-gcc.h

> +++ b/include/linux/compiler-gcc.h

> @@ -329,3 +329,67 @@

>   * code

>   */

>  #define uninitialized_var(x) x = x

> +

> +/*

> + * turn individual warnings and errors on and off locally, depending

> + * on version.

> + */

> +#if GCC_VERSION >= 40600

> +#define __diag_str1(s) #s

> +#define __diag_str(s) __diag_str1(s)

> +#define __diag(s) _Pragma(__diag_str(GCC diagnostic s))

> +

> +/* compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */

> +#define __diag_GCC_4_6(s) __diag(s)

> +#else

> +#define __diag(s)

> +#define __diag_GCC_4_6(s)

> +#endif

> +

> +#if GCC_VERSION >= 40700

> +#define __diag_GCC_4_7(s) __diag(s)

> +#else

> +#define __diag_GCC_4_7(s)

> +#endif

> +

> +#if GCC_VERSION >= 40800

> +#define __diag_GCC_4_8(s) __diag(s)

> +#else

> +#define __diag_GCC_4_8(s)

> +#endif

> +

> +#if GCC_VERSION >= 40900

> +#define __diag_GCC_4_9(s) __diag(s)

> +#else

> +#define __diag_GCC_4_9(s)

> +#endif

> +

> +#if GCC_VERSION >= 50000

> +#define __diag_GCC_5(s) __diag(s)

> +#else

> +#define __diag_GCC_5(s)

> +#endif

> +

> +#if GCC_VERSION >= 60000

> +#define __diag_GCC_6(s) __diag(s)

> +#else

> +#define __diag_GCC_6(s)

> +#endif

> +

> +#if GCC_VERSION >= 70000

> +#define __diag_GCC_7(s) __diag(s)

> +#else

> +#define __diag_GCC_7(s)

> +#endif

> +

> +#if GCC_VERSION >= 80000

> +#define __diag_GCC_8(s) __diag(s)

> +#else

> +#define __diag_GCC_8(s)

> +#endif

> +

> +#if GCC_VERSION >= 90000

> +#define __diag_GCC_9(s) __diag(s)

> +#else

> +#define __diag_GCC_9(s)

> +#endif

> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h

> index 6b79a9bba9a7..7e7664d57adb 100644

> --- a/include/linux/compiler_types.h

> +++ b/include/linux/compiler_types.h

> @@ -271,4 +271,14 @@ struct ftrace_likely_data {

>  # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))

>  #endif

>

> +#ifndef __diag

> +#define __diag(string)

> +#endif

> +

> +#define __diag_push()                  __diag(push)

> +#define __diag_ignore(version, option) __diag_ ## version (ignored option)

> +#define __diag_warn(version, option)   __diag_ ## version (warning option)

> +#define __diag_error(version, option)  __diag_ ## version (error   option)

> +#define __diag_pop()                   __diag(pop)

> +

>  #endif /* __LINUX_COMPILER_TYPES_H */

> --

> 2.9.0

>




-- 
Kees Cook
Pixel Security

On Tue, Dec 5, 2017 at 8:25 PM, Kees Cook <keescook@chromium.org> wrote:

> I like this. I wonder if it would be a good idea to add an additional

> argument that forces documentation of the reason for adding a diag

> marking? Something like:

>

> __diag_warn(GCC_7, vla, "No VLAs should be used in this code");


This would be similar to what glibc does, it names the (ignore) macro
DIAG_IGNORE_NEEDS_COMMENT(), and by convention requires
a comment block in front of it. Not sure if it will actually work in the kernel
where the reviews are much more scattered across subsystem maintainers,
but we could try it.

       Arnd

Hi Arnd,


2017-12-06 0:32 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> I have occasionally run into a situation where it would make sense to

> control a compiler warning from a source file rather than doing so from

> a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...)

> helpers.

>

> The approach here is similar to what glibc uses, using __diag() and

> related macros to encapsulate a _Pragma("GCC diagnostic ...") statement

> that gets turned into the respective "#pragma GCC diagnostic ..." by

> the preprocessor when the macro gets expanded.

>

> Like glibc, I also have an argument to pass the affected compiler

> version, but decided to actually evaluate that one. For now, this

> supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7,

> GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting

> versions is straightforward here. GNU compilers starting with gcc-4.2

> could support it in principle, but "#pragma GCC diagnostic push"

> was only added in gcc-4.6, so it seems simpler to not deal with those

> at all. The same versions show a large number of warnings already,

> so it seems easier to just leave it at that and not do a more

> fine-grained control for them.

>

> The use cases I found so far include:

>

> - turning off the gcc-8 -Wattribute-alias warning inside of the

>   SYSCALL_DEFINEx() macro without having to do it globally.

>

> - Reducing the build time for a simple re-make after a change,

>   once we move the warnings from ./Makefile and

>   ./scripts/Makefile.extrawarn into linux/compiler.h


Do you mean, list default warnings in linux/compiler.h

like

__diag_ignore(GCC_*, "-Wunused-but-set-variable");
__diag_ignore(GCC_*, "-Wunused-const-variable");

instead of

KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)

Is this what you mean?


I wonder if we should for all files to include <linux/compiler.h>...



> - More control over the warnings based on other configurations,

>   using preprocessor syntax instead of Makefile syntax. This should make

>   it easier for the average developer to understand and change things.

>

> - Adding an easy way to turn the W=1 option on unconditionally

>   for a subdirectory or a specific file. This has been requested

>   by several developers in the past that want to have their subsystems

>   W=1 clean.

>

> - Integrating clang better into the build systems. Clang supports

>   more warnings than GCC, and we probably want to classify them

>   as default, W=1, W=2 etc, but there are cases in which the

>   warnings should be classified differently due to excessive false

>   positives from one or the other compiler.

>

> - Adding a way to turn the default warnings into errors (e.g. using

>   a new "make E=0" tag) while not also turning the W=1 warnings into

>   errors.

>

> This patch for now just adds the minimal infrastructure in order to

> do the first of the list above. As the #pragma GCC diagnostic

> takes precedence over command line options, the next step would be

> to convert a lot of the individual Makefiles that set nonstandard

> options to use __diag() instead.



GCC manual says:
  Note that not all diagnostics are modifiable; at the moment only warnings
  (normally controlled by ‘-W…’) can be controlled, and not all of them.

https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas


Actually, my compiler does not react
to __diag_*(GCC_*, "-Wunused-but-set-variable") at all.

Is it possible to replace command line flags?



> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

> I'm marking this RFC for now, as I'd like to get consensus about

> whether we want to go down this particular route first, and maybe

> if someone can come up with better naming for the macros.

> ---

>  include/linux/compiler-gcc.h   | 64 ++++++++++++++++++++++++++++++++++++++++++

>  include/linux/compiler_types.h | 10 +++++++

>  2 files changed, 74 insertions(+)

>

> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h

> index 2272ded07496..5d595cfdb2c4 100644

> --- a/include/linux/compiler-gcc.h

> +++ b/include/linux/compiler-gcc.h

> @@ -329,3 +329,67 @@

>   * code

>   */

>  #define uninitialized_var(x) x = x

> +

> +/*

> + * turn individual warnings and errors on and off locally, depending

> + * on version.

> + */

> +#if GCC_VERSION >= 40600

> +#define __diag_str1(s) #s

> +#define __diag_str(s) __diag_str1(s)

> +#define __diag(s) _Pragma(__diag_str(GCC diagnostic s))

> +

> +/* compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */

> +#define __diag_GCC_4_6(s) __diag(s)

> +#else

> +#define __diag(s)

> +#define __diag_GCC_4_6(s)

> +#endif

> +

> +#if GCC_VERSION >= 40700

> +#define __diag_GCC_4_7(s) __diag(s)

> +#else

> +#define __diag_GCC_4_7(s)

> +#endif

> +

> +#if GCC_VERSION >= 40800

> +#define __diag_GCC_4_8(s) __diag(s)

> +#else

> +#define __diag_GCC_4_8(s)

> +#endif

> +

> +#if GCC_VERSION >= 40900

> +#define __diag_GCC_4_9(s) __diag(s)

> +#else

> +#define __diag_GCC_4_9(s)

> +#endif

> +

> +#if GCC_VERSION >= 50000

> +#define __diag_GCC_5(s) __diag(s)

> +#else

> +#define __diag_GCC_5(s)

> +#endif

> +

> +#if GCC_VERSION >= 60000

> +#define __diag_GCC_6(s) __diag(s)

> +#else

> +#define __diag_GCC_6(s)

> +#endif

> +

> +#if GCC_VERSION >= 70000

> +#define __diag_GCC_7(s) __diag(s)

> +#else

> +#define __diag_GCC_7(s)

> +#endif

> +

> +#if GCC_VERSION >= 80000

> +#define __diag_GCC_8(s) __diag(s)

> +#else

> +#define __diag_GCC_8(s)

> +#endif

> +

> +#if GCC_VERSION >= 90000

> +#define __diag_GCC_9(s) __diag(s)

> +#else

> +#define __diag_GCC_9(s)

> +#endif



For linux/compiler-intel.h case,
these macros are not defined at all,
so __diag_ignore(GCC_*, ...) will cause compile error.

Clang includes linux/compiler-gcc.h as well,
so it should be OK.

Interestingly, clang also defines GCC version,
but I have no idea the correspondence between
gcc version vs. clang version.




> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h

> index 6b79a9bba9a7..7e7664d57adb 100644

> --- a/include/linux/compiler_types.h

> +++ b/include/linux/compiler_types.h

> @@ -271,4 +271,14 @@ struct ftrace_likely_data {

>  # define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))

>  #endif

>

> +#ifndef __diag

> +#define __diag(string)

> +#endif

> +

> +#define __diag_push()                  __diag(push)

> +#define __diag_ignore(version, option) __diag_ ## version (ignored option)

> +#define __diag_warn(version, option)   __diag_ ## version (warning option)

> +#define __diag_error(version, option)  __diag_ ## version (error   option)

> +#define __diag_pop()                   __diag(pop)

> +

>  #endif /* __LINUX_COMPILER_TYPES_H */

> --

> 2.9.0

>

> --

> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in

> the body of a message to majordomo@vger.kernel.org

> More majordomo info at  http://vger.kernel.org/majordomo-info.html




-- 
Best Regards
Masahiro Yamada

On Mon, Dec 18, 2017 at 10:48 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2017-12-06 0:32 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:

>> I have occasionally run into a situation where it would make sense to

>> control a compiler warning from a source file rather than doing so from

>> a Makefile using the $(cc-disable-warning, ...) or $(cc-option, ...)

>> helpers.

>>

>> The approach here is similar to what glibc uses, using __diag() and

>> related macros to encapsulate a _Pragma("GCC diagnostic ...") statement

>> that gets turned into the respective "#pragma GCC diagnostic ..." by

>> the preprocessor when the macro gets expanded.

>>

>> Like glibc, I also have an argument to pass the affected compiler

>> version, but decided to actually evaluate that one. For now, this

>> supports GCC_4_6, GCC_4_7, GCC_4_8, GCC_4_9, GCC_5, GCC_6, GCC_7,

>> GCC_8 and GCC_9. Adding support for CLANG_5 and other interesting

>> versions is straightforward here. GNU compilers starting with gcc-4.2

>> could support it in principle, but "#pragma GCC diagnostic push"

>> was only added in gcc-4.6, so it seems simpler to not deal with those

>> at all. The same versions show a large number of warnings already,

>> so it seems easier to just leave it at that and not do a more

>> fine-grained control for them.

>>

>> The use cases I found so far include:

>>

>> - turning off the gcc-8 -Wattribute-alias warning inside of the

>>   SYSCALL_DEFINEx() macro without having to do it globally.

>>

>> - Reducing the build time for a simple re-make after a change,

>>   once we move the warnings from ./Makefile and

>>   ./scripts/Makefile.extrawarn into linux/compiler.h

>

> Do you mean, list default warnings in linux/compiler.h

>

> like

>

> __diag_ignore(GCC_*, "-Wunused-but-set-variable");

> __diag_ignore(GCC_*, "-Wunused-const-variable");

>

> instead of

>

> KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)

> KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)

>

> Is this what you mean?


In principle yes, though my plan was a bit more sophisticated than this:
I want to make those definitions specific to both the W= level and the compiler
version, so you can say things like

KBUILD_WARN(0, GCC_4_6, "-Wall")
KBUILD_WARN(0, CLANG5, "-Wall")

KBUILD_WARN(1, GCC_4_6, "-Wextra")
KBUILD_WARN(1, CLANG5, "-Wextra")

KBUILD_WARN(2, CLANG5, "-Wmost")

KBUILD_WARN(3, CLANG5, "-Weverything")

With the above, "make W=12 E=01" should would lead to
-Wall and -Wextra warnings to become errors on all compilers,
but also enable -Wmost as warnings rather than errors on clang.

> I wonder if we should for all files to include <linux/compiler.h>...


I assumed we already did that, but maybe I was wrong here. I see
that we include include/linux/kconfig.h everywhere, but I don't see
that for compiler.h. This would obviously be a requirement to do first.

>> This patch for now just adds the minimal infrastructure in order to

>> do the first of the list above. As the #pragma GCC diagnostic

>> takes precedence over command line options, the next step would be

>> to convert a lot of the individual Makefiles that set nonstandard

>> options to use __diag() instead.

>

>

> GCC manual says:

>   Note that not all diagnostics are modifiable; at the moment only warnings

>   (normally controlled by ‘-W…’) can be controlled, and not all of them.

>

> https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html#Diagnostic-Pragmas

>

>

> Actually, my compiler does not react

> to __diag_*(GCC_*, "-Wunused-but-set-variable") at all.

>

> Is it possible to replace command line flags?


My interpretation was that everything that had a -W option could be controlled
with a pragma as well. While we can turn any warning into an error, we cannot
turn most errors into warnings.

When I experimented with this a couple of months ago, I also saw that
"gcc -Q --help=warnings" reports some warnings that are language
specific, and it refuses to enable e.g. a fortran specific warning when
building a C file.

> For linux/compiler-intel.h case,

> these macros are not defined at all,

> so __diag_ignore(GCC_*, ...) will cause compile error.


Right, that would need to be fixed. I think it's ok to just ignore all
those statements and just continue with the default warnings.

> Clang includes linux/compiler-gcc.h as well,

> so it should be OK.

>

> Interestingly, clang also defines GCC version,

> but I have no idea the correspondence between

> gcc version vs. clang version.


I tried clang-3.9 and clang-5, both of which report compatibility with
gcc-4.2.1. It seems that for the most part, clang tries to implement
the  gcc warning options, but there are always some it does not have.
I can probably come up with a list of warning options that all
supported versions of either gcc or clang understand, and have
those set independent of the version, but for all others require
listing the specific version that introduces it. In some cases a
warning gets introduced in one version but isn't actually usable
until a later version.

       Arnd