Message ID | 20190725154730.80169-1-swboyd@chromium.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] kbuild: Check for unknown options with cc-option usage in Kconfig and clang | expand |
Hi, On Thu, Jul 25, 2019 at 8:47 AM Stephen Boyd <swboyd@chromium.org> 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 > 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. > > A compilation 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. Before commit 589834b3a009 ("kbuild: Add > -Werror=unknown-warning-option to CLANG_FLAGS") the build works fine, > but any cc-option test of a warning option in Kconfig files silently > evaluates to true, even if the warning option flag isn't supported on > clang. > > Note: This doesn't change cc-option usages in Makefiles because those > use a different rule that includes KBUILD_CFLAGS by default (see the > __cc-option command in scripts/Kbuild.incluide). The KBUILD_CFLAGS > variable already has the -Werror=unknown-warning-option flag set. 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> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> Fixes: 589834b3a009 ("kbuild: Add -Werror=unknown-warning-option to CLANG_FLAGS") Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Thu, Jul 25, 2019 at 08:47:30AM -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 > 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. > > A compilation 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. Before commit 589834b3a009 ("kbuild: Add > -Werror=unknown-warning-option to CLANG_FLAGS") the build works fine, > but any cc-option test of a warning option in Kconfig files silently > evaluates to true, even if the warning option flag isn't supported on > clang. > > Note: This doesn't change cc-option usages in Makefiles because those > use a different rule that includes KBUILD_CFLAGS by default (see the > __cc-option command in scripts/Kbuild.incluide). The KBUILD_CFLAGS > variable already has the -Werror=unknown-warning-option flag set. 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> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> Commit message wording looks better to me, thanks for the change! I think we might also want to add: Cc: stable@vger.kernel.org # 4.19+ in addition to Doug's suggested fixes tag because my patch has been AUTOSEL'd by Sasha: https://lore.kernel.org/lkml/20190719040732.17285-44-sashal@kernel.org/ https://git.kernel.org/sashal/linux-stable/c/a28859fa4fea5a834a53d86d51e502012ce09c57 Alternatively, I can just mention that this patch needs to be picked up in addition to that one when it is formally sent out in a stable review. Cheers, Nathan
Hi. On Fri, Jul 26, 2019 at 12:47 AM Stephen Boyd <swboyd@chromium.org> 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 > 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. > > A compilation 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. Before commit 589834b3a009 ("kbuild: Add > -Werror=unknown-warning-option to CLANG_FLAGS") the build works fine, > but any cc-option test of a warning option in Kconfig files silently > evaluates to true, even if the warning option flag isn't supported on > clang. > > Note: This doesn't change cc-option usages in Makefiles because those > use a different rule that includes KBUILD_CFLAGS by default (see the > __cc-option command in scripts/Kbuild.incluide). The KBUILD_CFLAGS > variable already has the -Werror=unknown-warning-option flag set. 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> > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- Thanks for catching this. I wonder if we could fix this issue by one-liner, like this: diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include index 8a5c4d645eb1..4bbf4fc163a2 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) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o /dev/null) # $(ld-option,<flag>) # Return y if the linker supports <flag>, n otherwise This propagates not only -Werror=unknown-warning-option but also other clang flags to Kconfig. Currently, we do not pass the target triplet to Kconfig. This means, cc-option in Kconfig evaluates the given flags against host-arch instead of target-arch. The compiler flags are mostly independent of the architecture, and this is not a big deal, I think. But, maybe, would it make more sense to pass the other basic clang flags as well? To do this, the following is necessary as a prerequisite: https://patchwork.kernel.org/patch/11063507/ Anyway, uninitialized CLANG_FLAGS is a bug, which must be back-ported. Thanks. > Changes from v1: > * Reworded commit text a bit > * Added Reviewed-by tag > > 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 > -- Best Regards Masahiro Yamada
Quoting Masahiro Yamada (2019-07-29 03:02:40) > > Thanks for catching this. > > I wonder if we could fix this issue > by one-liner, like this: > > > diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include > index 8a5c4d645eb1..4bbf4fc163a2 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) -Werror $(CLANG_FLAGS) $(1) -E -x c > /dev/null -o /dev/null) > > # $(ld-option,<flag>) > # Return y if the linker supports <flag>, n otherwise > > > > This propagates not only -Werror=unknown-warning-option > but also other clang flags to Kconfig. > > > Currently, we do not pass the target triplet to Kconfig. > This means, cc-option in Kconfig evaluates the given flags > against host-arch instead of target-arch. > The compiler flags are mostly independent of the architecture, > and this is not a big deal, I think. > But, maybe, would it make more sense to pass the other > basic clang flags as well? > Yes that also works and I had that earlier. I wanted to mirror what was done in scripts/Kbuild.include where there's a CC_OPTION_CFLAGS variable. I'm happy either way, so it's up to you.
On Mon, Jul 29, 2019 at 11:23 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Masahiro Yamada (2019-07-29 03:02:40) > > > > Thanks for catching this. > > > > I wonder if we could fix this issue > > by one-liner, like this: > > > > > > diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include > > index 8a5c4d645eb1..4bbf4fc163a2 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) -Werror $(CLANG_FLAGS) $(1) -E -x c > > /dev/null -o /dev/null) > > > > # $(ld-option,<flag>) > > # Return y if the linker supports <flag>, n otherwise > > > > > > > > This propagates not only -Werror=unknown-warning-option > > but also other clang flags to Kconfig. > > > > > > Currently, we do not pass the target triplet to Kconfig. > > This means, cc-option in Kconfig evaluates the given flags > > against host-arch instead of target-arch. > > The compiler flags are mostly independent of the architecture, > > and this is not a big deal, I think. > > But, maybe, would it make more sense to pass the other > > basic clang flags as well? > > > > Yes that also works and I had that earlier. I wanted to mirror what was > done in scripts/Kbuild.include where there's a CC_OPTION_CFLAGS > variable. I'm happy either way, so it's up to you. > Can you post v3 with $(CLANG_FLAGS) ? Thanks.
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