* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
@ 2022-03-03 17:30 ` Nathan Chancellor
0 siblings, 0 replies; 13+ messages in thread
From: Nathan Chancellor @ 2022-03-03 17:30 UTC (permalink / raw)
To: David Gow
Cc: Kees Cook, Jeff Dike, Richard Weinberger, Anton Ivanov,
Masahiro Yamada, Nick Desaulniers, linux-um, linux-kbuild,
linux-kselftest, kunit-dev, llvm, linux-kernel, linux-hardening
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
>
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
2022-03-03 17:30 ` Nathan Chancellor
(?)
@ 2022-03-03 18:04 ` Kees Cook
2022-03-03 18:26 ` Nathan Chancellor
-1 siblings, 1 reply; 13+ messages in thread
From: Kees Cook @ 2022-03-03 18:04 UTC (permalink / raw)
To: Nathan Chancellor
Cc: David Gow, Jeff Dike, Richard Weinberger, Anton Ivanov,
Masahiro Yamada, Nick Desaulniers, linux-um, linux-kbuild,
linux-kselftest, kunit-dev, llvm, linux-kernel, linux-hardening
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?
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
2022-03-03 18:04 ` Kees Cook
@ 2022-03-03 18:26 ` Nathan Chancellor
0 siblings, 0 replies; 13+ messages in thread
From: Nathan Chancellor @ 2022-03-03 18:26 UTC (permalink / raw)
To: Kees Cook
Cc: David Gow, Jeff Dike, Richard Weinberger, Anton Ivanov,
Masahiro Yamada, Nick Desaulniers, linux-um, linux-kbuild,
linux-kselftest, kunit-dev, llvm, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
@ 2022-03-03 18:26 ` Nathan Chancellor
0 siblings, 0 replies; 13+ messages in thread
From: Nathan Chancellor @ 2022-03-03 18:26 UTC (permalink / raw)
To: Kees Cook
Cc: David Gow, Jeff Dike, Richard Weinberger, Anton Ivanov,
Masahiro Yamada, Nick Desaulniers, linux-um, linux-kbuild,
linux-kselftest, kunit-dev, llvm, linux-kernel, linux-hardening
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
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
2022-03-03 18:26 ` Nathan Chancellor
@ 2022-03-03 18:39 ` Nick Desaulniers
-1 siblings, 0 replies; 13+ messages in thread
From: Nick Desaulniers @ 2022-03-03 18:39 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Kees Cook, David Gow, Jeff Dike, Richard Weinberger,
Anton Ivanov, Masahiro Yamada, linux-um, linux-kbuild,
linux-kselftest, kunit-dev, llvm, linux-kernel, linux-hardening
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
> > +
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
@ 2022-03-03 18:39 ` Nick Desaulniers
0 siblings, 0 replies; 13+ messages in thread
From: Nick Desaulniers @ 2022-03-03 18:39 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Kees Cook, David Gow, Jeff Dike, Richard Weinberger,
Anton Ivanov, Masahiro Yamada, linux-um, linux-kbuild,
linux-kselftest, kunit-dev, llvm, linux-kernel, linux-hardening
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
> > +
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
2022-03-03 18:39 ` Nick Desaulniers
@ 2022-03-04 11:42 ` Sedat Dilek
-1 siblings, 0 replies; 13+ messages in thread
From: Sedat Dilek @ 2022-03-04 11:42 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Nathan Chancellor, Kees Cook, David Gow, Jeff Dike,
Richard Weinberger, Anton Ivanov, Masahiro Yamada, linux-um,
linux-kbuild, linux-kselftest, kunit-dev, llvm, linux-kernel,
linux-hardening
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
@ 2022-03-04 11:42 ` Sedat Dilek
0 siblings, 0 replies; 13+ messages in thread
From: Sedat Dilek @ 2022-03-04 11:42 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Nathan Chancellor, Kees Cook, David Gow, Jeff Dike,
Richard Weinberger, Anton Ivanov, Masahiro Yamada, linux-um,
linux-kbuild, linux-kselftest, kunit-dev, llvm, linux-kernel,
linux-hardening
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
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
2022-03-03 17:30 ` Nathan Chancellor
@ 2022-03-04 6:51 ` David Gow
-1 siblings, 0 replies; 13+ messages in thread
From: David Gow @ 2022-03-04 6:51 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Kees Cook, Jeff Dike, Richard Weinberger, Anton Ivanov,
Masahiro Yamada, Nick Desaulniers, linux-um, linux-kbuild,
open list:KERNEL SELFTEST FRAMEWORK, KUnit Development, llvm,
Linux Kernel Mailing List, linux-hardening
[-- Attachment #1: Type: text/plain, Size: 3298 bytes --]
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
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] um: clang: Strip out -mno-global-merge from USER_CFLAGS
@ 2022-03-04 6:51 ` David Gow
0 siblings, 0 replies; 13+ messages in thread
From: David Gow @ 2022-03-04 6:51 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Kees Cook, Jeff Dike, Richard Weinberger, Anton Ivanov,
Masahiro Yamada, Nick Desaulniers, linux-um, linux-kbuild,
open list:KERNEL SELFTEST FRAMEWORK, KUnit Development, llvm,
Linux Kernel Mailing List, linux-hardening
[-- Attachment #1.1: Type: text/plain, Size: 3298 bytes --]
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
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
[-- Attachment #2: Type: text/plain, Size: 152 bytes --]
_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um
^ permalink raw reply [flat|nested] 13+ messages in thread