Message-ID: <6f33e183801543429ebf8dea4a009694@gmail.com>
Date:   Thu, 20 Aug 2020 00:15:00 -0000
From:   Michael Witten <mfwitten@...il.com>
To:     linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Ingo Molnar <mingo@...nel.org>,
        Eric Biederman <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        John Levon <john.levon@...ent.com>,
        John Levon <levon@...ementarian.org>,
        Pavel Machek <pavel@....cz>,
        David Laight <David.Laight@...lab.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Sam Ravnborg <sam@...nborg.org>
Subject: [RFC v2] kbuild: Introduce "Warnings for maintainers"

This revision of the patch makes the following notable changes:

  * The high-level control is placed in:

      scripts/Makefile.extrawarn

  * Interactions between "make W=2" and CONFIG_* are handled
    explicitly.

  * The new conditional logic within the makefiles uses 2 spaces
    for indentation rather than a tab character; this is because
    the tab character has special meaning in a makefile, and it
    is therefore best not to use it unless necessary.

  * The files under the following directory have been left alone:

      tools/

    It appears more work needs to be done to communicate ".config"
    settings to the build infastructure in that subtree.

  * This patch has been lightly tested on my machine:

      $ b()
        {
          make "$@" V=1 2>&1 |
            grep --color=always \
              -e declaration-after-statement \
              -e maybe-uninitialized \
              -e no-maybe-uninitialized \
              -e ^
        }
      $ b
      [...]
      $ b W=2
      [...]

    Look for the highlighted "-W" names; also, note that the
    following option gets passed to the compiler when required:

      -Wno-maybe-uninitialized

Sincerely,
Michael Witten

---8<------8<------8<------8<------8<------8<------8<------8<---
This commit introduces the following new Kconfig options:

  CONFIG_MAINTAINERS_WARNINGS
    |
    +-> CONFIG_WARN_DECLARATION_AFTER_STATEMENT
    |
    +-> CONFIG_WARN_MAYBE_UNINITIALIZED

These produce the following menu items:

  -> Kernel hacking
    -> Compile-time checks and compiler options
      * Warnings for maintainers                                [NEW]
        * Warn about mixing variable definitions and statements [NEW]
        * Warn about use of potentially uninitialized variables [NEW]

In short:

  * CONFIG_MAINTAINERS_WARNINGS (default: y)
      is the umbrella control.

  * CONFIG_WARN_DECLARATION_AFTER_STATEMENT (default: y)
      determines whether "-Wdeclaration-after-statement" is used.

  * CONFIG_WARN_MAYBE_UNINITIALIZED (default: n)
      determines whether "-Wmaybe-uninitialized" is used.

Currently, the default is "y" for CONFIG_MAINTAINERS_WARNINGS,
which should lead to the same warning behavior that existed before
this commit; however, it is intended that eventually the default
will be "n" for CONFIG_MAINTAINERS_WARNINGS.

Running "make" with "W=2" should turn on both warnings, unless
they are thwarted by some other Kbuild logic.

Signed-off-by: Michael Witten <mfwitten@...il.com>
---
 lib/Kconfig.debug                 | 64 +++++++++++++++++++++++++++++++++++++++
 Makefile                          |  6 ----
 scripts/Makefile.extrawarn        | 28 +++++++++++++++++
 arch/arm64/kernel/vdso32/Makefile |  7 ++++-
 4 files changed, 98 insertions(+), 7 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e068c3c7189a..b59185dc85bd 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -308,6 +308,70 @@ config FRAME_WARN
 	  Setting this too low will cause a lot of warnings.
 	  Setting it to 0 disables the warning.
 
+config MAINTAINERS_WARNINGS
+	bool "Warnings for maintainers"
+	default y
+	help
+	  These warnings may be useful to maintainers and contributors
+	  when patches are being prepared and reviewed; they may yield
+	  false positives, and are therefore intended to be turned off
+	  for a normal build.
+
+config WARN_DECLARATION_AFTER_STATEMENT
+	bool "Warn about mixing variable definitions and statements"
+	depends on MAINTAINERS_WARNINGS
+	default y
+	help
+	  Turn on the following gcc command-line option:
+
+	    -Wdeclaration-after-statement
+
+	  Traditionally, C code has been written such that variables
+	  are defined at the top of a block (e.g., at the top of a
+	  function body); C99 removed this requirement, allowing the
+	  mixing of variable definitions and statements, but the
+	  tradition has remained a convention of the kernel's coding
+	  style.
+
+	  When reviewing patches, a maintainer may wish to turn this
+	  warning on, in order to catch variable definitions that have
+	  been placed unnecessarily among the statements, and thereby
+	  enforce the dominant coding style.
+
+	  Of course, sometimes it is useful to define a variable among
+	  the statements, especially in the following cases:
+
+	    * Declaring a const variable.
+	    * Dealing with conditional compilation.
+
+	  Therefore, this warning is intended to be turned off for a
+	  normal build, where all of the code has already been merged
+	  succesfully into the repository.
+
+config WARN_MAYBE_UNINITIALIZED
+	bool "Warn about use of potentially uninitialized variables"
+	depends on MAINTAINERS_WARNINGS
+	default n
+	help
+	  Turn on the following gcc command-line option:
+
+	    -Wmaybe-uninitialized
+
+	  Serious bugs can result from using a variable whose value
+	  has never been explicitly initialized. When this warning
+	  is turned on, the compiler will use heuristic analyses of
+	  the code to determine whether a variable has been properly
+	  initialized before it is ever used.
+
+	  However, the heuristic nature of the analyses has often
+	  caused many false positive warnings that programmers find
+	  irritating; sometimes, bugs are introduced in the process
+	  of simply trying to silence the warning.
+
+	  Therefore, this warning is intended to be turned off for a
+	  normal build, where all of the code has already been merged
+	  succesfully into the repository.
+
 config STRIP_ASM_SYMS
 	bool "Strip assembler-generated symbols during link"
 	default n
