This is not just a matter of style; this is a matter of semantics,
especially with regard to:
* const Correctness.
A const-declared variable must be initialized when defined.
* Conditional Compilation.
When there is complex interaction between compile-time
configuration options, it's essential to be able to
make declarations where needed; otherwise unnecessary
gymnastics are required to silence the compiler.
Gentleman... Just let people say exactly what they mean to say.
Requiring every declaration to be at the top of a block is an
antiquated, vestigial naivete from a time when C was just a
glorified abstraction over conventional patterns in assembly
programming.
We are not just programming anymore. We are now encoding our
very thoughts, and thus we need this expressiveness in order
to capture those thoughts with sufficient clarity.
Therefore, this commit removes:
-Wdeclaration-after-statement
Signed-off-by: Michael Witten <[email protected]>
---
Makefile | 3 ---
arch/arm64/kernel/vdso32/Makefile | 1 -
tools/power/acpi/Makefile.config | 1 -
tools/power/cpupower/Makefile | 1 -
tools/scripts/Makefile.include | 1 -
5 files changed, 7 deletions(-)
diff --git a/Makefile b/Makefile
index 254e80a96b23..88ad4e430834 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
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index 5139a5f19256..672008c735fb 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -88,7 +88,6 @@ 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,)
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/tools/power/acpi/Makefile.config b/tools/power/acpi/Makefile.config
index 54a2857c2510..70ac50cfd7eb 100644
--- a/tools/power/acpi/Makefile.config
+++ b/tools/power/acpi/Makefile.config
@@ -64,7 +64,6 @@ OPTIMIZATION := $(call cc-supports,-Os,-O2)
WARNINGS := -Wall
WARNINGS += $(call cc-supports,-Wstrict-prototypes)
-WARNINGS += $(call cc-supports,-Wdeclaration-after-statement)
KERNEL_INCLUDE := $(OUTPUT)include
ACPICA_INCLUDE := $(srctree)/../../../drivers/acpi/acpica
diff --git a/tools/power/cpupower/Makefile b/tools/power/cpupower/Makefile
index c8622497ef23..d389179d4484 100644
--- a/tools/power/cpupower/Makefile
+++ b/tools/power/cpupower/Makefile
@@ -118,7 +118,6 @@ 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)
WARNINGS += -Wshadow
override CFLAGS += -DVERSION=\"$(VERSION)\" -DPACKAGE=\"$(PACKAGE)\" \
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index a7974638561c..83c2a5a1ed15 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -21,7 +21,6 @@ endif
# Include saner warnings here, which can catch bugs:
#
EXTRA_WARNINGS := -Wbad-function-cast
-EXTRA_WARNINGS += -Wdeclaration-after-statement
EXTRA_WARNINGS += -Wformat-security
EXTRA_WARNINGS += -Wformat-y2k
EXTRA_WARNINGS += -Winit-self
--
2.22.0
On Sun, Aug 16, 2020 at 04:35:00PM -0000, Michael Witten wrote:
> This is not just a matter of style; this is a matter of semantics,
> especially with regard to:
>
> * const Correctness.
> A const-declared variable must be initialized when defined.
>
> * Conditional Compilation.
> When there is complex interaction between compile-time
> configuration options, it's essential to be able to
> make declarations where needed; otherwise unnecessary
> gymnastics are required to silence the compiler.
>
> Gentleman... Just let people say exactly what they mean to say.
>
> Requiring every declaration to be at the top of a block is an
> antiquated, vestigial naivete from a time when C was just a
> glorified abstraction over conventional patterns in assembly
> programming.
>
> We are not just programming anymore. We are now encoding our
> very thoughts, and thus we need this expressiveness in order
> to capture those thoughts with sufficient clarity.
You obviously need every bit of help in that task, judging by the amount
of clarity (or thoughts, for that matter) visible in the spew above...
NAK. And as for letting people say exactly what they mean to say...
I am tempted to take you on that invitation, but that would be cruel
to gregkh - he would have to reply to inevitable screeds about
CoC-violating postings on l-k.
On Sun, 2020-08-16 at 16:35 +0000, Michael Witten wrote:
> Requiring every declaration to be at the top of a block is an
> antiquated, vestigial naivete from a time when C was just a
> glorified abstraction over conventional patterns in assembly
> programming.
I rather prefer block declarations instead of
sprinkling declarations around with code.
> We are not just programming anymore. We are now encoding our
> very thoughts, and thus we need this expressiveness in order
> to capture those thoughts with sufficient clarity.
So how does this removal have anything to do with
expressiveness and clarity?
Joe Perches (Sun, 16 Aug 2020 10:56:53 -0700):
> I rather prefer block declarations instead of
> sprinkling declarations around with code.
Hey, we all have our guilty pleasures.
Fortunately, even with this patch, you'd still be able to indulge
in your preferred style, or even enforce it among contributors to
the code that you maintain.
However, the following statement should hold:
If merged code is correct (portable, safe, etc.),
then the kernel must build without any warning
about that merged code.
Sometimes, code is clearest (or indeed safest) when it is written
with a variable definition that occurs at a point well within the
body of statements. Authors need to have the option to write such
code; otherwise, style ceases to be means of clarity, and instead
becomes a laborious end unto itself.
Matters of style should probably not be enforced by the build
infrastructure; style is a matter for the maintainer to enforce:
* Perhaps there could be a new build-time switch. By default,
the warning can be off for a normal build; a maintainer can
flip the switch to turn it on locally, and thereby check
whether a patch declares variables unnecessarily hither and
thither, as determined by the maintainer's taste.
* Perhaps `scripts/checkpatch.pl' could be taught about this
issue. Though probably easier said than done, the script
could parse every modified block, and warn about declarations
after statements (but maybe ignore the declarations that
introduce const variables).
* Perhaps there is already linting infrastructure that could be
put to such use.
This way, good code can compile cleanly, and style can just be an
ongoing topic of discussion among contributors.
Sincerely,
Michael Witten
On Mon, 2020-08-17 at 03:37 +0000, Michael Witten wrote:
> Matters of style should probably not be enforced by the build
> infrastructure; style is a matter for the maintainer to enforce:
I rather doubt style advice should be taken from someone who
right justifies fixed pitch block text.
cheers, Joe
Joe Perches (Sun, 16 Aug 2020 10:56:53 -0700):
> On Mon, 2020-08-17 at 03:37 +0000, Michael Witten wrote:
>> Matters of style should probably not be enforced by
>> the build infrastructure; style is a matter for the
>> maintainer to enforce:
>
> I rather doubt style advice should be taken from someone
> who right justifies fixed pitch block text.
>
> cheers, Joe
Clearly, I never offered style advice.
Indeed, the very first sentence I wrote in this thread was:
> This is not just a matter of style; this is a matter of
> semantics[.]
Sincerely,
Michael Witten
As an aside, the venerable 'man' program justifies fixed
pitch text, at least by default.
On Sun 2020-08-16 21:19:23, Joe Perches wrote:
> On Mon, 2020-08-17 at 03:37 +0000, Michael Witten wrote:
> > Matters of style should probably not be enforced by the build
> > infrastructure; style is a matter for the maintainer to enforce:
>
> I rather doubt style advice should be taken from someone who
> right justifies fixed pitch block text.
Ad hominem?
You may be right but this is not fair discussion.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> > This is not just a matter of style; this is a matter of semantics,
> > especially with regard to:
> >
> > * const Correctness.
> > A const-declared variable must be initialized when defined.
> >
> > * Conditional Compilation.
> > When there is complex interaction between compile-time
> > configuration options, it's essential to be able to
> > make declarations where needed; otherwise unnecessary
> > gymnastics are required to silence the compiler.
> >
> > Gentleman... Just let people say exactly what they mean to say.
..
> You obviously need every bit of help in that task, judging by the amount
> of clarity (or thoughts, for that matter) visible in the spew above...
>
> NAK. And as for letting people say exactly what they mean to say...
> I am tempted to take you on that invitation, but that would be cruel
> to gregkh - he would have to reply to inevitable screeds about
> CoC-violating postings on l-k.
We should really get rid of CoC, because I'd really like to see you
_not_ resist that temptation.
But... he's right.
With rust-like programming style with widespread consts, this warning
has to go. And it is causing additional/ugly #ifdefs in some cases.
Maybe author can show examples in kernel .c where disabling the
warning would lead to nicer code. I believe we should give it a try.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek <[email protected]> writes:
> Hi!
>
>> > This is not just a matter of style; this is a matter of semantics,
>> > especially with regard to:
>> >
>> > * const Correctness.
>> > A const-declared variable must be initialized when defined.
>> >
>> > * Conditional Compilation.
>> > When there is complex interaction between compile-time
>> > configuration options, it's essential to be able to
>> > make declarations where needed; otherwise unnecessary
>> > gymnastics are required to silence the compiler.
>> >
>> > Gentleman... Just let people say exactly what they mean to say.
> ..
>
>> You obviously need every bit of help in that task, judging by the amount
>> of clarity (or thoughts, for that matter) visible in the spew above...
>>
>> NAK. And as for letting people say exactly what they mean to say...
>> I am tempted to take you on that invitation, but that would be cruel
>> to gregkh - he would have to reply to inevitable screeds about
>> CoC-violating postings on l-k.
>
> We should really get rid of CoC, because I'd really like to see you
> _not_ resist that temptation.
>
> But... he's right.
>
> With rust-like programming style with widespread consts, this warning
> has to go. And it is causing additional/ugly #ifdefs in some cases.
>
> Maybe author can show examples in kernel .c where disabling the
> warning would lead to nicer code. I believe we should give it a try.
This change came in with 535231e8252e ("[PATCH] add
-Wdeclaration-after-statement"). AKA
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/commit/?id=535231e8252eea14abf4f14d28f6c1c03f5e0f02
Does anyone remember why we added this warning? I had always thought
it's purpose was to ensure we stayed within our chosen dialect of C.
The actual commit says that it was in reaction to gcc miscompiling proc.
Which is a far more serious thing.
With all of the our bumping of our gcc version lately perhaps it is safe
to remove this warning.
Eric
On Mon, Aug 17, 2020 at 2:15 PM Eric W. Biederman <[email protected]> wrote:
>
> Does anyone remember why we added this warning? I had always thought
> it's purpose was to ensure we stayed within our chosen dialect of C.
As far as I'm concerned, that's the primary motivation.
I'm not seeing why we'd suddenly allow the "put variable declarations
anywhere" when we've been able to keep from doing it until now.
We're still building primarily good old K&R ANSI C, just with
extensions. Wild variable placement doesn't seem like a useful
extension.
(Other variable placement improvements are: block-scope variable
declarations inside the "for()" statement is very syntactically
useful, for example. THAT would be useful if we can finally enable it
without gcc going all wonky on us)
Linus
On Mon 2020-08-17 14:29:37, Linus Torvalds wrote:
> On Mon, Aug 17, 2020 at 2:15 PM Eric W. Biederman <[email protected]> wrote:
> >
> > Does anyone remember why we added this warning? I had always thought
> > it's purpose was to ensure we stayed within our chosen dialect of C.
>
> As far as I'm concerned, that's the primary motivation.
>
> I'm not seeing why we'd suddenly allow the "put variable declarations
> anywhere" when we've been able to keep from doing it until now.
>
> We're still building primarily good old K&R ANSI C, just with
> extensions. Wild variable placement doesn't seem like a useful
> extension.
I certainly hope we are not going back to good old K&R C :-).
Submitter believes "wild variable placement" can help with
#ifdefs.. and that may be actually good tradeoff.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Mon, Aug 17, 2020 at 3:09 PM Pavel Machek <[email protected]> wrote:
>
> Submitter believes "wild variable placement" can help with
> #ifdefs.. and that may be actually good tradeoff.
I agree that it can help in some cases.
But it can also make it really hard to find the variable declarations
in other cases. I've seen a lot of code that ends up actively
declaring the variable close to where it's used (because people find
that to be locally more legible) and then it just means that people
who arent' familiar with the code have a much harder time finding it.
I'd instead try to discourage people from using #ifdef's inside code.
Linus
* Linus Torvalds <[email protected]> wrote:
> On Mon, Aug 17, 2020 at 3:09 PM Pavel Machek <[email protected]> wrote:
> >
> > Submitter believes "wild variable placement" can help with
> > #ifdefs.. and that may be actually good tradeoff.
>
> I agree that it can help in some cases.
>
> But it can also make it really hard to find the variable declarations
> in other cases. I've seen a lot of code that ends up actively
> declaring the variable close to where it's used (because people find
> that to be locally more legible) and then it just means that people
> who arent' familiar with the code have a much harder time finding it.
>
> I'd instead try to discourage people from using #ifdef's inside code.
I'm a big fan of -Wdeclaration-after-statement and I think C++ style
mixed variables/statements code has several disadvantages:
- One advantage of -Wdeclaration-after-statement is that it can detect
mismerges that can happen with the 'patch' tool when it applies a
patch with fuzz.
- Also, enforcing -Wdeclaration-after-statement means we have the nice
symmetry that local variable declarations are always at the
beginning of curly brace blocks, which includes function
definitions. This IMO is a very helpful visual clue that allows the
quick reading of kernel code.
- A third advantage is that the grouping of local variables at the
beginning of curly brace blocks encourages smaller, better
structured functions: a large function would look automatically ugly
due to the many local variables crammed at the beginning of it.
So the gentle code structure message is: you can declare new local
variables in a loop construct or branch, at the cost of losing one
level of indentation. If it gets too deep, you are encouraged to split
your logic up better with helper functions. The kind of run-on
mega-functions that C++ style mixed variables often allow looks
*automatically* uglier under -Wdeclaration-after-statement and quickly
breaks simple kernel style rules such as col80 or indentation level
depth or the too high visual complexity of variable definition lines.
Basically the removal of -Wdeclaration-after-statement removes a
helpful symmetry & allows the addition of random noise to our code
base, with very little benefits offered. I'd be sad to see it go.
Thanks,
Ingo
> I'm a big fan of -Wdeclaration-after-statement and I think C++ style
> mixed variables/statements code has several disadvantages:
Agreed.
Personally I think declarations should either be either right
at the top of a function or in a very small code block.
Otherwise they are annoying to find.
You also get very hard to spot bugs unless -Wshadow
is enabled (I can't remember if the linux kernel has
it enabled).
C++ (sort of) has to allow definitions in the middle
of code blocks because it doesn't allow uninitialised
variables - so definitions are best delayed until the
copy-constructor can be used.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
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 <[email protected]>
---
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
The quick RFC patch I just proposed in the parent email is
broken in its implementation. I will submit an updated
version soon.
Michael Witten (Tue, 18 Aug 2020 22:05:00 -0000):
> 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 <[email protected]>
>
> [... PATCH ...]
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 <[email protected]>
---
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