Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'
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 -): > 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 > > [... PATCH ...]
Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'
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 --- 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
RE: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'
> 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'
* Linus Torvalds wrote: > On Mon, Aug 17, 2020 at 3:09 PM Pavel Machek 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'
On Mon, Aug 17, 2020 at 3:09 PM Pavel Machek 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'
On Mon 2020-08-17 14:29:37, Linus Torvalds wrote: > On Mon, Aug 17, 2020 at 2:15 PM Eric W. Biederman > 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 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 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 signature.asc Description: Digital signature
Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'
On Mon, Aug 17, 2020 at 2:15 PM Eric W. Biederman 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 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 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'
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 signature.asc Description: Digital signature
Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'
On Sun 2020-08-16 21:19:23, Joe Perches wrote: > On Mon, 2020-08-17 at 03:37 +, 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 signature.asc Description: Digital signature
Re: [PATCH] Makefile: Yes. Finally remove '-Wdeclaration-after-statement'
Joe Perches (Sun, 16 Aug 2020 10:56:53 -0700): > On Mon, 2020-08-17 at 03:37 +, 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'
On Mon, 2020-08-17 at 03:37 +, 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'
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'
On Sun, 2020-08-16 at 16:35 +, 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'
On Sun, Aug 16, 2020 at 04:35:00PM -, 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.
