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

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

[Michael Witten]

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 <mfwitten@gmail.com>

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

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

[Al Viro]

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.

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Eric W. Biederman]

Pavel Machek <pavel@ucw.cz> 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

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

[Linus Torvalds]

On Mon, Aug 17, 2020 at 2:15 PM Eric W. Biederman <ebiederm@xmission.com> 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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

On Mon 2020-08-17 14:29:37, Linus Torvalds wrote:
> On Mon, Aug 17, 2020 at 2:15 PM Eric W. Biederman <ebiederm@xmission.com> 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Linus Torvalds]

On Mon, Aug 17, 2020 at 3:09 PM Pavel Machek <pavel@ucw.cz> 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

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

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, Aug 17, 2020 at 3:09 PM Pavel Machek <pavel@ucw.cz> 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

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

[David Laight]

> 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)

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

[Joe Perches]

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?

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

[Michael Witten]

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

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

[Joe Perches]

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

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

[Michael Witten]

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.

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Michael Witten]

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

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

[Michael Witten]

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 <mfwitten@gmail.com>
>
> [... PATCH ...]

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

[Michael Witten]

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@gmail.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