[RFC v2] kbuild: Introduce "Warnings for maintainers"

From: Michael Witten
Date: Wed Aug 19 2020 - 20:15:22 EST

Next message: Andy Lutomirski: "[PATCH] x86/debug: Allow a single level of #DB recursion"
Previous message: Sasha Levin: "[PATCH AUTOSEL 5.8 03/27] media: budget-core: Improve exception handling in budget_register()"
In reply to: Michael Witten: "Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
---
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

Next message: Andy Lutomirski: "[PATCH] x86/debug: Allow a single level of #DB recursion"
Previous message: Sasha Levin: "[PATCH AUTOSEL 5.8 03/27] media: budget-core: Improve exception handling in budget_register()"
In reply to: Michael Witten: "Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]