[PATCH] Revert "Enable '-Werror' by default for all kernel builds"

This reverts commit 3fe617ccafd6f5bb33c2391d6f4eeb41c1fd0151.

The above commit seems as though it was merged in response to
https://lore.kernel.org/linux-hardening/CAHk-=wj4EG=kCOaqyPEq5VXa97kyUHsBpBn3DWwE91qcnDytOQ@mail.gmail.com/.

While I can appreciate the intent of enabling -Werror, I don't think it
is the right tool to address the root cause of developers not testing
certain toolchains or configurations, or taking existing reports they're
getting serious enough.

Having more appropriate CI or processes in place to prevent untested
patches from being merged into mainline may also be worth discussing.

I'd also like to see such a patch sent formally to the list for
discussion and have time to soak in next rather than be merged directly
into mainline without either.

-Werror is great for preventing new errors from creeping in when a
codebase is free of warnings for all configs and all targets and the
toolchain is never updated. Unfortunately, none of the above is the case
for the Linux kernel at this time.

The addition of new compiler diagnostic flags in the -W group to -Wall
make toolchain updates excessively more painful. This can lead to
commits that disable warnings rather than work towards addressing them.
Some diagnostics are useful but take incredible work or churn to
completely free a codebase from them.

Warning can be upgraded to errors with -Werror=foo or downgraded from
errors back to warnings via -Wno-error=foo. -Wno-error=foo is a double
edged sword; it doesn't help you spot the introduction of additional
instances of that warning easily.

This change has caused nearly all of our CI to go red, and requires us
to now disable CONFIG_WERROR until every last target and every last
config is addressed. Rather than require everyone to disable the above
config to keep builds going, perhaps certain CI systems should instead
set CFLAGS_KERNEL=-Werror.

Why don't we just fix every warning? We have been, for years, and we're
still not done yet. See our issue tracker below, contributors wanted.

With more time/active discussion, we can probably land something more
appropriate. It should involve the Kbuild maintainer and list.

For instance, I have questions around how should such a config interact
with randconfigs and allconfigs. This config also seems to duplicate the
existing CONFIG_PPC_DISABLE_WERROR without merging the two.

I do recognize the irony of someone who's spent a lot of time cleaning
up warnings to be advocating for disabling -Werror...it's not lost on
me. Our Pixel (née Nexus) team has been effectively carrying an out of
tree patch enabling -Werror since before I ever contributed to the
kernel.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Kees Cook <redacted>
Cc: Mark Brown <broonie@kernel.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Nathan Chancellor <nathan@kernel.org>
Link: https://github.com/ClangBuiltLinux/linux/issues/1449
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 Makefile     |  3 ---
 init/Kconfig | 14 --------------
 2 files changed, 17 deletions(-)

diff --git a/Makefile b/Makefile
index d45fc2edf186..6bc1c5b17a62 100644
--- a/Makefile
+++ b/Makefile

@@ -785,9 +785,6 @@ stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG)      := -fstack-protector-strong
 
 KBUILD_CFLAGS += $(stackp-flags-y)
 
-KBUILD_CFLAGS-$(CONFIG_WERROR) += -Werror
-KBUILD_CFLAGS += $(KBUILD_CFLAGS-y)
-
 ifdef CONFIG_CC_IS_CLANG
 KBUILD_CPPFLAGS += -Qunused-arguments
 # The kernel builds with '-std=gnu89' so use of GNU extensions is acceptable.

diff --git a/init/Kconfig b/init/Kconfig
index 8cb97f141b70..e708180e9a59 100644
--- a/init/Kconfig
+++ b/init/Kconfig

@@ -137,20 +137,6 @@ config COMPILE_TEST
 	  here. If you are a user/distributor, say N here to exclude useless
 	  drivers to be distributed.
 
-config WERROR
-	bool "Compile the kernel with warnings as errors"
-	default y
-	help
-	  A kernel build should not cause any compiler warnings, and this
-	  enables the '-Werror' flag to enforce that rule by default.
-
-	  However, if you have a new (or very old) compiler with odd and
-	  unusual warnings, or you have some architecture with problems,
-	  you may need to disable this config option in order to
-	  successfully build the kernel.
-
-	  If in doubt, say Y.
-
 config UAPI_HEADER_TEST
 	bool "Compile test UAPI headers"
 	depends on HEADERS_INSTALL && CC_CAN_LINK

