[PATCH] kbuild: Check for unknown options with cc-option and clang in Kbuild

[PATCH] kbuild: Check for unknown options with cc-option and clang in Kbuild

[Stephen Boyd]

If the particular version of clang a user has doesn't enable
-Werror=unknown-warning-option by default, even though it is the
default[1], then make sure to pass the option to the Kconfig cc-option
command so that testing options from Kconfig files works properly.
Otherwise, depending on the default values setup in the clang toolchain
we will silently assume options such as -Wmaybe-uninitialized are
supported by clang, when they really aren't.

This issue only started happening for me once commit 589834b3a009
("kbuild: Add -Werror=unknown-warning-option to CLANG_FLAGS") was
applied on top of commit b303c6df80c9 ("kbuild: compute false-positive
-Wmaybe-uninitialized cases in Kconfig"). This leads kbuild to try and
test for the existence of the -Wmaybe-uninitialized flag with the
cc-option command in scripts/Kconfig.include, and it doesn't see an
error returned from the option test so it sets the config value to Y.
Then the makefile tries to pass the unknown option on the command line
and -Werror=unknown-warning-option catches the invalid option and breaks
the build.

Note: this doesn't change the cc-option tests in Makefiles, because
those use a different rule that includes KBUILD_CFLAGS by default, and
the KBUILD_CFLAGS already has -Werror=unknown-warning-option. Thanks to
Doug for pointing out the different rule.

[1] https://clang.llvm.org/docs/DiagnosticsReference.html#wunknown-warning-option
Cc: Peter Smith <peter.smith@linaro.org>
Cc: Nathan Chancellor <natechancellor@gmail.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Change-Id: I3bb69d45bb062d1306acbf19bc0adfb60f706442
---
 Makefile                | 5 +++++
 scripts/Kconfig.include | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 9be5834073f8..28177674178a 100644
--- a/Makefile
+++ b/Makefile
@@ -517,6 +517,8 @@ ifdef building_out_of_srctree
 	{ echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
 endif
 
+KCONFIG_CC_OPTION_FLAGS := -Werror
+
 ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),)
 ifneq ($(CROSS_COMPILE),)
 CLANG_FLAGS	:= --target=$(notdir $(CROSS_COMPILE:%-=%))
@@ -531,11 +533,14 @@ ifeq ($(shell $(AS) --version 2>&1 | head -n 1 | grep clang),)
 CLANG_FLAGS	+= -no-integrated-as
 endif
 CLANG_FLAGS	+= -Werror=unknown-warning-option
+KCONFIG_CC_OPTION_FLAGS += -Werror=unknown-warning-option
 KBUILD_CFLAGS	+= $(CLANG_FLAGS)
 KBUILD_AFLAGS	+= $(CLANG_FLAGS)
 export CLANG_FLAGS
 endif
 
+export KCONFIG_CC_OPTION_FLAGS
+
 # The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
 # Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
 # CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
index 8a5c4d645eb1..144e83e7cb81 100644
--- a/scripts/Kconfig.include
+++ b/scripts/Kconfig.include
@@ -25,7 +25,7 @@ failure = $(if-success,$(1),n,y)
 
 # $(cc-option,<flag>)
 # Return y if the compiler supports <flag>, n otherwise
-cc-option = $(success,$(CC) -Werror $(1) -E -x c /dev/null -o /dev/null)
+cc-option = $(success,$(CC) $(KCONFIG_CC_OPTION_FLAGS) $(1) -E -x c /dev/null -o /dev/null)
 
 # $(ld-option,<flag>)
 # Return y if the linker supports <flag>, n otherwise
-- 
Sent by a computer through tubes

Re: [PATCH] kbuild: Check for unknown options with cc-option and clang in Kbuild

[Nathan Chancellor]

Hi Stephen,

Was the second Kbuild in the subject line supposed to be Kconfig?

On Wed, Jul 24, 2019 at 04:50:30PM -0700, Stephen Boyd wrote:
> If the particular version of clang a user has doesn't enable
> -Werror=unknown-warning-option by default, even though it is the
> default[1], then make sure to pass the option to the Kconfig cc-option

Hmmm interesting, I did not even know that was possible... Is that a
clang configuration option or an out of tree patch? Looks like it has
been on by default since clang 3.2: https://godbolt.org/z/mOmusu