diff --git a/Makefile b/Makefile
index 9cac6fde3479..51a503411d5b 100644
--- a/Makefile
+++ b/Makefile
@@ -900,9 +900,6 @@ endif
 # arch Makefile may override CC so keep this after arch Makefile is included
 NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 
-# warn about C99 declaration after statement
-KBUILD_CFLAGS += -Wdeclaration-after-statement
-
 # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
 KBUILD_CFLAGS += -Wvla
 
@@ -920,9 +917,6 @@ KBUILD_CFLAGS += $(call cc-disable-warning, stringop-overflow)
 # Another good warning that we'll want to enable eventually
 KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
 
-# Enabled with W=2, disabled by default as noisy
-KBUILD_CFLAGS += $(call cc-disable-warning, maybe-uninitialized)
-
 # disable invalid "can't wrap" optimizations for signed / pointers
 KBUILD_CFLAGS	+= $(call cc-option,-fno-strict-overflow)
 
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 62c275685b75..a9c90a50785b 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -4,6 +4,8 @@
 #
 # There are three warning groups enabled by W=1, W=2, W=3.
 # They are independent, and can be combined like W=12 or W=123.
+#
+# Also, this adds other warnings that can be switched on via .config
 # ==========================================================================
 
 KBUILD_CFLAGS += $(call cc-disable-warning, packed-not-aligned)
@@ -17,6 +19,9 @@ endif
 
 export KBUILD_EXTRA_WARN
 
+extrawarn_already_added_-Wdeclaration-after-statement := n
+extrawarn_already_added_-Wmaybe-uninitialized := n
+
 #
 # W=1 - warnings which may be relevant and do not occur too often
 #
@@ -68,7 +73,13 @@ KBUILD_CFLAGS += $(call cc-option, -Wlogical-op)
 KBUILD_CFLAGS += -Wmissing-field-initializers
 KBUILD_CFLAGS += -Wsign-compare
 KBUILD_CFLAGS += -Wtype-limits
+
+KBUILD_CFLAGS += -Wdeclaration-after-statement
+extrawarn_already_added_-Wdeclaration-after-statement := y
+
 KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized)
+extrawarn_already_added_-Wmaybe-uninitialized := y
+
 KBUILD_CFLAGS += $(call cc-option, -Wunused-macros)
 
 KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN2
@@ -93,3 +104,20 @@ KBUILD_CFLAGS += $(call cc-option, -Wpacked-bitfield-compat)
 KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN3
 
 endif
+
+#
+# Maintainers' Warnings
+#
+ifeq ($(extrawarn_already_added_-Wdeclaration-after-statement), n)
+  ifeq ($(CONFIG_WARN_DECLARATION_AFTER_STATEMENT), y)
+      KBUILD_CFLAGS += -Wdeclaration-after-statement
+  endif
+endif
+
+ifeq ($(extrawarn_already_added_-Wmaybe-uninitialized), n)
+  ifeq ($(CONFIG_WARN_MAYBE_UNINITIALIZED), y)
+    KBUILD_CFLAGS += $(call cc-option,-Wmaybe-uninitialized)
+  else
+    KBUILD_CFLAGS += $(call cc-disable-warning,maybe-uninitialized)
+  endif
+endif
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index 5139a5f19256..561f98f27edd 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -88,7 +88,12 @@ VDSO_CFLAGS += -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
                -std=gnu89
 VDSO_CFLAGS  += -O2
 # Some useful compiler-dependent flags from top-level Makefile
-VDSO_CFLAGS += $(call cc32-option,-Wdeclaration-after-statement,)
+ifeq ($(CONFIG_WARN_DECLARATION_AFTER_STATEMENT), y)
+  VDSO_CFLAGS += $(call cc32-option,-Wdeclaration-after-statement)
+endif
+ifeq ($(CONFIG_WARN_MAYBE_UNINITIALIZED), y)
+  VDSO_CFLAGS += $(call cc32-option,-Wmaybe-uninitialized)
+endif
 VDSO_CFLAGS += $(call cc32-option,-Wno-pointer-sign)
 VDSO_CFLAGS += $(call cc32-option,-fno-strict-overflow)
 VDSO_CFLAGS += $(call cc32-option,-Werror=strict-prototypes)
--
2.22.0

Powered by blists - more mailing lists