base-commit: 4b93c544e90e2b28326182d31ee008eb80e02074
-- 
2.33.0.153.gba50c8fa24-goog

On Tue, Sep 7, 2021 at 11:39 AM Nick Desaulniers
[off-list ref] wrote:

This reverts commit 3fe617ccafd6f5bb33c2391d6f4eeb41c1fd0151.

The above commit seems as though it was merged in response to
https://lore.kernel.org/linux-hardening/CAHk-=wj4EG=kCOaqyPEq5VXa97kyUHsBpBn3DWwE91qcnDytOQ@mail.gmail.com/.

While I can appreciate the intent of enabling -Werror, I don't think it
is the right tool to address the root cause of developers not testing
certain toolchains or configurations, or taking existing reports they're
getting serious enough.

Having more appropriate CI or processes in place to prevent untested
patches from being merged into mainline may also be worth discussing.

I agree that -Werror by default needs more discussion.
Default WERROR makes building old kernels with new compilers more painful.

CI systems could do a better job surfacing compiler warnings if they
don't do it currently.

-- 
宋方睿

On Tue, Sep 7, 2021 at 11:39 AM Nick Desaulniers
[off-list ref] wrote:

The above commit seems as though it was merged in response to
https://lore.kernel.org/linux-hardening/CAHk-=wj4EG=kCOaqyPEq5VXa97kyUHsBpBn3DWwE91qcnDytOQ@mail.gmail.com/.

No. It was merged in response of _years_ of pain, with the last one
just being the final drop.

I'm not going to revert that change. I probably will have to limit it
(by making that WERROR option depend on certain expectations), but
basically any maintainer who has code that causes warnings should
expect that they will have to fix those warnings.

If it's clang that generates bogus warnings, then we'll have to start
disable clang warnings. The clang people tend to be proud of thir
fewer false positives, but so far looking at things, I am not
convinced.

And I'm most definitely not convinced when the "let's finally enable
-Werror after years of talking about it", people end up going "but but
but I have thousands of warnings".

That's the POINT of that commit. That "but but but I have thousands of
warnings" is not acceptable.

I spent hours yesterday getting rid of some warnings. It shouldn't be
on me fixing peoples code. It shouldn't be on me noticing that people
send me crap that warns.

And it really shouldn't be "Linus cares about warnings, so
configurations that Linus doesn't test can continue for years to have
them".

My "no warnings" policy isn't exactly new, and people shouldn't be
shocked when I then say "time to clean up *YOUR* house too".

            Linus

On Tue, Sep 7, 2021 at 12:16 PM Linus Torvalds
[off-list ref] wrote:

On Tue, Sep 7, 2021 at 11:39 AM Nick Desaulniers
[off-list ref] wrote:

quoted

The above commit seems as though it was merged in response to
https://lore.kernel.org/linux-hardening/CAHk-=wj4EG=kCOaqyPEq5VXa97kyUHsBpBn3DWwE91qcnDytOQ@mail.gmail.com/.

No. It was merged in response of _years_ of pain, with the last one
just being the final drop.

Well, I think it's important to enumerate some of the pain points
explicitly so that we might all better understand what they are, and
come to an agreement together on what the right way to resolve them
is.

Impulse commits without discussion over a holiday weekend are firmly
in the category of "I wish you'd rather not have done that."

Let's look closer at the final drop.  I think there's a bigger question here:
1. did the CI have enough time to run in multiple configurations with
multiple toolchains to even find a warning, let alone report it?
2. what did the developer do with the report (if they even got it, see
1 above)? Ignore it? Proceed with the PR anyways?

it looks to me as though the "final drop" fits into 1 above. I'm not
sure that -Werror is the correct way to resolve issues from that.

I'm not going to revert that change. I probably will have to limit it
(by making that WERROR option depend on certain expectations), but
basically any maintainer who has code that causes warnings should
expect that they will have to fix those warnings.

I'm not 100% against it; I think it could land in a more useful
variation. But it would be good to discuss that on-list, and give it
time to soak. This is a conversation we should be having with CI
maintainers IMO, and folks that maintain the build infra, at least.
I'm happy to kick off that discussion with this RFC.