> command so that testing options from Kconfig files works properly.
> Otherwise, depending on the default values setup in the clang toolchain
> we will silently assume options such as -Wmaybe-uninitialized are
> supported by clang, when they really aren't.
> 
> This issue only started happening for me once commit 589834b3a009
> ("kbuild: Add -Werror=unknown-warning-option to CLANG_FLAGS") was
> applied on top of commit b303c6df80c9 ("kbuild: compute false-positive
> -Wmaybe-uninitialized cases in Kconfig"). This leads kbuild to try and

Prior to 589834b3a009, how did cc-option work at all if
-Wunknown-warning-option wasn't enabled by default? I assume that clang
would just eat any unknown flags while returning zero so it looked like
the flag was supported?

> test for the existence of the -Wmaybe-uninitialized flag with the
> cc-option command in scripts/Kconfig.include, and it doesn't see an
> error returned from the option test so it sets the config value to Y.

It might be worth explicitly saying somewhere in here that clang will
not error on unknown flags without -Werror + -Wunknown-warning-option.

> Then the makefile tries to pass the unknown option on the command line
> and -Werror=unknown-warning-option catches the invalid option and breaks
> the build.
> 
> Note: this doesn't change the cc-option tests in Makefiles, because
> those use a different rule that includes KBUILD_CFLAGS by default, and
> the KBUILD_CFLAGS already has -Werror=unknown-warning-option. Thanks to
> Doug for pointing out the different rule.
> 
> [1] https://clang.llvm.org/docs/DiagnosticsReference.html#wunknown-warning-option
> Cc: Peter Smith <peter.smith@linaro.org>
> Cc: Nathan Chancellor <natechancellor@gmail.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> 
> Change-Id: I3bb69d45bb062d1306acbf19bc0adfb60f706442

I assume that shouldn't be there?

Overall, seems okay to me (took me a sec to understand the bug,
certainly a very specific one). It might make sense to explicitly add
somewhere in the commit message that this syncs cc-option behavior
between Kconfig and Kbuild as a whole, as I didn't understand that at
first. Thanks for the triage and sorry for the breakage!

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

Re: [PATCH] kbuild: Check for unknown options with cc-option and clang in Kbuild

[Stephen Boyd]

Quoting Nathan Chancellor (2019-07-24 22:18:57)
> Hi Stephen,
> 
> Was the second Kbuild in the subject line supposed to be Kconfig?

Sure. I'll change it to Kconfig.

> 
> On Wed, Jul 24, 2019 at 04:50:30PM -0700, Stephen Boyd wrote:
> > If the particular version of clang a user has doesn't enable
> > -Werror=unknown-warning-option by default, even though it is the
> > default[1], then make sure to pass the option to the Kconfig cc-option
> 
> Hmmm interesting, I did not even know that was possible... Is that a
> clang configuration option or an out of tree patch? Looks like it has
> been on by default since clang 3.2: https://godbolt.org/z/mOmusu

I asked and it turns out that we force this flag off in the ChromeOS
toolchain so that we can compile the multitude of packages in our system
that assume various GCC specific warning flags. I guess this is easier
than patching all the Makefiles out there.

> 
> > command so that testing options from Kconfig files works properly.
> > Otherwise, depending on the default values setup in the clang toolchain
> > we will silently assume options such as -Wmaybe-uninitialized are
> > supported by clang, when they really aren't.
> > 
> > This issue only started happening for me once commit 589834b3a009
> > ("kbuild: Add -Werror=unknown-warning-option to CLANG_FLAGS") was
> > applied on top of commit b303c6df80c9 ("kbuild: compute false-positive
> > -Wmaybe-uninitialized cases in Kconfig"). This leads kbuild to try and
> 
> Prior to 589834b3a009, how did cc-option work at all if
> -Wunknown-warning-option wasn't enabled by default? I assume that clang
> would just eat any unknown flags while returning zero so it looked like
> the flag was supported?

Yes. But just warning options?

> 
> > test for the existence of the -Wmaybe-uninitialized flag with the
> > cc-option command in scripts/Kconfig.include, and it doesn't see an
> > error returned from the option test so it sets the config value to Y.
> 
> It might be worth explicitly saying somewhere in here that clang will
> not error on unknown flags without -Werror + -Wunknown-warning-option.

I think it warns on unknown flags, just not unknown warning options
(-Wfoo), so I didn't mention this.

> 
> > Then the makefile tries to pass the unknown option on the command line
> > and -Werror=unknown-warning-option catches the invalid option and breaks
> > the build.
> > 
> > Note: this doesn't change the cc-option tests in Makefiles, because
> > those use a different rule that includes KBUILD_CFLAGS by default, and
> > the KBUILD_CFLAGS already has -Werror=unknown-warning-option. Thanks to
> > Doug for pointing out the different rule.
> > 
> > [1] https://clang.llvm.org/docs/DiagnosticsReference.html#wunknown-warning-option
> > Cc: Peter Smith <peter.smith@linaro.org>
> > Cc: Nathan Chancellor <natechancellor@gmail.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > 
> > Change-Id: I3bb69d45bb062d1306acbf19bc0adfb60f706442
> 
> I assume that shouldn't be there?
> 
> Overall, seems okay to me (took me a sec to understand the bug,
> certainly a very specific one). It might make sense to explicitly add
> somewhere in the commit message that this syncs cc-option behavior
> between Kconfig and Kbuild as a whole, as I didn't understand that at
> first. Thanks for the triage and sorry for the breakage!
> 
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

I reworded the commit text a bit now and I'll resend it soon. Thanks for
the review.

Re: [PATCH] kbuild: Check for unknown options with cc-option and clang in Kbuild

[Nathan Chancellor]

On Thu, Jul 25, 2019 at 08:41:25AM -0700, Stephen Boyd wrote:
> Quoting Nathan Chancellor (2019-07-24 22:18:57)
> > Hi Stephen,
> > 
> > Was the second Kbuild in the subject line supposed to be Kconfig?
> 
> Sure. I'll change it to Kconfig.
> 
> > 
> > On Wed, Jul 24, 2019 at 04:50:30PM -0700, Stephen Boyd wrote:
> > > If the particular version of clang a user has doesn't enable
> > > -Werror=unknown-warning-option by default, even though it is the
> > > default[1], then make sure to pass the option to the Kconfig cc-option
> > 
> > Hmmm interesting, I did not even know that was possible... Is that a
> > clang configuration option or an out of tree patch? Looks like it has
> > been on by default since clang 3.2: https://godbolt.org/z/mOmusu
> 
> I asked and it turns out that we force this flag off in the ChromeOS
> toolchain so that we can compile the multitude of packages in our system
> that assume various GCC specific warning flags. I guess this is easier
> than patching all the Makefiles out there.

Ah, that makes sense. I forget that most versions of clang have to
compile thousands of packages and such.

> 
> > 
> > > command so that testing options from Kconfig files works properly.
> > > Otherwise, depending on the default values setup in the clang toolchain
> > > we will silently assume options such as -Wmaybe-uninitialized are
> > > supported by clang, when they really aren't.
> > > 
> > > This issue only started happening for me once commit 589834b3a009
> > > ("kbuild: Add -Werror=unknown-warning-option to CLANG_FLAGS") was
> > > applied on top of commit b303c6df80c9 ("kbuild: compute false-positive
> > > -Wmaybe-uninitialized cases in Kconfig"). This leads kbuild to try and
> > 
> > Prior to 589834b3a009, how did cc-option work at all if
> > -Wunknown-warning-option wasn't enabled by default? I assume that clang
> > would just eat any unknown flags while returning zero so it looked like
> > the flag was supported?
> 
> Yes. But just warning options?
> 
> > 
> > > test for the existence of the -Wmaybe-uninitialized flag with the
> > > cc-option command in scripts/Kconfig.include, and it doesn't see an
> > > error returned from the option test so it sets the config value to Y.
> > 
> > It might be worth explicitly saying somewhere in here that clang will
> > not error on unknown flags without -Werror + -Wunknown-warning-option.
> 
> I think it warns on unknown flags, just not unknown warning options
> (-Wfoo), so I didn't mention this.

Ah right, duh (it's in the name of the option...), sorry wasn't
thinking.

> 
> > 
> > > Then the makefile tries to pass the unknown option on the command line
> > > and -Werror=unknown-warning-option catches the invalid option and breaks
> > > the build.
> > > 
> > > Note: this doesn't change the cc-option tests in Makefiles, because
> > > those use a different rule that includes KBUILD_CFLAGS by default, and
> > > the KBUILD_CFLAGS already has -Werror=unknown-warning-option. Thanks to
> > > Doug for pointing out the different rule.
> > > 
> > > [1] https://clang.llvm.org/docs/DiagnosticsReference.html#wunknown-warning-option
> > > Cc: Peter Smith <peter.smith@linaro.org>
> > > Cc: Nathan Chancellor <natechancellor@gmail.com>
> > > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > > Cc: Douglas Anderson <dianders@chromium.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > 
> > > Change-Id: I3bb69d45bb062d1306acbf19bc0adfb60f706442
> > 
> > I assume that shouldn't be there?
> > 
> > Overall, seems okay to me (took me a sec to understand the bug,
> > certainly a very specific one). It might make sense to explicitly add
> > somewhere in the commit message that this syncs cc-option behavior
> > between Kconfig and Kbuild as a whole, as I didn't understand that at
> > first. Thanks for the triage and sorry for the breakage!
> > 
> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> 
> I reworded the commit text a bit now and I'll resend it soon. Thanks for
> the review.
> 

Cheers,
Nathan