Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'

From: Michael Witten <mfwitten@gmail.com>
To: linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Michal Marek <michal.lkml@markovi.net>,
	Ingo Molnar <mingo@kernel.org>,
	Eric Biederman <ebiederm@xmission.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	John Levon <john.levon@joyent.com>,
	John Levon <levon@movementarian.org>, Pavel Machek <pavel@ucw.cz>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Sam Ravnborg <sam@ravnborg.org>
Subject: Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'
Date: Tue, 18 Aug 2020 22:05:00 -0000	[thread overview]
Message-ID: <5820d82432a0485b9a0d40bc093cfeb8@gmail.com> (raw)
In-Reply-To: <23e2e6c2d3a24954bccb67a3186019b9@gmail.com>

I think there's an important distinction to make between
the following 2 kinds of code:

  * The curated code people just want to build.
  * The new patches that maintainers are reviewing.

Certainly, maintainers should have a wide range of tools
at their disposal to probe the quality of a patch; then,
after bending rules of style to taste, the maintainers
declare the merged code to be curated, after which that
merged code need not be probed so invasively every time
it is built.

To this end, I propose the following new patch, which
introduces some build-time switches that will help
people make this distinction.

  As you know, you can save this email to some path:

    /path/to/email.eml

  Then apply and review the patch as follows:

    $ cd /path/to/linux/repo
    $ git reset --hard HEAD
    $ git checkout --detach origin/master
    $ git am --scissors /path/to/email.eml
    $ git log -1 -p

At this point, the patch is intended as a[n] RFC;
I haven't tested it, and just wanted to get the
idea out there.

Sincerely,
Michael Witten

---8<------8<------8<------8<------8<------8<------8<------8<---
Subject: [PATCH] kbuild: Introduce "Warnings for maintainers"
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
      is the umbrella control.

  * CONFIG_WARN_DECLARATION_AFTER_STATEMENT
      determines whether "-Wdeclaration-after-statement" is used.

  * CONFIG_WARN_MAYBE_UNINITIALIZED
      determines whether "-Wmaybe-uninitialized" is used.

Currently, the default is "y" for each, but it is expected that
eventually the default will be "n" for CONFIG_MAINTAINERS_WARNINGS.

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

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 arch/arm64/kernel/vdso32/Makefile |  7 +++-
 lib/Kconfig.debug                 | 64 +++++++++++++++++++++++++++++++
 scripts/Makefile.extrawarn        |  1 +
 tools/power/acpi/Makefile.config  |  7 +++-
 tools/power/cpupower/Makefile     |  7 +++-
 tools/scripts/Makefile.include    |  9 ++++-
 6 files changed, 91 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index 5139a5f19256..8cc997b9ccef 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)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e068c3c7189a..4e3a87ce77c8 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 y
+	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/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 62c275685b75..afadbdcc7c53 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -68,6 +68,7 @@ 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
 KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized)
 KBUILD_CFLAGS += $(call cc-option, -Wunused-macros)
 
diff --git a/tools/power/acpi/Makefile.config b/tools/power/acpi/Makefile.config
index 54a2857c2510..6fe0b34eddd7 100644
--- a/tools/power/acpi/Makefile.config
+++ b/tools/power/acpi/Makefile.config
@@ -64,7 +64,12 @@ OPTIMIZATION := $(call cc-supports,-Os,-O2)
 
 WARNINGS := -Wall
 WARNINGS += $(call cc-supports,-Wstrict-prototypes)
-WARNINGS += $(call cc-supports,-Wdeclaration-after-statement)
+ifeq($(CONFIG_WARN_DECLARATION_AFTER_STATEMENT), y)
+	WARNINGS += $(call cc-supports,-Wdeclaration-after-statement)
+endif
+ifeq($(CONFIG_WARN_MAYBE_UNINITIALIZED), y)
+	WARNINGS += $(call cc-supports,-Wmaybe-uninitialized)
+endif
 
 KERNEL_INCLUDE := $(OUTPUT)include
 ACPICA_INCLUDE := $(srctree)/../../../drivers/acpi/acpica
diff --git a/tools/power/cpupower/Makefile b/tools/power/cpupower/Makefile
index c8622497ef23..8d26c2003d7d 100644
--- a/tools/power/cpupower/Makefile
+++ b/tools/power/cpupower/Makefile
@@ -118,7 +118,12 @@ OPTIMIZATION := $(call cc-supports,-Os,-O2)
 
 WARNINGS := -Wall -Wchar-subscripts -Wpointer-arith -Wsign-compare
 WARNINGS += $(call cc-supports,-Wno-pointer-sign)
-WARNINGS += $(call cc-supports,-Wdeclaration-after-statement)
+ifeq($(CONFIG_WARN_DECLARATION_AFTER_STATEMENT), y)
+	WARNINGS += $(call cc-supports,-Wdeclaration-after-statement)
+endif
+ifeq($(CONFIG_WARN_MAYBE_UNINITIALIZED), y)
+	WARNINGS += $(call cc-supports,-Wmaybe-uninitialized)
+endif
 WARNINGS += -Wshadow
 
 override CFLAGS += -DVERSION=\"$(VERSION)\" -DPACKAGE=\"$(PACKAGE)\" \
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index a7974638561c..a9acd52b5e84 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -17,11 +17,18 @@ OUTDIR := $(shell cd $(OUTPUT) && pwd)
 $(if $(OUTDIR),, $(error output directory "$(OUTPUT)" does not exist))
 endif
 
+#
+# Maintainers' Warnings
+#
+MAINTAINERS_WARNINGS-y :=
+MAINTAINERS_WARNINGS-$(CONFIG_WARN_DECLARATION_AFTER_STATEMENT) += -Wdeclaration-after-statement
+MAINTAINERS_WARNINGS-$(CONFIG_WARN_MAYBE_UNINITIALIZED) += -Wmaybe-uninitialized
+
 #
 # Include saner warnings here, which can catch bugs:
 #
 EXTRA_WARNINGS := -Wbad-function-cast
-EXTRA_WARNINGS += -Wdeclaration-after-statement
+EXTRA_WARNINGS += $(MAINTAINERS_WARNINGS-y)
 EXTRA_WARNINGS += -Wformat-security
 EXTRA_WARNINGS += -Wformat-y2k
 EXTRA_WARNINGS += -Winit-self
-- 
2.22.0