If it's clang that generates bogus warnings, then we'll have to start
disable clang warnings. The clang people tend to be proud of thir
fewer false positives, but so far looking at things, I am not
convinced.

Now you're just attacking a strawman.

And I'm most definitely not convinced when the "let's finally enable
-Werror after years of talking about it", people end up going "but but
but I have thousands of warnings".

The kernel is full of thousands of warnings at the moment.  It might
not be for your limited set of configs, targets, and toolchains, but
the folks running CI are very very well aware that the kernel is far
from enabling -Werror seriously.

Any given maintainer sending you a PR cannot (and should not, IMO)
know under what combination of configs, targets, and toolchains you'll
test under, and -Werror isn't going to help them figure it out.

Not every commit that makes its way to mainline has spent equal
amounts of soak time in -next. (I suspect there are commits going into
mainline that spent zero time in -next, not just from you).  Patches
merged by maintainers shortly before the merge window open have
minimal coverage compared to older commits they've accepted.  So CI
systems can't find diagnostics (warnings/errors) in various
combinations of config/target/toolchain if patches are skipping -next
or spending a short amount of time in -next.

To be clear, you have merged patches into mainline that broke the
build for combinations of configs/targets/toolchains that you are not
testing.  It's not realistic for you or any one person to test all
such combinations either.  Other software projects have solved this by
relying on bots to handle merges. Bots that don't forget to test
combinations of various targets, or enable developers to ignore
warnings (if the warnings are even reported).

That's the POINT of that commit. That "but but but I have thousands of
warnings" is not acceptable.

I spent hours yesterday getting rid of some warnings. It shouldn't be
on me fixing peoples code. It shouldn't be on me noticing that people
send me crap that warns.

I agree. I disagree that -Werror will solve that.  Developers will get
the same report from 0day bot, just now as an error rather than
warning, and they will continue to ignore the report because "well it
works for me."  (Maybe I'm attacking the strawman now).

And it really shouldn't be "Linus cares about warnings, so
configurations that Linus doesn't test can continue for years to have
them".

I agree, and we have been making progress on this. I'd say the advent
of the various CI systems over the past 5 years is a much welcomed
improvement in at least understanding which combinations are exposing
issues.

My "no warnings" policy isn't exactly new, and people shouldn't be
shocked when I then say "time to clean up *YOUR* house too".

We have been cleaning house, for nearly half a decade in my
experience. Fixing warnings in the Linux kernel for all possible
configs, targets, and toolchains while the toolchains continue to add
more diagnostics is more sisyphean than digging a hole in wet sand.
-- 
Thanks,
~Nick Desaulniers

On Tue, Sep 07, 2021 at 01:30PM -0700, Nick Desaulniers wrote:

On Tue, Sep 7, 2021 at 12:16 PM Linus Torvalds [off-list ref] wrote:

[...]

quoted

I'm not going to revert that change. I probably will have to limit it
(by making that WERROR option depend on certain expectations), but
basically any maintainer who has code that causes warnings should
expect that they will have to fix those warnings.

I'm not 100% against it; I think it could land in a more useful
variation. But it would be good to discuss that on-list, and give it
time to soak. This is a conversation we should be having with CI
maintainers IMO, and folks that maintain the build infra, at least.
I'm happy to kick off that discussion with this RFC.

Here's a datapoint: I had to disable CONFIG_WERROR on a bunch of syzbot
instances which started failing because of -Werror [1], because syzbot's
time is better spent on fuzzing, and having the odd warning in some
subsystem penalize fuzzing of the entire kernel is not appropriate.

[1] https://github.com/google/syzkaller/commit/e096c0a2a414e487412c9669426780ce5acdde9d

Getting the kernel built is a hard requirement for any sort of runtime
testing. Once the kernel is built, runtime testing of various subsystems
can proceed in parallel. A single warning in some odd subsystem
penalizing the _entire_ kernel's testing progress is inappropriate. The
severity of a use-after-free bug found by runtime testing is orders of
magnitude more severe than some "unused variable" warning. Now such a
warning would delay finding bugs at runtime on many CI systems that
haven't yet disabled the warning.

I'm predicting most distributions and runtime-focused CIs will disable
the warning.

I have formulated this in the form of a patch below, that might move
this new Kconfig option towards its appropriate usecases by default.

The intent is not to dispute the usefulness of -Werror, but select the
appropriate usecases by default, limiting friction for all those who can
do little more than say CONFIG_WERROR=n.

