[PATCH] Drop -Wdeclaration-after-statement

[PATCH] Drop -Wdeclaration-after-statement

[Alexey Dobriyan]

Newly added static_assert() is formally a declaration, which will give
a warning if used in the middle of the function.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 Makefile |    3 ---
 1 file changed, 3 deletions(-)

--- a/Makefile
+++ b/Makefile
@@ -792,9 +792,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 += $(call cc-option,-Wvla)
 

Re: [PATCH] Drop -Wdeclaration-after-statement

[Andrew Morton]

On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> Newly added static_assert() is formally a declaration, which will give
> a warning if used in the middle of the function.
> 
> ...
>
> --- a/Makefile
> +++ b/Makefile
> @@ -792,9 +792,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 += $(call cc-option,-Wvla)

I do wish your changelogs were more elaborate :(

So the proposal is to disable -Wdeclaration-after-statement in all
cases for all time because static_assert() doesn't work correctly?

Surely there's something we can do to squish the static_assert() issue
while retaining -Wdeclaration-after-statement?  Perhaps by making
static_assert() a nop if -Wdeclaration-after-statement is in use. 
Perhaps simply by putting { } around the static_assert()?

Re: [PATCH] Drop -Wdeclaration-after-statement

[Alexey Dobriyan]

On Mon, Mar 11, 2019 at 05:38:45PM -0700, Andrew Morton wrote:
> On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > Newly added static_assert() is formally a declaration, which will give
> > a warning if used in the middle of the function.
> > 
> > ...
> >
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -792,9 +792,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 += $(call cc-option,-Wvla)
> 
> I do wish your changelogs were more elaborate :(

> So the proposal is to disable -Wdeclaration-after-statement in all
> cases for all time because static_assert() doesn't work correctly?

Yes. I converted 2 cases in /proc to static_assert() and you can't write

	{
		[code]
		static_assert()
	}

without a warning because static_assert() is declaration.
So people would move BUILD_BUG_ON() to where it doesn't belong.

> Surely there's something we can do to squish the static_assert() issue
> while retaining -Wdeclaration-after-statement?

It is not good in my opinion to stick to -Wdeclaration-after-statement.

> Perhaps by making
> static_assert() a nop if -Wdeclaration-after-statement is in use. 
> Perhaps simply by putting { } around the static_assert()?

Making a statement out of it would disable current cases where it is
placed in headers.

Re: [PATCH] Drop -Wdeclaration-after-statement

[Andrew Morton]

On Tue, 12 Mar 2019 20:24:47 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Mon, Mar 11, 2019 at 05:38:45PM -0700, Andrew Morton wrote:
> > On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > 
> > > Newly added static_assert() is formally a declaration, which will give
> > > a warning if used in the middle of the function.
> > > 
> > > ...
> > >
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -792,9 +792,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 += $(call cc-option,-Wvla)
> > 
> > I do wish your changelogs were more elaborate :(
> 
> > So the proposal is to disable -Wdeclaration-after-statement in all
> > cases for all time because static_assert() doesn't work correctly?
> 
> Yes. I converted 2 cases in /proc to static_assert() and you can't write
> 
> 	{
> 		[code]
> 		static_assert()
> 	}
> 
> without a warning because static_assert() is declaration.
> So people would move BUILD_BUG_ON() to where it doesn't belong.

Sure.

> > Surely there's something we can do to squish the static_assert() issue
> > while retaining -Wdeclaration-after-statement?
> 
> It is not good in my opinion to stick to -Wdeclaration-after-statement.

Why?

> > Perhaps by making
> > static_assert() a nop if -Wdeclaration-after-statement is in use. 
> > Perhaps simply by putting { } around the static_assert()?
> 
> Making a statement out of it would disable current cases where it is
> placed in headers.

I think you mean cases where static_assert() is used outside functions?

We could have two versions of it, one for use inside functions, one for
use outside functions?

Re: [PATCH] Drop -Wdeclaration-after-statement

[Alexey Dobriyan]

On Tue, Mar 12, 2019 at 12:50:17PM -0700, Andrew Morton wrote:
> On Tue, 12 Mar 2019 20:24:47 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > On Mon, Mar 11, 2019 at 05:38:45PM -0700, Andrew Morton wrote:
> > > On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > > 
> > > > Newly added static_assert() is formally a declaration, which will give
> > > > a warning if used in the middle of the function.
> > > > 
> > > > ...
> > > >
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -792,9 +792,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 += $(call cc-option,-Wvla)
> > > 
> > > I do wish your changelogs were more elaborate :(
> > 
> > > So the proposal is to disable -Wdeclaration-after-statement in all
> > > cases for all time because static_assert() doesn't work correctly?
> > 
> > Yes. I converted 2 cases in /proc to static_assert() and you can't write
> > 
> > 	{
> > 		[code]
> > 		static_assert()
> > 	}
> > 
> > without a warning because static_assert() is declaration.
> > So people would move BUILD_BUG_ON() to where it doesn't belong.
> 
> Sure.
> 
> > > Surely there's something we can do to squish the static_assert() issue
> > > while retaining -Wdeclaration-after-statement?
> > 
> > It is not good in my opinion to stick to -Wdeclaration-after-statement.
> 
> Why?

It is useful to have declarations mixed with code.
It reduces effective scope of a variable:

	int a;
	[a misused]
		...
	[a used correctly]

vs

	[a misused -- compile error]
		...
	int a;
	[a used correctly]

It is possible to partially workaround that but at the cost of a
indentation level. I'll post the following patch soon:

-       NEW_AUX_ENT(AT_UID, from_kuid_munged(cred->user_ns, cred->uid));
-       NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
-       NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
-       NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
+       {
+               const struct cred *cred = current_cred();
+               struct user_namespace *user_ns = cred->user_ns;
+
+               NEW_AUX_ENT(AT_UID,  from_kuid_munged(user_ns, cred->uid));
+               NEW_AUX_ENT(AT_EUID, from_kuid_munged(user_ns, cred->euid));
+               NEW_AUX_ENT(AT_GID,  from_kgid_munged(user_ns, cred->gid));
+               NEW_AUX_ENT(AT_EGID, from_kgid_munged(user_ns, cred->egid));
+       }

Often it is simply not possible to shift big function one level
deeper.

Another related thing, C99 has this very cool feature of per-for-loop
declarations:

	for (int i = 0; ...)

Once kernel will switch to C99 or C11 it _will_ be used to the point of
requiring it on the coding style level. The superstition of declaring
everything in the beginning of a function will fall, so might as well
start earlier.

Re: [PATCH] Drop -Wdeclaration-after-statement

[Alexey Dobriyan]

On Tue, Mar 12, 2019 at 12:50:17PM -0700, Andrew Morton wrote:
> On Tue, 12 Mar 2019 20:24:47 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > On Mon, Mar 11, 2019 at 05:38:45PM -0700, Andrew Morton wrote:
> > > On Sun, 10 Mar 2019 16:35:35 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > > 
> > > > Newly added static_assert() is formally a declaration, which will give
> > > > a warning if used in the middle of the function.
> > > > 
> > > > ...
> > > >
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -792,9 +792,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 += $(call cc-option,-Wvla)
> > > 
> > > I do wish your changelogs were more elaborate :(
> > 
> > > So the proposal is to disable -Wdeclaration-after-statement in all
> > > cases for all time because static_assert() doesn't work correctly?
> > 
> > Yes. I converted 2 cases in /proc to static_assert() and you can't write
> > 
> > 	{
> > 		[code]
> > 		static_assert()
> > 	}
> > 
> > without a warning because static_assert() is declaration.
> > So people would move BUILD_BUG_ON() to where it doesn't belong.
> 
> Sure.
> 
> > > Surely there's something we can do to squish the static_assert() issue
> > > while retaining -Wdeclaration-after-statement?
> > 
> > It is not good in my opinion to stick to -Wdeclaration-after-statement.
> 
> Why?
> 
> > > Perhaps by making
> > > static_assert() a nop if -Wdeclaration-after-statement is in use. 
> > > Perhaps simply by putting { } around the static_assert()?
> > 
> > Making a statement out of it would disable current cases where it is
> > placed in headers.
> 
> I think you mean cases where static_assert() is used outside functions?
> 
> We could have two versions of it, one for use inside functions, one for
> use outside functions?

I don't know. It was made a declaration in the standard specifically
to have only one name which is a good thing.

Just count the number of BUILD_BUG_* variants, it is ridiculous for such
a simple thing. Or even better, all *alloc* variants. :^)

Re: [PATCH] Drop -Wdeclaration-after-statement

[Andrew Morton]

> > > 
> > > It is not good in my opinion to stick to -Wdeclaration-after-statement.
> > 
> > Why?
> 
> It is useful to have declarations mixed with code.

Am inclined to agree.  Maybe.

> Once kernel will switch to C99 or C11 it _will_ be used to the point of
> requiring it on the coding style level. The superstition of declaring
> everything in the beginning of a function will fall, so might as well
> start earlier.

Sure, it's a matter of kernel coding conventions.

But this isn't the way to get those changed!  The process for making such
changes involves large numbers of people arguing at each other (perhaps
at kernel summit) until Linus comes in an tells everyone how it's going
to be.

Re: [PATCH] Drop -Wdeclaration-after-statement

[Andrew Morton]

On Fri, 25 Feb 2022 11:15:58 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> Putting declarations before statement is relict of single pass compiler
> era. It was necessary to allocate stack slots before generating code.
> 
> Recently added static_assert() is a declaration. -Wdeclaration-after-statement
> prevents its placement anywhere in the code for no reason.
> 
> Placing variable declarations in the beginning of the block increases
> variable "LOC lifetime" so to speak and chances that it will be misused.
> This is very low probability bug but still. Declaring variables right
> before first use will make "LOC lifetime" smaller.
> 
> 	{
> 		int x;
> 		[x is misused due to a typo]
> 		f(x);	// first correct use
> 	}
> 
> vs
> 
> 	{
> 		[can't misuse undeclared variable]
> 		int x;
> 		f(x);	// first correct use
> 	}
> 

Oh man.  I so wish you'd cc'ed Linus on this one so we could admire his
reaction.

Re: [PATCH] Drop -Wdeclaration-after-statement

[Alexey Dobriyan]

On Fri, Feb 25, 2022 at 10:01:23AM +0000, David Laight wrote:
> From: Alexey Dobriyan
> > Sent: 25 February 2022 08:16
> > 
> > Putting declarations before statement is relict of single pass compiler
> > era. It was necessary to allocate stack slots before generating code.
> > 
> > Recently added static_assert() is a declaration. -Wdeclaration-after-statement
> > prevents its placement anywhere in the code for no reason.
> 
> That could enclose its declaration inside a block.

It could but why put useless characters on the screen?

> But then it wouldn't be usable at global scope (is it anyway?)

It is usable in global scope.

> > Placing variable declarations in the beginning of the block increases
> > variable "LOC lifetime" so to speak and chances that it will be misused.
> > This is very low probability bug but still. Declaring variables right
> > before first use will make "LOC lifetime" smaller.
> 
> NAK it makes it very hard for a human (some of us are) to find
> the declaration.

What?

Which editor are you using?

Shift+3 with selection highlighting in vim works just fine.
I don't believe Emacs doesn't have an equivalent.

> Indeed putting them anywhere other that the top of a function
> or the top of a very short code block makes them hard to find.

RE: [PATCH] Drop -Wdeclaration-after-statement

[David Laight]

From: Alexey Dobriyan
> Sent: 25 February 2022 08:16
> 
> Putting declarations before statement is relict of single pass compiler
> era. It was necessary to allocate stack slots before generating code.
> 
> Recently added static_assert() is a declaration. -Wdeclaration-after-statement
> prevents its placement anywhere in the code for no reason.

That could enclose its declaration inside a block.
But then it wouldn't be usable at global scope (is it anyway?)

> Placing variable declarations in the beginning of the block increases
> variable "LOC lifetime" so to speak and chances that it will be misused.
> This is very low probability bug but still. Declaring variables right
> before first use will make "LOC lifetime" smaller.

NAK it makes it very hard for a human (some of us are) to find
the declaration.

Indeed putting them anywhere other that the top of a function
or the top of a very short code block makes them hard to find.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH] Drop -Wdeclaration-after-statement

[Alexey Dobriyan]

Putting declarations before statement is relict of single pass compiler
era. It was necessary to allocate stack slots before generating code.

Recently added static_assert() is a declaration. -Wdeclaration-after-statement
prevents its placement anywhere in the code for no reason.

Placing variable declarations in the beginning of the block increases
variable "LOC lifetime" so to speak and chances that it will be misused.
This is very low probability bug but still. Declaring variables right
before first use will make "LOC lifetime" smaller.

	{
		int x;
		[x is misused due to a typo]
		f(x);	// first correct use
	}

vs

	{
		[can't misuse undeclared variable]
		int x;
		f(x);	// first correct use
	}

Signed-off-by: Alexey Dobriyan <adobriyan@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(-)

--- a/Makefile
+++ b/Makefile
@@ -933,9 +933,6 @@ endif
 # arch Makefile may override CC so keep this after arch Makefile is included
 NOSTDINC_FLAGS += -nostdinc
 
-# 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
 
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -71,7 +71,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 += -fno-strict-overflow
 VDSO_CFLAGS += $(call cc32-option,-Werror=strict-prototypes)
--- a/tools/power/acpi/Makefile.config
+++ b/tools/power/acpi/Makefile.config
@@ -63,7 +63,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
--- 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)\" \
--- 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