| Message ID | 20220303090643.241747-1-davidgow@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | um: clang: Strip out -mno-global-merge from USER_CFLAGS | expand |
Hi David, On Thu, Mar 03, 2022 at 05:06:42PM +0800, David Gow wrote: > The things built with USER_CFLAGS don't seem to recognise it as a > compiler option, and print a warning: > clang: warning: argument unused during compilation: '-mno-global-merge' [-Wunused-command-line-argument] > > Fixes: 744814d2fa ("um: Allow builds with Clang") > Signed-off-by: David Gow <davidgow@google.com> > --- > > This warning shows up after merging: > https://lore.kernel.org/lkml/20220227184517.504931-6-keescook@chromium.org/ > > I'm not 100% sure why this is necessary, but it does seem to work. All > the attempts to get rid of -mno-global-merge entirely have been met with > skepticism, but I'm guessing that it's not a problem for just the UML > "user" files, as they shouldn't(?) interact too much with modules. Thank you for the patch! I think it is correct, as this flag only works for AArch64 and ARM, as it is only used in Clang::AddAArch64TargetArgs() and Clang::AddARMTargetArgs() in clang/lib/Driver/ToolChains/Clang.cpp, which are obviously never called with UML. I am not sure why we do not see warning during regular kernel builds, maybe something about how UML objects are compiled exposes this? Regardless, I would definitely like to clean up this instance of the warning because I would like to make this warning a hard error so that we do not get cryptic cc-option failures: https://github.com/ClangBuiltLinux/linux/issues/1587 Reviewed-by: Nathan Chancellor <nathan@kernel.org> One small comment below. > arch/um/Makefile | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/um/Makefile b/arch/um/Makefile > index f2fe63bfd819..320b09cd513c 100644 > --- a/arch/um/Makefile > +++ b/arch/um/Makefile > @@ -75,6 +75,10 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \ > -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \ > -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__ > > +ifdef CONFIG_CC_IS_CLANG Is this ifdef needed? > +USER_CFLAGS := $(patsubst -mno-global-merge,,$(USER_CFLAGS)) > +endif > + > #This will adjust *FLAGS accordingly to the platform. > include $(srctree)/$(ARCH_DIR)/Makefile-os-$(OS) > > -- > 2.35.1.616.g0bdcbb4464-goog >
On Thu, Mar 03, 2022 at 10:04:28AM -0800, Kees Cook wrote: > On Thu, Mar 03, 2022 at 10:30:47AM -0700, Nathan Chancellor wrote: > > Hi David, > > > > On Thu, Mar 03, 2022 at 05:06:42PM +0800, David Gow wrote: > > > The things built with USER_CFLAGS don't seem to recognise it as a > > > compiler option, and print a warning: > > > clang: warning: argument unused during compilation: '-mno-global-merge' [-Wunused-command-line-argument] > > > > > > Fixes: 744814d2fa ("um: Allow builds with Clang") > > > Signed-off-by: David Gow <davidgow@google.com> > > > --- > > > > > > This warning shows up after merging: > > > https://lore.kernel.org/lkml/20220227184517.504931-6-keescook@chromium.org/ > > > > > > I'm not 100% sure why this is necessary, but it does seem to work. All > > > the attempts to get rid of -mno-global-merge entirely have been met with > > > skepticism, but I'm guessing that it's not a problem for just the UML > > > "user" files, as they shouldn't(?) interact too much with modules. > > > > Thank you for the patch! I think it is correct, as this flag only works > > for AArch64 and ARM, as it is only used in Clang::AddAArch64TargetArgs() > > and Clang::AddARMTargetArgs() in clang/lib/Driver/ToolChains/Clang.cpp, > > which are obviously never called with UML. I am not sure why we do not > > see warning during regular kernel builds, maybe something about how UML > > objects are compiled exposes this? > > Yeah, I got really turned around when I tried to find this last week. > > > > > Regardless, I would definitely like to clean up this instance of the > > warning because I would like to make this warning a hard error so that > > we do not get cryptic cc-option failures: > > > > https://github.com/ClangBuiltLinux/linux/issues/1587 > > > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > > > One small comment below. > > > > > arch/um/Makefile | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/arch/um/Makefile b/arch/um/Makefile > > > index f2fe63bfd819..320b09cd513c 100644 > > > --- a/arch/um/Makefile > > > +++ b/arch/um/Makefile > > > @@ -75,6 +75,10 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \ > > > -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \ > > > -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__ > > > > > > +ifdef CONFIG_CC_IS_CLANG > > > > Is this ifdef needed? > > > > > +USER_CFLAGS := $(patsubst -mno-global-merge,,$(USER_CFLAGS)) > > > +endif > > How does -mno-global-merge get KBUILD_CFLAGS in the first place? If it's > arm/arm64 only, shouldn't that get relocated to those architectures? > > *time travel* found it: > > 61163efae020 ("kbuild: LLVMLinux: Add Kbuild support for building kernel with Clang") > > So I think this may have been universally true long ago, and now only > arm/arm64 need it? I don't think so. -mno-global-merge was introduced in [1], which only added the option handling in Clang::AddARMTargetArgs(). In fact, when -mglobal-merge was added, there was a test added that checked that -mglobal-merge/-mno-global-merge had no impact with an x86_64 triple [2]. Perhaps at the time that 61163efae020 was written, they were testing against either ARCH=arm or ARCH=arm64, which would have shown the problem that they mentioned? There is no context in the commit message so *shrug*. Regardless, I think your patch is equally viable, perhaps even better, as I am sure this is not the only area where this warning might show up. [1]: https://github.com/llvm/llvm-project/commit/ba3df1d3ca8569ff8f6e289594c53926e71e519a [2]: https://github.com/llvm/llvm-project/commit/256a869d312b37ed7b5d131329050fcdadb11148 Cheers, Nathan > diff --git a/Makefile b/Makefile > index 2f6fbba88a0e..fbc42c3c389e 100644 > --- a/Makefile > +++ b/Makefile > @@ -852,10 +852,6 @@ ifdef CONFIG_CC_IS_CLANG > KBUILD_CPPFLAGS += -Qunused-arguments > # The kernel builds with '-std=gnu89' so use of GNU extensions is acceptable. > KBUILD_CFLAGS += -Wno-gnu > -# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the > -# source of a reference will be _MergedGlobals and not on of the whitelisted names. > -# See modpost pattern 2 > -KBUILD_CFLAGS += -mno-global-merge > else > > # gcc inanely warns about local variables called 'main' > diff --git a/arch/arm/Makefile b/arch/arm/Makefile > index a2391b8de5a5..dcab28c44c26 100644 > --- a/arch/arm/Makefile > +++ b/arch/arm/Makefile > @@ -48,6 +48,13 @@ CHECKFLAGS += -D__ARMEL__ > KBUILD_LDFLAGS += -EL > endif > > +ifdef CONFIG_CC_IS_CLANG > +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the > +# source of a reference will be _MergedGlobals and not on of the whitelisted names. > +# See modpost pattern 2 > +KBUILD_CFLAGS += -mno-global-merge > +endif > + > # > # The Scalar Replacement of Aggregates (SRA) optimization pass in GCC 4.9 and > # later may result in code being generated that handles signed short and signed > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile > index 2f1de88651e6..2d20fb2ea664 100644 > --- a/arch/arm64/Makefile > +++ b/arch/arm64/Makefile > @@ -36,6 +36,13 @@ ifeq ($(CONFIG_BROKEN_GAS_INST),y) > $(warning Detected assembler with broken .inst; disassembly will be unreliable) > endif > > +ifdef CONFIG_CC_IS_CLANG > +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the > +# source of a reference will be _MergedGlobals and not on of the whitelisted names. > +# See modpost pattern 2 > +KBUILD_CFLAGS += -mno-global-merge > +endif > + > KBUILD_CFLAGS += -mgeneral-regs-only \ > $(compat_vdso) $(cc_has_k_constraint) > KBUILD_CFLAGS += $(call cc-disable-warning, psabi) > > > > + > > > #This will adjust *FLAGS accordingly to the platform. > > > include $(srctree)/$(ARCH_DIR)/Makefile-os-$(OS) > > > > > > -- > > > 2.35.1.616.g0bdcbb4464-goog > > > > > -- > Kees Cook
On Thu, Mar 3, 2022 at 10:26 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Thu, Mar 03, 2022 at 10:04:28AM -0800, Kees Cook wrote: > > How does -mno-global-merge get KBUILD_CFLAGS in the first place? If it's > > arm/arm64 only, shouldn't that get relocated to those architectures? > > > > *time travel* found it: > > > > 61163efae020 ("kbuild: LLVMLinux: Add Kbuild support for building kernel with Clang") > > > > So I think this may have been universally true long ago, and now only > > arm/arm64 need it? Looks like that's the case from LLVM sources. <snip> > > diff --git a/arch/arm/Makefile b/arch/arm/Makefile > > index a2391b8de5a5..dcab28c44c26 100644 > > --- a/arch/arm/Makefile > > +++ b/arch/arm/Makefile > > @@ -48,6 +48,13 @@ CHECKFLAGS += -D__ARMEL__ > > KBUILD_LDFLAGS += -EL > > endif > > > > +ifdef CONFIG_CC_IS_CLANG > > +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the > > +# source of a reference will be _MergedGlobals and not on of the whitelisted names. I think there's a typo in the original comment. s/on of/one of/ ? Also, I'm not sure what's meant by _MergedGlobals. Perhaps this is an opportunity to make this clearer? "Clang's "global-merge" pass (implemented only for arm and aarch64) may break modpost Pattern 2 if symbols are renamed and thus don't appear on modpost's allowlist. > > +# See modpost pattern 2 > > +KBUILD_CFLAGS += -mno-global-merge > > +endif > > +
On Fri, Mar 4, 2022 at 1:30 AM Nathan Chancellor <nathan@kernel.org> wrote: > > Hi David, > > On Thu, Mar 03, 2022 at 05:06:42PM +0800, David Gow wrote: > > The things built with USER_CFLAGS don't seem to recognise it as a > > compiler option, and print a warning: > > clang: warning: argument unused during compilation: '-mno-global-merge' [-Wunused-command-line-argument] > > > > Fixes: 744814d2fa ("um: Allow builds with Clang") > > Signed-off-by: David Gow <davidgow@google.com> > > --- > > > > This warning shows up after merging: > > https://lore.kernel.org/lkml/20220227184517.504931-6-keescook@chromium.org/ > > > > I'm not 100% sure why this is necessary, but it does seem to work. All > > the attempts to get rid of -mno-global-merge entirely have been met with > > skepticism, but I'm guessing that it's not a problem for just the UML > > "user" files, as they shouldn't(?) interact too much with modules. > > Thank you for the patch! I think it is correct, as this flag only works > for AArch64 and ARM, as it is only used in Clang::AddAArch64TargetArgs() > and Clang::AddARMTargetArgs() in clang/lib/Driver/ToolChains/Clang.cpp, > which are obviously never called with UML. I am not sure why we do not > see warning during regular kernel builds, maybe something about how UML > objects are compiled exposes this? Yeah: this only seems to show up with UML source compiled as "user" files (i.e., talking to the host system). Most of the kernel source doesn't show the warning because -Qunused-arguments is set somewhere, probably in KBUILD_CPPFLAGS, which doesn't filter down into USER_CFLAGS: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Makefile?id=7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3#n784 Is this flag supposed to only work on ARM/AArch64, or is it just currently not implemented anywhere else. If it's actually ARM-specific, then moving it to the arch/arm{,64} makefile as in Kees' patch definitely makes more sense. If it's going to show up on other architectures at some point anyway, maybe something like this makes sense. > > Regardless, I would definitely like to clean up this instance of the > warning because I would like to make this warning a hard error so that > we do not get cryptic cc-option failures: > > https://github.com/ClangBuiltLinux/linux/issues/1587 > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > One small comment below. > > > arch/um/Makefile | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/um/Makefile b/arch/um/Makefile > > index f2fe63bfd819..320b09cd513c 100644 > > --- a/arch/um/Makefile > > +++ b/arch/um/Makefile > > @@ -75,6 +75,10 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \ > > -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \ > > -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__ > > > > +ifdef CONFIG_CC_IS_CLANG > > Is this ifdef needed? > No: it works fine without the ifdef: I just wanted to limit the damage I could do playing with things I didn't fully understand. :-) > > +USER_CFLAGS := $(patsubst -mno-global-merge,,$(USER_CFLAGS)) > > +endif > > + > > #This will adjust *FLAGS accordingly to the platform. > > include $(srctree)/$(ARCH_DIR)/Makefile-os-$(OS) > > Cheers, -- David
On Thu, Mar 3, 2022 at 9:43 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Thu, Mar 3, 2022 at 10:26 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > On Thu, Mar 03, 2022 at 10:04:28AM -0800, Kees Cook wrote: > > > How does -mno-global-merge get KBUILD_CFLAGS in the first place? If it's > > > arm/arm64 only, shouldn't that get relocated to those architectures? > > > > > > *time travel* found it: > > > > > > 61163efae020 ("kbuild: LLVMLinux: Add Kbuild support for building kernel with Clang") > > > > > > So I think this may have been universally true long ago, and now only > > > arm/arm64 need it? > > Looks like that's the case from LLVM sources. > > <snip> > > > > diff --git a/arch/arm/Makefile b/arch/arm/Makefile > > > index a2391b8de5a5..dcab28c44c26 100644 > > > --- a/arch/arm/Makefile > > > +++ b/arch/arm/Makefile > > > @@ -48,6 +48,13 @@ CHECKFLAGS += -D__ARMEL__ > > > KBUILD_LDFLAGS += -EL > > > endif > > > > > > +ifdef CONFIG_CC_IS_CLANG > > > +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the > > > +# source of a reference will be _MergedGlobals and not on of the whitelisted names. > > I think there's a typo in the original comment. > s/on of/one of/ ? > > Also, I'm not sure what's meant by _MergedGlobals. Perhaps this is an > opportunity to make this clearer? > > "Clang's "global-merge" pass (implemented only for arm and aarch64) > may break modpost Pattern 2 if symbols are renamed and thus don't > appear on modpost's allowlist. > > > > +# See modpost pattern 2 > > > +KBUILD_CFLAGS += -mno-global-merge > > > +endif > > > + > I can remember on x86-64 I was able to build and boot by dropping it entirely. - Sedat - > > > -- > Thanks, > ~Nick Desaulniers
diff --git a/arch/um/Makefile b/arch/um/Makefile index f2fe63bfd819..320b09cd513c 100644 --- a/arch/um/Makefile +++ b/arch/um/Makefile @@ -75,6 +75,10 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \ -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \ -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__ +ifdef CONFIG_CC_IS_CLANG +USER_CFLAGS := $(patsubst -mno-global-merge,,$(USER_CFLAGS)) +endif + #This will adjust *FLAGS accordingly to the platform. include $(srctree)/$(ARCH_DIR)/Makefile-os-$(OS)
The things built with USER_CFLAGS don't seem to recognise it as a compiler option, and print a warning: clang: warning: argument unused during compilation: '-mno-global-merge' [-Wunused-command-line-argument] Fixes: 744814d2fa ("um: Allow builds with Clang") Signed-off-by: David Gow <davidgow@google.com> --- This warning shows up after merging: https://lore.kernel.org/lkml/20220227184517.504931-6-keescook@chromium.org/ I'm not 100% sure why this is necessary, but it does seem to work. All the attempts to get rid of -mno-global-merge entirely have been met with skepticism, but I'm guessing that it's not a problem for just the UML "user" files, as they shouldn't(?) interact too much with modules. arch/um/Makefile | 4 ++++ 1 file changed, 4 insertions(+)