Thanks,
-- Marco

------ >8 ------

From: Marco Elver <elver@google.com>
Date: Tue, 7 Sep 2021 23:12:08 +0200
Subject: [PATCH] kbuild: Only default to -Werror if COMPILE_TEST

The cross-product of the kernel's supported toolchains, architectures,
and configuration options is large. So large, that it's generally
accepted to be infeasible to enumerate and build+test them all
(many compile-testers rely on randomly generated configs).

Without the possibility to enumerate all possible combinations of
toolchains, architectures, and configuration options, it is inevitable
that compiler warnings in this space exist.

With -Werror, this means that an innumerable set of kernels are now
broken, yet had been perfectly usable before (confused compilers, code
with warnings unused, or luck).

Distributors will necessarily pick a point in the toolchain X arch X
config space, and if unlucky, will have a broken build. Granted, those
will likely disable CONFIG_WERROR and move on.

The kernel's default configuration is unlikely to be suitable for all
users, but it's inappropriate to force many users to set CONFIG_WERROR=n.

This also holds for CI systems which are focused on runtime testing,
where the odd warning in some subsystem will disrupt testing of the rest
of the kernel. Many of those runtime-focused CI systems run tests or
fuzz the kernel using runtime debugging tools. Runtime testing of
different subsystems can proceed in parallel, and potentially uncover
serious bugs; halting runtime testing of the entire kernel because of
the odd warning (now error) in a subsystem or driver is simply
inappropriate.

Therefore, runtime-focused CI systems will likely choose CONFIG_WERROR=n
as well.

The appropriate usecase for -Werror is therefore compile-test focused
builds (often done by developers or CI systems).

Reflect this in the Kconfig option by making the default value of WERROR
match COMPILE_TEST.

Signed-off-by: Marco Elver <elver@google.com>
---
 init/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/init/Kconfig b/init/Kconfig
index 8cb97f141b70..11f8a845f259 100644
--- a/init/Kconfig
+++ b/init/Kconfig

@@ -139,7 +139,7 @@ config COMPILE_TEST
 
 config WERROR
 	bool "Compile the kernel with warnings as errors"
-	default y
+	default COMPILE_TEST
 	help
 	  A kernel build should not cause any compiler warnings, and this
 	  enables the '-Werror' flag to enforce that rule by default.

-- 
2.33.0.153.gba50c8fa24-goog

On Tue, Sep 7, 2021 at 3:14 PM Marco Elver [off-list ref] wrote:

That seems reasonable. It very much is about build-testing.

              Linus

On 9/7/21 3:14 PM, Marco Elver wrote:

On Tue, Sep 07, 2021 at 01:30PM -0700, Nick Desaulniers wrote:

quoted

On Tue, Sep 7, 2021 at 12:16 PM Linus Torvalds [off-list ref] wrote:

[...]

quoted

quoted

I'm not going to revert that change. I probably will have to limit it
(by making that WERROR option depend on certain expectations), but
basically any maintainer who has code that causes warnings should
expect that they will have to fix those warnings.

I'm not 100% against it; I think it could land in a more useful
variation. But it would be good to discuss that on-list, and give it
time to soak. This is a conversation we should be having with CI
maintainers IMO, and folks that maintain the build infra, at least.
I'm happy to kick off that discussion with this RFC.

Here's a datapoint: I had to disable CONFIG_WERROR on a bunch of syzbot
instances which started failing because of -Werror [1], because syzbot's
time is better spent on fuzzing, and having the odd warning in some
subsystem penalize fuzzing of the entire kernel is not appropriate.

[1] https://github.com/google/syzkaller/commit/e096c0a2a414e487412c9669426780ce5acdde9d

Getting the kernel built is a hard requirement for any sort of runtime
testing. Once the kernel is built, runtime testing of various subsystems
can proceed in parallel. A single warning in some odd subsystem
penalizing the _entire_ kernel's testing progress is inappropriate. The
severity of a use-after-free bug found by runtime testing is orders of
magnitude more severe than some "unused variable" warning. Now such a
warning would delay finding bugs at runtime on many CI systems that
haven't yet disabled the warning.

I'm predicting most distributions and runtime-focused CIs will disable
the warning.

I have formulated this in the form of a patch below, that might move
this new Kconfig option towards its appropriate usecases by default.

The intent is not to dispute the usefulness of -Werror, but select the
appropriate usecases by default, limiting friction for all those who can
do little more than say CONFIG_WERROR=n.

Thanks,
-- Marco

------ >8 ------

From: Marco Elver <elver@google.com>
Date: Tue, 7 Sep 2021 23:12:08 +0200
Subject: [PATCH] kbuild: Only default to -Werror if COMPILE_TEST

The cross-product of the kernel's supported toolchains, architectures,
and configuration options is large. So large, that it's generally
accepted to be infeasible to enumerate and build+test them all
(many compile-testers rely on randomly generated configs).

Without the possibility to enumerate all possible combinations of
toolchains, architectures, and configuration options, it is inevitable
that compiler warnings in this space exist.

With -Werror, this means that an innumerable set of kernels are now
broken, yet had been perfectly usable before (confused compilers, code
with warnings unused, or luck).

Distributors will necessarily pick a point in the toolchain X arch X
config space, and if unlucky, will have a broken build. Granted, those
will likely disable CONFIG_WERROR and move on.

The kernel's default configuration is unlikely to be suitable for all
users, but it's inappropriate to force many users to set CONFIG_WERROR=n.

This also holds for CI systems which are focused on runtime testing,
where the odd warning in some subsystem will disrupt testing of the rest
of the kernel. Many of those runtime-focused CI systems run tests or
fuzz the kernel using runtime debugging tools. Runtime testing of
different subsystems can proceed in parallel, and potentially uncover
serious bugs; halting runtime testing of the entire kernel because of
the odd warning (now error) in a subsystem or driver is simply
inappropriate.

Therefore, runtime-focused CI systems will likely choose CONFIG_WERROR=n
as well.

The appropriate usecase for -Werror is therefore compile-test focused
builds (often done by developers or CI systems).

Reflect this in the Kconfig option by making the default value of WERROR
match COMPILE_TEST.

Signed-off-by: Marco Elver <elver@google.com>

I like it.

Acked-by: Guenter Roeck <linux@roeck-us.net>

On 9/7/21 3:18 PM, Linus Torvalds wrote:

On Tue, Sep 7, 2021 at 3:14 PM Marco Elver [off-list ref] wrote:

quoted

  config WERROR
         bool "Compile the kernel with warnings as errors"
-       default y
+       default COMPILE_TEST

That seems reasonable. It very much is about build-testing.

That and 2 more things IMO:

a. having developers be responsible for build warnings, not just
   build errors

b. having maintainers merge them more like they are build errors
   and not just some warnings that can be overlooked.

I don't see enough of a. or b.  :(

-- 
~Randy

On Wed, Sep 08, 2021 at 12:14:19AM +0200, Marco Elver wrote:

Here's a datapoint: I had to disable CONFIG_WERROR on a bunch of syzbot
instances which started failing because of -Werror [1], because syzbot's
time is better spent on fuzzing, and having the odd warning in some
subsystem penalize fuzzing of the entire kernel is not appropriate.

Similarly, I have to disable -Werror (which various archs and subsystems
already use) whenever I test building the kernel with new toolchains.
It is the biggest set of kernel patches I keep, already, since many
years.

I actually have good hopes that a centralised -Werror thing will make
this easier :-)

Maybe there can be an E=[01] kernel build flag to disable / enable
CONFIG_WERROR?  Something that will override it for just that command.
This would make life easier for many use cases, while at the same time
not being something that people can "forget" they did.


Segher

On Wed, Sep 08, 2021 at 12:14:19AM +0200, Marco Elver wrote:

I'm predicting most distributions and runtime-focused CIs will disable
the warning.

Yes, indeed.

Date: Tue, 7 Sep 2021 23:12:08 +0200
Subject: [PATCH] kbuild: Only default to -Werror if COMPILE_TEST

The cross-product of the kernel's supported toolchains, architectures,
and configuration options is large. So large, that it's generally
accepted to be infeasible to enumerate and build+test them all
(many compile-testers rely on randomly generated configs).

Reviwed-by: Mark Brown [off-list ref]

On Wed, Sep 08, 2021 at 12:14:19AM +0200, Marco Elver wrote:

On Tue, Sep 07, 2021 at 01:30PM -0700, Nick Desaulniers wrote:

quoted

On Tue, Sep 7, 2021 at 12:16 PM Linus Torvalds [off-list ref] wrote:

[...]

quoted

quoted

I'm not going to revert that change. I probably will have to limit it
(by making that WERROR option depend on certain expectations), but
basically any maintainer who has code that causes warnings should
expect that they will have to fix those warnings.

I'm not 100% against it; I think it could land in a more useful
variation. But it would be good to discuss that on-list, and give it
time to soak. This is a conversation we should be having with CI
maintainers IMO, and folks that maintain the build infra, at least.
I'm happy to kick off that discussion with this RFC.

Here's a datapoint: I had to disable CONFIG_WERROR on a bunch of syzbot
instances which started failing because of -Werror [1], because syzbot's
time is better spent on fuzzing, and having the odd warning in some
subsystem penalize fuzzing of the entire kernel is not appropriate.

[1] https://github.com/google/syzkaller/commit/e096c0a2a414e487412c9669426780ce5acdde9d

Getting the kernel built is a hard requirement for any sort of runtime
testing. Once the kernel is built, runtime testing of various subsystems
can proceed in parallel. A single warning in some odd subsystem
penalizing the _entire_ kernel's testing progress is inappropriate. The
severity of a use-after-free bug found by runtime testing is orders of
magnitude more severe than some "unused variable" warning. Now such a
warning would delay finding bugs at runtime on many CI systems that
haven't yet disabled the warning.

I'm predicting most distributions and runtime-focused CIs will disable
the warning.

I have formulated this in the form of a patch below, that might move
this new Kconfig option towards its appropriate usecases by default.

The intent is not to dispute the usefulness of -Werror, but select the
appropriate usecases by default, limiting friction for all those who can
do little more than say CONFIG_WERROR=n.

Thanks,
-- Marco

------ >8 ------

From: Marco Elver <elver@google.com>
Date: Tue, 7 Sep 2021 23:12:08 +0200
Subject: [PATCH] kbuild: Only default to -Werror if COMPILE_TEST

The cross-product of the kernel's supported toolchains, architectures,
and configuration options is large. So large, that it's generally
accepted to be infeasible to enumerate and build+test them all
(many compile-testers rely on randomly generated configs).

Without the possibility to enumerate all possible combinations of
toolchains, architectures, and configuration options, it is inevitable
that compiler warnings in this space exist.

With -Werror, this means that an innumerable set of kernels are now
broken, yet had been perfectly usable before (confused compilers, code
with warnings unused, or luck).

Distributors will necessarily pick a point in the toolchain X arch X
config space, and if unlucky, will have a broken build. Granted, those
will likely disable CONFIG_WERROR and move on.

The kernel's default configuration is unlikely to be suitable for all
users, but it's inappropriate to force many users to set CONFIG_WERROR=n.

This also holds for CI systems which are focused on runtime testing,
where the odd warning in some subsystem will disrupt testing of the rest
of the kernel. Many of those runtime-focused CI systems run tests or
fuzz the kernel using runtime debugging tools. Runtime testing of
different subsystems can proceed in parallel, and potentially uncover
serious bugs; halting runtime testing of the entire kernel because of
the odd warning (now error) in a subsystem or driver is simply
inappropriate.

Therefore, runtime-focused CI systems will likely choose CONFIG_WERROR=n
as well.

The appropriate usecase for -Werror is therefore compile-test focused
builds (often done by developers or CI systems).

Reflect this in the Kconfig option by making the default value of WERROR
match COMPILE_TEST.

Signed-off-by: Marco Elver <elver@google.com>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

On Tue, Sep 07, 2021 at 12:16:22PM -0700, Linus Torvalds wrote:

On Tue, Sep 7, 2021 at 11:39 AM Nick Desaulniers
[off-list ref] wrote:

quoted

The above commit seems as though it was merged in response to
https://lore.kernel.org/linux-hardening/CAHk-=wj4EG=kCOaqyPEq5VXa97kyUHsBpBn3DWwE91qcnDytOQ@mail.gmail.com/.

No. It was merged in response of _years_ of pain, with the last one
just being the final drop.

I'm not going to revert that change. I probably will have to limit it
(by making that WERROR option depend on certain expectations), but
basically any maintainer who has code that causes warnings should
expect that they will have to fix those warnings.

Echoing what others have said about runtime testing having -Werror on by
default for defconfigs is going to cause issues for bisection and just
generally noticing promptly when runtime issues are introduced - people
or systems bisecting boot or testsuite issues will encounter more blocks
of commits that they skip due to unrelated issues, and it's much more
likely that we'll just have gaps in coverage when for example the day's
-next has some random warning on some platforms.  In terms of
prioritisation it feels like the wrong call to have -Werror on
everywhere.

The bisection really is helpful, especially when the people looking at
the problem don't have direct access the systems to be able to run tests
themselves - it really increases the quality of reports when the
automation in CI services is able to identify the likely commit, and
makes it *much* more likely those reports be sent in the first place.

Of course this isn't insurmountable, the test systems can always add
config fragments to tweak the configurations they're testing (they
already need to do this to run lots of kselftest for example) but that's
something we've generally tried to minimise since everyone working with
custom configs has generally caused friction in the past.

And it really shouldn't be "Linus cares about warnings, so
configurations that Linus doesn't test can continue for years to have
them".

My "no warnings" policy isn't exactly new, and people shouldn't be
shocked when I then say "time to clean up *YOUR* house too".

IME pushing on this stuff it's not that people don't care, it's that
they naturally have different test coverage due to their particular
interests and the resources available to them.  Even people who are very
dilligent in their own testing and paying a lot of attention to -next
and the automated test stuff that's out there are going to introduce
some breakage from time to time, and sometimes there's good reasons for
processes to get short circuited.

Some of this is also on the people doing the more niche stuff to keep on
top of reporting issues that come up as they do so, they should be able
to expect that people will pay attention when they do so but the more
niche you get the less surprising it is when nobody else notices issues.

On Tue, Sep 07, 2021 at 12:16:22PM -0700, Linus Torvalds wrote:

That's the POINT of that commit. That "but but but I have thousands of
warnings" is not acceptable.

I'm actually surprised you did this after the discussion with gcc warning
about using "main" as a local variable.

My "no warnings" policy isn't exactly new, and people shouldn't be
shocked when I then say "time to clean up *YOUR* house too".

Note, ktest has a way to create a list of current warnings, and then test your
code against it, and will fail on any new warning. I run this on all my pull
requests to make sure that I do not introduce any new warnings.

That said, I still get bug reports on various configs that I did not test,
where my code introduces a warning. I hate to be the one that fails their
builds.

It's not the configs that have something enabled, its the configs that have
something not enabled, where another config depends on it.

I'm not against the -Werror. I just don't want to be changing local variables
called "main" because it breaks someones build due to some daft warning that
the compiler is emitting.

-- Steve

Hi!

quoted

quoted

 config WERROR
        bool "Compile the kernel with warnings as errors"
-       default y
+       default COMPILE_TEST

That seems reasonable. It very much is about build-testing.

That and 2 more things IMO:

a. having developers be responsible for build warnings, not just
   build errors

b. having maintainers merge them more like they are build errors
   and not just some warnings that can be overlooked.

I don't see enough of a. or b.  :(

Do we really want developers treat warnings as errors? When the code
is okay but some random version of gcc dislikes it...

Plus, there's question of stable. We already get ton of churn there
("this fixes random warning"). WERROR will only encourage that...

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

On Mon, Sep 13, 2021 at 11:32:56AM +0200, Pavel Machek wrote:

Hi!

quoted

quoted

quoted

 config WERROR
        bool "Compile the kernel with warnings as errors"
-       default y
+       default COMPILE_TEST

That seems reasonable. It very much is about build-testing.

That and 2 more things IMO:

a. having developers be responsible for build warnings, not just
   build errors

b. having maintainers merge them more like they are build errors
   and not just some warnings that can be overlooked.

I don't see enough of a. or b.  :(

Do we really want developers treat warnings as errors? When the code
is okay but some random version of gcc dislikes it...

Plus, there's question of stable. We already get ton of churn there
("this fixes random warning"). WERROR will only encourage that...

I will not be backporting this patch to older stable kernels, but I
_want_ to see stable builds build with no warnings.  When we add
warnings, they are almost always things we need to fix up properly.

Over time, I have worked to reduce the number of build warnings in older
stable kernels.  For newer versions of gcc, sometimes that is
impossible, but we are close...

thanks,

greg k-h

Hi!

quoted

Do we really want developers treat warnings as errors? When the code
is okay but some random version of gcc dislikes it...

Plus, there's question of stable. We already get ton of churn there
("this fixes random warning"). WERROR will only encourage that...

I will not be backporting this patch to older stable kernels, but I
_want_ to see stable builds build with no warnings.  When we add
warnings, they are almost always things we need to fix up properly.

Well, everyone _wants_ to see clean builds... unless the price is too
high.

Over time, I have worked to reduce the number of build warnings in older
stable kernels.  For newer versions of gcc, sometimes that is
impossible, but we are close...

You clearly can't backport this patch, but for 5.16-stable, you'll
have it in, and now warnings are same as errors... and I don't believe
that's good idea for stable.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

On Mon, Sep 13, 2021 at 12:02:30PM +0200, Pavel Machek wrote:

Hi!

quoted

quoted

Do we really want developers treat warnings as errors? When the code
is okay but some random version of gcc dislikes it...

Plus, there's question of stable. We already get ton of churn there
("this fixes random warning"). WERROR will only encourage that...

I will not be backporting this patch to older stable kernels, but I
_want_ to see stable builds build with no warnings.  When we add
warnings, they are almost always things we need to fix up properly.

Well, everyone _wants_ to see clean builds... unless the price is too
high.

quoted

Over time, I have worked to reduce the number of build warnings in older
stable kernels.  For newer versions of gcc, sometimes that is
impossible, but we are close...

You clearly can't backport this patch, but for 5.16-stable, you'll
have it in, and now warnings are same as errors... and I don't believe
that's good idea for stable.

I do, it will force us to keep these trees clean over time.

And it will be in 5.15, not 5.16 :)

Worst case, we disable it in 4 years when gcc 15 or so generates so
many errors we can't resolve them in this old kernel.

thanks,

greg k-h

On 9/13/21 2:32 AM, Pavel Machek wrote:

Hi!

quoted

quoted

quoted

  config WERROR
         bool "Compile the kernel with warnings as errors"
-       default y
+       default COMPILE_TEST

That seems reasonable. It very much is about build-testing.

That and 2 more things IMO:

a. having developers be responsible for build warnings, not just
    build errors

b. having maintainers merge them more like they are build errors
    and not just some warnings that can be overlooked.

I don't see enough of a. or b.  :(

Do we really want developers treat warnings as errors? When the code
is okay but some random version of gcc dislikes it...

Plus, there's question of stable. We already get ton of churn there
("this fixes random warning"). WERROR will only encourage that...

All Chrome OS builds are already done with -Werror enabled. Having it
enabled in the incoming stable releases will reduce our workload when
backporting stable releases. I am actually working on making at
least chromeos-5.10 "clean" for allmodconfig builds on arm, arm64,
and x86 (everything else is hopeless, and even arm may be futile,
but arm64 and x86 seem to be doable).

I'd rather have warnings fixed in incoming stable releases than having
to pull additional patches into our kernels.

Guenter

On Mon, Sep 13, 2021 at 12:50 PM Pavel Machek [off-list ref] wrote:

quoted

quoted

Do we really want developers treat warnings as errors? When the code
is okay but some random version of gcc dislikes it...

Plus, there's question of stable. We already get ton of churn there
("this fixes random warning"). WERROR will only encourage that...

I will not be backporting this patch to older stable kernels, but I
_want_ to see stable builds build with no warnings.  When we add
warnings, they are almost always things we need to fix up properly.

Well, everyone _wants_ to see clean builds... unless the price is too
high.

quoted

Over time, I have worked to reduce the number of build warnings in older
stable kernels.  For newer versions of gcc, sometimes that is
impossible, but we are close...

You clearly can't backport this patch, but for 5.16-stable, you'll
have it in, and now warnings are same as errors... and I don't believe
that's good idea for stable.

The good thing about the config option is that there's now a single point
to enable or disable -Werror.  In the past, maintainers sprinkled -Werror
all over the various Makefiles in the tree, which means you have to
edit multiple files to disable it again.

Background: I've been investigating an issue that involved building old
v2.6.x kernels. Apart from having to use very old compilers, it still caused
compiler warnings that obviously weren't seen with the slightly different
compiler versions used by maintainers who added -Werror.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds