LLVM: make x86_64 kernel build with clang.
Subsystems (4)
cover LLVM: make x86_64 kernel build with clang.
This patch set is sufficient to get the x86_64 kernel to build and boot correctly with clang-3.8 or greater.
The resulting build still has about 300 warnings, very few of which appear to be significant. Most of them should be fixable with some minor code refactoring although a few of them, such as the complaints about implict conversions between enumerated types may be candidates for just being disabled.
Michael Davidson (7):
Makefile, LLVM: add -no-integrated-as to KBUILD_[AC]FLAGS Makefile, x86, LLVM: disable unsupported optimization flags x86, LLVM: suppress clang warnings about unaligned accesses x86, boot, LLVM: #undef memcpy etc in string.c x86, boot, LLVM: Use regparm=0 for memcpy and memset md/raid10, LLVM: get rid of variable length array crypto, x86, LLVM: aesni - fix token pasting
Makefile | 4 ++++ arch/x86/Makefile | 7 +++++++ arch/x86/boot/copy.S | 15 +++++++++++++-- arch/x86/boot/string.c | 9 +++++++++ arch/x86/boot/string.h | 13 +++++++++++++ arch/x86/crypto/aes_ctrby8_avx-x86_64.S | 7 ++----- drivers/md/raid10.c | 9 ++++----- 7 files changed, 52 insertions(+), 12 deletions(-)
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
PATCH 1/7 Makefile, LLVM: add -no-integrated-as to KBUILD_[AC]FLAGS
Add -no-integrated-as to KBUILD_AFLAGS and KBUILD_CFLAGS for clang.
--- Makefile | 2 ++ 1 file changed, 2 insertions(+)
Makefile +2 -0
@@ -704,6 +704,8 @@ KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) # See modpost pattern 2 KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,) KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior) +KBUILD_CFLAGS += $(call cc-option, -no-integrated-as) +KBUILD_AFLAGS += $(call cc-option, -no-integrated-as) else # These warnings generated too much noise in a regular build.
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH 1/7] Makefile, LLVM: add -no-integrated-as to KBUILD_[AC]FLAGS
El Thu, Mar 16, 2017 at 05:15:14PM -0700 Michael Davidson ha dit:
Quoted 13 lines
Add -no-integrated-as to
KBUILD_AFLAGSandKBUILD_CFLAGSfor clang.Signed-off-by Michael Davidson <[email protected]>--- Makefile | 2 ++ 1 file changed, 2 insertions(+)Makefile +0
-0
Makefile KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) · line 711
Quoted 8 lines
Makefile +2
-0@@ -704,6 +704,8 @@ KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) # See modpost pattern 2 KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,) KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior) +KBUILD_CFLAGS += $(call cc-option, -no-integrated-as) +KBUILD_AFLAGS += $(call cc-option, -no-integrated-as) else # These warnings generated too much noise in a regular build.
Ping, any feedback on this patch?
Cheers
Matthias
Re: [PATCH 1/7] Makefile, LLVM: add -no-integrated-as to KBUILD_[AC]FLAGS
In reply to Michael DavidsonHi Michael,
2017-03-17 9:15 GMT+09:00 Michael Davidson <[email protected]>:
Quoted 4 lines
Add -no-integrated-as to
KBUILD_AFLAGSandKBUILD_CFLAGSfor clang.From the code-diff, it is apparent that
you added -no-integrated-as.
Rather, I'd like to see "why" in the git-log.
Obviously, clang needs this patch to build the kernel, but can you describe the reason why the integrated assembler is bad?
With git-log reworded, I will pick up this shortly.
Thanks!
Quoted 18 lines
Signed-off-by Michael Davidson <[email protected]>--- Makefile | 2 ++ 1 file changed, 2 insertions(+)Makefile +2
-0@@ -704,6 +704,8 @@ KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) # See modpost pattern 2 KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,) KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior) +KBUILD_CFLAGS += $(call cc-option, -no-integrated-as) +KBUILD_AFLAGS += $(call cc-option, -no-integrated-as) else # These warnings generated too much noise in a regular build.
Signature · 2 lines
Best Regards Masahiro Yamada
PATCH 2/7 Makefile, x86, LLVM: disable unsupported optimization flags
In reply to Michael DavidsonUnfortunately, while clang generates a warning about these flags being unsupported it still exits with a status of 0 so we have to explicitly disable them instead of just using a cc-option check.
--- Makefile | 2 ++ arch/x86/Makefile | 2 ++ 2 files changed, 4 insertions(+)
Makefile +2 -0
@@ -629,7 +629,9 @@ ARCH_AFLAGS := ARCH_CFLAGS := include arch/$(SRCARCH)/Makefile +ifneq ($(cc-name),clang) KBUILD_CFLAGS += $(call cc-option,-fno-delete-null-pointer-checks,) +endif KBUILD_CFLAGS += $(call cc-disable-warning,frame-address,) ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
arch/x86/Makefile +2 -0
@@ -87,11 +87,13 @@ else KBUILD_AFLAGS += -m64 KBUILD_CFLAGS += -m64 +ifneq ($(cc-name),clang) # Align jump targets to 1 byte, not the default 16 bytes: KBUILD_CFLAGS += -falign-jumps=1 # Pack loops tightly as well: KBUILD_CFLAGS += -falign-loops=1 +endif # Don't autogenerate traditional x87 instructions KBUILD_CFLAGS += $(call cc-option,-mno-80387)
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
H. Peter Anvin
reviewerRe: [PATCH 2/7] Makefile, x86, LLVM: disable unsupported optimization flags
Quoted 33 lines · Michael Davidson
Unfortunately, while clang generates a warning about these flags being unsupported it still exits with a status of 0 so we have to explicitly disable them instead of just using a cc-option check.
Signed-off-by Michael Davidson <[email protected]>--- Makefile | 2 ++ arch/x86/Makefile | 2 ++ 2 files changed, 4 insertions(+)Makefile +2
-0@@ -629,7 +629,9 @@ ARCH_AFLAGS := ARCH_CFLAGS := include arch/$(SRCARCH)/Makefile +ifneq ($(cc-name),clang) KBUILD_CFLAGS += $(call cc-option,-fno-delete-null-pointer-checks,) +endif KBUILD_CFLAGS += $(call cc-disable-warning,frame-address,) ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATIONarch/x86/Makefile +0
-0@@ -87,11 +87,13 @@ else KBUILD_AFLAGS += -m64 KBUILD_CFLAGS += -m64
arch/x86/Makefile else · line 99
Quoted 10 lines
arch/x86/Makefile +2
-0+ifneq ($(cc-name),clang) # Align jump targets to 1 byte, not the default 16 bytes: KBUILD_CFLAGS += -falign-jumps=1 # Pack loops tightly as well: KBUILD_CFLAGS += -falign-loops=1 +endif # Don't autogenerate traditional x87 instructions KBUILD_CFLAGS += $(call cc-option,-mno-80387)
NAK. Fix your compiler, or use a wrapper script or something. It is absolutely *not* acceptable to disable this since future versions of clang *should* support that.
That being said, it might make sense to look for a key pattern like "(un|not )supported" on stderr the try-run macro. Is there really no -Wno- or -Werror= option to turn off this craziness?
-hpa
H. Peter Anvin
reviewerRe: [PATCH 2/7] Makefile, x86, LLVM: disable unsupported optimization flags
Quoted 9 lines · H. Peter Anvin
NAK. Fix your compiler, or use a wrapper script or something. It is absolutely *not* acceptable to disable this since future versions of clang *should* support that.
That being said, it might make sense to look for a key pattern like "(un|not )supported" on stderr the try-run macro. Is there really no -Wno- or -Werror= option to turn off this craziness?
Well, guess what... I found it myself.
-W{no-,error=}ignored-optimization-argument
Either variant will make this sane.
-hpa
Re: [PATCH 2/7] Makefile, x86, LLVM: disable unsupported optimization flags
In reply to Michael DavidsonHi Michael,
2017-03-17 9:15 GMT+09:00 Michael Davidson <[email protected]>:
Quoted 5 lines
Unfortunately, while clang generates a warning about these flags being unsupported it still exits with a status of 0 so we have to explicitly disable them instead of just using a cc-option check.
Signed-off-by Michael Davidson <[email protected]>
Instead, does the following work for you?
https://patchwork.kernel.org/patch/9657285/
You need to use
$(call cc-option, ...)
for -falign-jumps=1 and -falign-loops=1
Signature · 2 lines
Best Regards Masahiro Yamada
Re: [PATCH 2/7] Makefile, x86, LLVM: disable unsupported optimization flags
Hi Masahiro,
El Thu, Apr 06, 2017 at 03:08:26AM +0900 Masahiro Yamada ha dit:
Quoted 10 lines
2017-03-17 9:15 GMT+09:00 Michael Davidson <[email protected]>:
Quoted 5 lines
Unfortunately, while clang generates a warning about these flags being unsupported it still exits with a status of 0 so we have to explicitly disable them instead of just using a cc-option check.
Signed-off-by Michael Davidson <[email protected]>Instead, does the following work for you?
https://patchwork.kernel.org/patch/9657285/
Thanks for the pointer, I was about to give this change (or rather its ancestor) a rework myself :)
Quoted 3 lines
You need to use
$(call cc-option, ...)
for -falign-jumps=1 and -falign-loops=1
I can confirm that this works.
Thanks
Matthias
Re: [PATCH 2/7] Makefile, x86, LLVM: disable unsupported optimization flags
It "works" for the cases that I currently care about but I have to say that I am uneasy about adding -Werror to the cc-option test in this way.
Suppose that one of the *other* flags that is implicitly passed to the compiler by cc-option - eg something that was explicitly specified in $(KBUILD_CFLAGS) - triggers a warning. In that case all calls to cc-option will silently fail because of the -Werror and valid options will not be detected correctly.
If everyone is OK with that because "it shouldn't normally ever happen" then that is fine, but if does result in a subtle change from existing behavior (and a trap that I almost immediately fell into after applying a similar patch).
Quoted 27 lines · Matthias Kaehlcke
Hi Masahiro,
El Thu, Apr 06, 2017 at 03:08:26AM +0900 Masahiro Yamada ha dit:
Quoted 10 lines
2017-03-17 9:15 GMT+09:00 Michael Davidson <[email protected]>:
Quoted 5 lines
Unfortunately, while clang generates a warning about these flags being unsupported it still exits with a status of 0 so we have to explicitly disable them instead of just using a cc-option check.
Signed-off-by Michael Davidson <[email protected]>Instead, does the following work for you?
https://patchwork.kernel.org/patch/9657285/Thanks for the pointer, I was about to give this change (or rather its ancestor) a rework myself :)
Quoted 3 lines
You need to use
$(call cc-option, ...)
for -falign-jumps=1 and -falign-loops=1I can confirm that this works.
Thanks
Matthias
Re: [PATCH 2/7] Makefile, x86, LLVM: disable unsupported optimization flags
Hi.
2017-04-06 4:11 GMT+09:00 Michael Davidson <[email protected]>:
Quoted 9 lines
It "works" for the cases that I currently care about but I have to say that I am uneasy about adding -Werror to the cc-option test in this way.
Suppose that one of the *other* flags that is implicitly passed to the compiler by cc-option - eg something that was explicitly specified in $(KBUILD_CFLAGS) - triggers a warning. In that case all calls to cc-option will silently fail because of the -Werror and valid options will not be detected correctly.
Theoretically, options explicitly specified in KBUILD_CFLAGS should be always valid.
Options that may not be supported in some cases should be wrapped with $(call cc-option ).
Quoted 4 lines
If everyone is OK with that because "it shouldn't normally ever happen" then that is fine, but if does result in a subtle change from existing behavior (and a trap that I almost immediately fell into after applying a similar patch).
There is a rare case where a particular combination fails (such as the conflict between -pg and -ffunction-sections as reported in https://patchwork.kernel.org/patch/9624573/).
In a such case, we may end up with swapping the order, but this should not happen quite often.
Quoted 32 lines · Matthias Kaehlcke
Quoted 22 lines
Hi Masahiro,
El Thu, Apr 06, 2017 at 03:08:26AM +0900 Masahiro Yamada ha dit:
Quoted 10 lines
2017-03-17 9:15 GMT+09:00 Michael Davidson <[email protected]>:
Quoted 5 lines
Unfortunately, while clang generates a warning about these flags being unsupported it still exits with a status of 0 so we have to explicitly disable them instead of just using a cc-option check.
Signed-off-by Michael Davidson <[email protected]>Instead, does the following work for you?
https://patchwork.kernel.org/patch/9657285/Thanks for the pointer, I was about to give this change (or rather its ancestor) a rework myself :)
Quoted 3 lines
You need to use
$(call cc-option, ...)
for -falign-jumps=1 and -falign-loops=1Quoted 5 lines
I can confirm that this works.
Thanks
Matthias
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 2 lines
Best Regards Masahiro Yamada
PATCH 3/7 x86, LLVM: suppress clang warnings about unaligned accesses
In reply to Michael DavidsonSuppress clang warnings about potential unaliged accesses to members in packed structs. This gets rid of almost 10,000 warnings about accesses to the ring 0 stack pointer in the TSS.
--- arch/x86/Makefile | 5 +++++ 1 file changed, 5 insertions(+)
arch/x86/Makefile +5 -0
@@ -128,6 +128,11 @@ endif KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args) endif +ifeq ($(cc-name),clang) +# Suppress clang warnings about potential unaligned accesses. +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) +endif + ifdef CONFIG_X86_X32 x32_ld_ok := $(call try-run,\ /bin/echo -e '1: .quad 1b' | \
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
[email protected]
reviewerRe: [PATCH 3/7] x86, LLVM: suppress clang warnings about unaligned accesses
Quoted 23 lines · Michael Davidson
Suppress clang warnings about potential unaliged accesses to members in packed structs. This gets rid of almost 10,000 warnings about accesses to the ring 0 stack pointer in the TSS.
Signed-off-by Michael Davidson <[email protected]>---
arch/x86/Makefile| 5 +++++1 file changed, 5 insertions(+)arch/x86/Makefile +0
-0@@ -128,6 +128,11 @@ endif KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)endif
arch/x86/Makefile +5
-0+ifeq ($(cc-name),clang) +# Suppress clang warnings about potential unaligned accesses. +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) +endif +ifdef
CONFIG_X86_X32
Quoted 2 lines
x32_ld_ok := $(call try-run,\ /bin/echo -e '1: .quad 1b' | \
Why conditional on clang?
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 3/7] x86, LLVM: suppress clang warnings about unaligned accesses
El Fri, Mar 17, 2017 at 04:50:19PM -0700 [email protected] ha dit:
Quoted 18 lines · Michael Davidson
Quoted 17 lines
Suppress clang warnings about potential unaliged accesses to members in packed structs. This gets rid of almost 10,000 warnings about accesses to the ring 0 stack pointer in the TSS.
Signed-off-by Michael Davidson <[email protected]>---
arch/x86/Makefile| 5 +++++1 file changed, 5 insertions(+)arch/x86/Makefile +0
-0@@ -128,6 +128,11 @@ endif KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)endif
Quoted 10 lines
Quoted 8 lines
arch/x86/Makefile +5
-0+ifeq ($(cc-name),clang) +# Suppress clang warnings about potential unaligned accesses. +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) +endif +ifdef
CONFIG_X86_X32x32_ld_ok := $(call try-run,\ /bin/echo -e '1: .quad 1b' | \Why conditional on clang?
My understanding is that this warning is clang specific, it is not listed on https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Cheers
Matthias
Re: [PATCH 3/7] x86, LLVM: suppress clang warnings about unaligned accesses
El Mon, Apr 03, 2017 at 04:01:58PM -0700 Matthias Kaehlcke ha dit:
Quoted 31 lines
El Fri, Mar 17, 2017 at 04:50:19PM -0700 [email protected] ha dit:
Quoted 18 lines · Michael Davidson
Quoted 17 lines
Suppress clang warnings about potential unaliged accesses to members in packed structs. This gets rid of almost 10,000 warnings about accesses to the ring 0 stack pointer in the TSS.
Signed-off-by Michael Davidson <[email protected]>---
arch/x86/Makefile| 5 +++++1 file changed, 5 insertions(+)arch/x86/Makefile +0
-0@@ -128,6 +128,11 @@ endif KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)endif
Quoted 10 lines
Quoted 8 lines
arch/x86/Makefile +5
-0+ifeq ($(cc-name),clang) +# Suppress clang warnings about potential unaligned accesses. +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) +endif +ifdef
CONFIG_X86_X32x32_ld_ok := $(call try-run,\ /bin/echo -e '1: .quad 1b' | \Why conditional on clang?
Quoted 2 lines
My understanding is that this warning is clang specific, it is not listed on https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Actually this warning affects other platforms besides x86 (e.g. arm64), I'll submit a patch that disables the warning on all platforms.
Cheers
Matthias
H. Peter Anvin
reviewerRe: [PATCH 3/7] x86, LLVM: suppress clang warnings about unaligned accesses
Quoted 33 lines · Matthias Kaehlcke
El Mon, Apr 03, 2017 at 04:01:58PM -0700 Matthias Kaehlcke ha dit:
Quoted 31 lines
El Fri, Mar 17, 2017 at 04:50:19PM -0700 [email protected] ha dit:
Quoted 28 lines · Michael Davidson
Quoted 23 lines
Suppress clang warnings about potential unaliged accesses to members in packed structs. This gets rid of almost 10,000 warnings about accesses to the ring 0 stack pointer in the TSS.
Signed-off-by Michael Davidson <[email protected]>---
arch/x86/Makefile| 5 +++++1 file changed, 5 insertions(+)arch/x86/Makefile +0
-0@@ -128,6 +128,11 @@ endif KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)endif
arch/x86/Makefile +5
-0+ifeq ($(cc-name),clang) +# Suppress clang warnings about potential unaligned accesses. +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) +endif +ifdef
CONFIG_X86_X32Quoted 2 lines
x32_ld_ok := $(call try-run,\ /bin/echo -e '1: .quad 1b' | \Why conditional on clang?
Quoted 7 lines
Quoted 2 lines
My understanding is that this warning is clang specific, it is not listed on https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Actually this warning affects other platforms besides x86 (e.g. arm64), I'll submit a patch that disables the warning on all platforms.
Drop the ifeq ($(cc-name),clang).
You should assume that if you have to add one of those ifeq's then you are doing something fundamentally wrong.
-hpa
Re: [PATCH 3/7] x86, LLVM: suppress clang warnings about unaligned accesses
El Thu, Apr 13, 2017 at 04:55:00PM -0700 H. Peter Anvin ha dit:
Quoted 42 lines · Matthias Kaehlcke
Quoted 33 lines
El Mon, Apr 03, 2017 at 04:01:58PM -0700 Matthias Kaehlcke ha dit:
Quoted 31 lines
El Fri, Mar 17, 2017 at 04:50:19PM -0700 [email protected] ha dit:
Quoted 28 lines · Michael Davidson
Quoted 23 lines
Suppress clang warnings about potential unaliged accesses to members in packed structs. This gets rid of almost 10,000 warnings about accesses to the ring 0 stack pointer in the TSS.
Signed-off-by Michael Davidson <[email protected]>---
arch/x86/Makefile| 5 +++++1 file changed, 5 insertions(+)arch/x86/Makefile +0
-0@@ -128,6 +128,11 @@ endif KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args)endif
arch/x86/Makefile +5
-0+ifeq ($(cc-name),clang) +# Suppress clang warnings about potential unaligned accesses. +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member) +endif +ifdef
CONFIG_X86_X32Quoted 2 lines
x32_ld_ok := $(call try-run,\ /bin/echo -e '1: .quad 1b' | \Why conditional on clang?
Quoted 7 lines
Quoted 2 lines
My understanding is that this warning is clang specific, it is not listed on https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Actually this warning affects other platforms besides x86 (e.g. arm64), I'll submit a patch that disables the warning on all platforms.
Quoted 4 lines
Drop the ifeq ($(cc-name),clang).
You should assume that if you have to add one of those ifeq's then you are doing something fundamentally wrong.
Thanks, however in the case of the global Makefile it seems we should put it inside the already existing clang section:
http://lxr.free-electrons.com/source/Makefile#L692
Cheers
Matthias
[email protected]
reviewerRe: [PATCH 3/7] x86, LLVM: suppress clang warnings about unaligned accesses
Quoted 53 lines · Matthias Kaehlcke
El Thu, Apr 13, 2017 at 04:55:00PM -0700 H. Peter Anvin ha dit:
Quoted 6 lines · Matthias Kaehlcke
Quoted 5 lines
El Mon, Apr 03, 2017 at 04:01:58PM -0700 Matthias Kaehlcke ha dit:
Quoted 3 lines
El Fri, Mar 17, 2017 at 04:50:19PM -0700 [email protected] ha dit:
Quoted 1 line
On March 16, 2017 5:15:16 PM PDT, Michael Davidson
Quoted 9 lines · <[email protected]> wrote:
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Suppress clang warnings about potential unaliged accesses to members in packed structs. This gets rid of almost 10,000 warnings about accesses to the ring 0 stack pointer in the TSS.
Signed-off-by Michael Davidson <[email protected]>---
arch/x86/Makefile| 5 +++++1 file changed, 5 insertions(+)Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
arch/x86/Makefile endif · line 128
Quoted 6 lines
arch/x86/Makefile +0
-0@@ -128,6 +128,11 @@ endif KBUILD_CFLAGS += $(callcc-option,-maccumulate-outgoing-args)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
arch/x86/Makefile endif
Quoted 5 lines
endif
arch/x86/Makefile +3
-0+ifeq ($(cc-name),clang) +# Suppress clang warnings about potential unaligned accesses. +KBUILD_CFLAGS += $(call cc-disable-warning,address-of-packed-member)
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 7 lines
Quoted 5 lines
arch/x86/Makefile +2
-0+endif +ifdef
CONFIG_X86_X32x32_ld_ok := $(call try-run,\ /bin/echo -e '1: .quad 1b' | \Why conditional on clang?
My understanding is that this warning is clang specific, it is not listed on https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Quoted 8 lines
Quoted 4 lines
Actually this warning affects other platforms besides x86 (e.g. arm64), I'll submit a patch that disables the warning on all platforms.
Drop the ifeq ($(cc-name),clang).
You should assume that if you have to add one of those ifeq's then
you
Quoted 1 line
are doing something fundamentally wrong.
Quoted 8 lines
Thanks, however in the case of the global
Makefileit seems we should put it inside the already existing clang section:http://lxr.free-electrons.com/source/Makefile#L692
Cheers
Matthias
We shouldn't, unless it will actively break non-clang builds...
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
PATCH 4/7 x86, boot, LLVM: #undef memcpy etc in string.c
In reply to Michael Davidsonundef memcpy and friends in boot/string.c so that the functions defined here will have the correct names, otherwise we end up up trying to redefine __builtin_memcpy etc. Surprisingly, gcc allows this (and, helpfully, discards the __builtin_ prefix from the function name when compiling it), but clang does not.
Adding these #undef's appears to preserve what I assume was the original intent of the code.
--- arch/x86/boot/string.c | 9 +++++++++ 1 file changed, 9 insertions(+)
arch/x86/boot/string.c +9 -0
@@ -16,6 +16,15 @@#include "ctype.h" #include "string.h" +/* +* Undef these macros so that the functions that we provide +* here will have the correct names regardless of how string.h +* may have chosen to #define them. +*/ +#undef memcpy +#undef memset +#undef memcmp +int memcmp (const void *s1,const void *s2,size_t len) {bool diff;
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH 4/7] x86, boot, LLVM: #undef memcpy etc in string.c
(removed some non-x86 lists and folks from recipients)
El Thu, Mar 16, 2017 at 05:15:17PM -0700 Michael Davidson ha dit:
Quoted 9 lines
undef memcpy and friends in
boot/string.cso that the functions defined here will have the correct names, otherwise we end up up trying to redefine__builtin_memcpyetc. Surprisingly, gcc allows this (and, helpfully, discards the__builtin_prefix from the function name when compiling it), but clang does not.Adding these #undef's appears to preserve what I assume was the original intent of the code.
Any comments on this patch?
Quoted 25 lines
Signed-off-by Michael Davidson <[email protected]>--- arch/x86/boot/string.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/x86/boot/string.c +9
-0@@ -16,6 +16,15 @@#include "ctype.h" #include "string.h" +/* +* Undef these macros so that the functions that we provide +* here will have the correct names regardless of how string.h +* may have chosen to #define them. +*/ +#undef memcpy +#undef memset +#undef memcmp +int memcmp (const void *s1,const void *s2,size_t len) {bool diff;
Re: [PATCH 4/7] x86, boot, LLVM: #undef memcpy etc in string.c
Quoted 29 lines · Matthias Kaehlcke
(removed some non-x86 lists and folks from recipients)
El Thu, Mar 16, 2017 at 05:15:17PM -0700 Michael Davidson ha dit:
Quoted 9 lines
undef memcpy and friends in
boot/string.cso that the functions defined here will have the correct names, otherwise we end up up trying to redefine__builtin_memcpyetc. Surprisingly, gcc allows this (and, helpfully, discards the__builtin_prefix from the function name when compiling it), but clang does not.Adding these #undef's appears to preserve what I assume was the original intent of the code.
Any comments on this patch?
Quoted 13 lines
Signed-off-by Michael Davidson <[email protected]>--- arch/x86/boot/string.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/x86/boot/string.c +0
-0@@ -16,6 +16,15 @@#include "ctype.h" #include "string.h"
Quoted 12 lines
arch/x86/boot/string.c line 30
Quoted 12 lines
arch/x86/boot/string.c +9
-0+/* +* Undef these macros so that the functions that we provide +* here will have the correct names regardless of how string.h +* may have chosen to #define them. +*/ +#undef memcpy +#undef memset +#undef memcmp +int memcmp (const void *s1,const void *s2,size_t len) {bool diff;
-hpa
Re: [PATCH 4/7] x86, boot, LLVM: #undef memcpy etc in string.c
El Thu, Jun 22, 2017 at 03:37:23PM -0700 H. Peter Anvin ha dit:
Quoted 30 lines · Matthias Kaehlcke
Quoted 29 lines
(removed some non-x86 lists and folks from recipients)
El Thu, Mar 16, 2017 at 05:15:17PM -0700 Michael Davidson ha dit:
Quoted 9 lines
undef memcpy and friends in
boot/string.cso that the functions defined here will have the correct names, otherwise we end up up trying to redefine__builtin_memcpyetc. Surprisingly, gcc allows this (and, helpfully, discards the__builtin_prefix from the function name when compiling it), but clang does not.Adding these #undef's appears to preserve what I assume was the original intent of the code.
Any comments on this patch?
Quoted 13 lines
Signed-off-by Michael Davidson <[email protected]>--- arch/x86/boot/string.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/x86/boot/string.c +0
-0@@ -16,6 +16,15 @@#include "ctype.h" #include "string.h"
Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
arch/x86/boot/string.c +4
-0+/* +* Undef these macros so that the functions that we provide +* here will have the correct names regardless of how string.h +* may have chosen to #define them. Quoted 8 lines
arch/x86/boot/string.c line 30
Quoted 8 lines
arch/x86/boot/string.c +5
-0+*/ +#undef memcpy +#undef memset +#undef memcmp +int memcmp (const void *s1,const void *s2,size_t len) {bool diff;Acked-by H. Peter Anvin <[email protected]>
Ingo, do you plan to pick this change?
Thanks
Matthias
PATCH 5/7 x86, boot, LLVM: Use regparm=0 for memcpy and memset
In reply to Michael DavidsonUse the standard regparm=0 calling convention for memcpy and memset when building with clang.
This is a work around for a long standing clang bug (see https://llvm.org/bugs/show_bug.cgi?id=3997) where clang always uses the standard regparm=0 calling convention for any implcit calls to memcpy and memset that it generates (eg for structure assignments and initialization) even if an alternate calling convention such as regparm=3 has been specified.
--- arch/x86/boot/copy.S | 15 +++++++++++++-- arch/x86/boot/string.h | 13 +++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-)
arch/x86/boot/copy.S +13 -2
@@ -18,6 +18,12 @@ .text GLOBAL(memcpy) +#ifdef __clang__ /* Use normal ABI calling conventions */ + movw 4(%esp), %ax + movw 8(%esp), %dx + movw 12(%esp), %cx +#endif +_memcpy: pushw %si pushw %di movw %ax, %di
@@ -34,6 +40,11 @@ GLOBAL(memcpy) ENDPROC(memcpy) GLOBAL(memset) +#ifdef __clang__ /* Use normal ABI calling conventions */ + movw 4(%esp), %ax + movw 8(%esp), %dx + movw 12(%esp), %cx +#endif pushw %di movw %ax, %di movzbl %dl, %eax
@@ -52,7 +63,7 @@ GLOBAL(copy_from_fs) pushw %ds pushw %fs popw %ds - calll memcpy + calll _memcpy popw %ds retl ENDPROC(copy_from_fs)
@@ -61,7 +72,7 @@ GLOBAL(copy_to_fs) pushw %es pushw %fs popw %es - calll memcpy + calll _memcpy popw %es retl ENDPROC(copy_to_fs)
arch/x86/boot/string.h +13 -0
@@ -6,8 +6,21 @@#undef memset#undef memcmp +/* +* Use normal ABI calling conventions - i.e. regparm(0) - +* for memcpy() and memset() if we are building the real +* mode setup code with clang since clang may make implicit +* calls to these functions that assume regparm(0). +*/ +#if defined(_SETUP) && defined(__clang__) +void __attribute__((regparm (0 ))) *memcpy (void *dst,const void *src, +size_t len); +void __attribute__((regparm (0 ))) *memset (void *dst,int c,size_t len); +#else void *memcpy (void *dst,const void *src,size_t len);void *memset (void *dst,int c,size_t len); +#endif +int memcmp (const void *s1,const void *s2,size_t len);/*
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH 5/7] x86, boot, LLVM: Use regparm=0 for memcpy and memset
Quoted 9 lines · Michael Davidson
Use the standard regparm=0 calling convention for memcpy and memset when building with clang.
This is a work around for a long standing clang bug (see https://llvm.org/bugs/show_bug.cgi?id=3997) where clang always uses the standard regparm=0 calling convention for any implcit calls to memcpy and memset that it generates (eg for structure assignments and initialization) even if an alternate calling convention such as regparm=3 has been specified.
Seriously, fix LLVM already.
H. Peter Anvin
reviewerRe: [PATCH 5/7] x86, boot, LLVM: Use regparm=0 for memcpy and memset
Quoted 4 lines · Peter Zijlstra
Quoted 3 lines · Michael Davidson
Use the standard regparm=0 calling convention for memcpy and memset when building with clang.
Quoted 9 lines
Quoted 6 lines
This is a work around for a long standing clang bug (see https://llvm.org/bugs/show_bug.cgi?id=3997) where clang always uses the standard regparm=0 calling convention for any implcit calls to memcpy and memset that it generates (eg for structure assignments and initialization) even if an alternate calling convention such as regparm=3 has been specified.
Seriously, fix LLVM already.
Yes, this is a real stinker, in no small part because once clang is fixed to DTRT then this is actually broken...
-hpa
PATCH 6/7 md/raid10, LLVM: get rid of variable length array
In reply to Michael DavidsonReplace a variable length array in a struct by allocating the memory for the entire struct in a char array on the stack.
--- drivers/md/raid10.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/md/raid10.c +4 -5
@@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,/* Use sync reads to get the blocks from somewhere else */ int sectors = r10_bio->sectors;struct r10conf *conf = mddev->private; -struct { -struct r10bio r10_bio; -struct r10dev devs[conf->copies]; - } on_stack; -struct r10bio *r10b = &on_stack.r10_bio; +char on_stack_r10_bio[sizeof (struct r10bio ) + + conf->copies *sizeof (struct r10dev )] +__aligned(__alignof__(struct r10bio)) ; +struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;int slot =0 ;int idx =0 ;struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 18 lines · Michael Davidson
Replace a variable length array in a struct by allocating the memory for the entire struct in a char array on the stack.
Signed-off-by Michael Davidson <[email protected]>--- drivers/md/raid10.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)drivers/md/raid10.c +0
-2@@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,/* Use sync reads to get the blocks from somewhere else */ int sectors = r10_bio->sectors;struct r10conf *conf = mddev->private; -struct { -struct r10bio r10_bio;
drivers/md/raid10.c static int handle_reshape_read_error(struct mddev *mddev, · line 4663
Quoted 10 lines
drivers/md/raid10.c +4
-3-struct r10dev devs[conf->copies]; - } on_stack; -struct r10bio *r10b = &on_stack.r10_bio; +char on_stack_r10_bio[sizeof (struct r10bio ) + + conf->copies *sizeof (struct r10dev )] +__aligned(__alignof__(struct r10bio)) ; +struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;int slot =0 ;int idx =0 ;struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
That's disgusting. Why not fix LLVM to support this?
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 24 lines · Peter Zijlstra
Quoted 23 lines · Michael Davidson
Replace a variable length array in a struct by allocating the memory for the entire struct in a char array on the stack.
Signed-off-by Michael Davidson <[email protected]>--- drivers/md/raid10.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)drivers/md/raid10.c +2
-5@@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,/* Use sync reads to get the blocks from somewhere else */ int sectors = r10_bio->sectors;struct r10conf *conf = mddev->private; -struct { -struct r10bio r10_bio; -struct r10dev devs[conf->copies]; - } on_stack; -struct r10bio *r10b = &on_stack.r10_bio; +char on_stack_r10_bio[sizeof (struct r10bio ) + + conf->copies *sizeof (struct r10dev )]
Quoted 8 lines
drivers/md/raid10.c static int handle_reshape_read_error(struct mddev *mddev, · line 4663
Quoted 5 lines
drivers/md/raid10.c +2
-0+__aligned(__alignof__(struct r10bio)) ; +struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;int slot =0 ;int idx =0 ;struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;That's disgusting. Why not fix LLVM to support this?
IIUC there's only a handful of VLAIS instances in LLVM code, why not just drop them for the sake of better code portability? (To quote Linus, "this feature is an abomination":
https://lkml.org/lkml/2013/9/23/500)
Signature · 10 lines
Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 34 lines · Alexander Potapenko
Quoted 24 lines · Peter Zijlstra
Quoted 23 lines · Michael Davidson
Replace a variable length array in a struct by allocating the memory for the entire struct in a char array on the stack.
Signed-off-by Michael Davidson <[email protected]>--- drivers/md/raid10.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)drivers/md/raid10.c +2
-5@@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,/* Use sync reads to get the blocks from somewhere else */ int sectors = r10_bio->sectors;struct r10conf *conf = mddev->private; -struct { -struct r10bio r10_bio; -struct r10dev devs[conf->copies]; - } on_stack; -struct r10bio *r10b = &on_stack.r10_bio; +char on_stack_r10_bio[sizeof (struct r10bio ) + + conf->copies *sizeof (struct r10dev )]Quoted 8 lines
drivers/md/raid10.c static int handle_reshape_read_error(struct mddev *mddev, · line 4663
Quoted 5 lines
drivers/md/raid10.c +2
-0+__aligned(__alignof__(struct r10bio)) ; +struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;int slot =0 ;int idx =0 ;struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;That's disgusting. Why not fix LLVM to support this?
Quoted 1 line
IIUC there's only a handful of VLAIS instances in LLVM code, why not
Sorry, "kernel code", not "LLVM code".
Quoted 15 lines
just drop them for the sake of better code portability? (To quote Linus, "this feature is an abomination":
https://lkml.org/lkml/2013/9/23/500)--
Alexander Potapenko
Software EngineerGoogle Germany GmbH
Erika-Mann-Straße, 33
80636 MünchenGeschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Signature · 10 lines
Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
Borislav Petkov
maintainerRe: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 3 lines · Alexander Potapenko
Quoted 1 line
IIUC there's only a handful of VLAIS instances in LLVM code, why not
Sorry, "kernel code", not "LLVM code".
Quoted 1 line
just drop them for the sake of better code portability?
And what happens if someone else adds a variable thing like this somewhere else, builds with gcc, everything's fine and patch gets applied? Or something else llvm can't stomach.
Does that mean there'll be the occasional, every-so-often whack-a-mole patchset from someone, fixing the kernel build with llvm yet again?
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 11 lines · Borislav Petkov
Quoted 3 lines · Alexander Potapenko
Quoted 1 line
IIUC there's only a handful of VLAIS instances in LLVM code, why not
Sorry, "kernel code", not "LLVM code".
Quoted 1 line
just drop them for the sake of better code portability?
And what happens if someone else adds a variable thing like this somewhere else, builds with gcc, everything's fine and patch gets applied? Or something else llvm can't stomach.
Does that mean there'll be the occasional, every-so-often whack-a-mole patchset from someone, fixing the kernel build with llvm yet again?
This problem is more general and is not specific to clang. It equally applies to different versions of gcc, different arches and different configs (namely, anything else than what a developer used for testing). A known, reasonably well working solution to this problem is a system of try bots that test patches before commit with different compilers/configs/archs. We already have such system in the form of 0-day bots. It would be useful to extend it with clang as soon as kernel builds.
Borislav Petkov
maintainerRe: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 4 lines · Dmitry Vyukov
This problem is more general and is not specific to clang. It equally applies to different versions of gcc, different arches and different configs (namely, anything else than what a developer used for testing).
I guess. We do carry a bunch of gcc workarounds along with the cc-* macros in scripts/Kbuild.include.
Quoted 5 lines
A known, reasonably well working solution to this problem is a system of try bots that test patches before commit with different compilers/configs/archs. We already have such system in the form of 0-day bots. It would be useful to extend it with clang as soon as kernel builds.
Has someone actually already talked to Fengguang about it?
Oh, and the stupid question: why the effort to build the kernel with clang at all? Just because or are there some actual, palpable advantages?
Signature · 4 lines
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 9 lines · Borislav Petkov
Quoted 4 lines · Dmitry Vyukov
This problem is more general and is not specific to clang. It equally applies to different versions of gcc, different arches and different configs (namely, anything else than what a developer used for testing).
I guess. We do carry a bunch of gcc workarounds along with the cc-* macros in
scripts/Kbuild.include.
Quoted 7 lines
Quoted 5 lines
A known, reasonably well working solution to this problem is a system of try bots that test patches before commit with different compilers/configs/archs. We already have such system in the form of 0-day bots. It would be useful to extend it with clang as soon as kernel builds.
Has someone actually already talked to Fengguang about it?
+Fengguang
Quoted 3 lines
Oh, and the stupid question: why the effort to build the kernel with clang at all? Just because or are there some actual, palpable advantages?
On our side it is:
- clang make it possible to implement KMSAN (dynamic detection of
uses of uninit memory)
- better code coverage for fuzzing
- why simpler and faster development (e.g. we can port our user-space
hardening technologies -- CFI and SafeStack)
You can also find some reasons in the Why section of LLVM-Linux project:
http://llvm.linuxfoundation.org/index.php/Main_Page
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 4 lines · Dmitry Vyukov
You can also find some reasons in the Why section of LLVM-Linux project:
http://llvm.linuxfoundation.org/index.php/Main_PageFrom that:
- LLVM/Clang is a fast moving project with many things fixed quickly and features added.
So what's the deal with that 5 year old bug you want us to work around?
Also, clang doesn't support asm cc flags output and a few other extensions last time I checked.
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 6 lines · Peter Zijlstra
Quoted 2 lines · Dmitry Vyukov
You can also find some reasons in the Why section of LLVM-Linux project:
http://llvm.linuxfoundation.org/index.php/Main_PageFrom that:
Quoted 8 lines
- LLVM/Clang is a fast moving project with many things fixed quickly and features added.
So what's the deal with that 5 year old bug you want us to work around?
Also, clang doesn't support asm cc flags output and a few other extensions last time I checked.
Another great one:
- BSD License (some people prefer this license to the GPL)
Seems a very weak argument to make when talking about the Linux Kernel which is very explicitly GPLv2 (and not later).
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 16 lines · Peter Zijlstra
Quoted 6 lines · Peter Zijlstra
Quoted 2 lines · Dmitry Vyukov
You can also find some reasons in the Why section of LLVM-Linux project:
http://llvm.linuxfoundation.org/index.php/Main_PageFrom that:
Quoted 8 lines
- LLVM/Clang is a fast moving project with many things fixed quickly and features added.
So what's the deal with that 5 year old bug you want us to work around?
Also, clang doesn't support asm cc flags output and a few other extensions last time I checked.
Quoted 6 lines
Another great one:
- BSD License (some people prefer this license to the GPL)
Seems a very weak argument to make when talking about the Linux Kernel which is very explicitly GPLv2 (and not later).
OK, I guess should not have referenced the llvm-linux page. So here are reasons on our side that I am ready to vouch:
- clang make it possible to implement KMSAN (dynamic detection of
uses of uninit memory)
- better code coverage for fuzzing
- why simpler and faster development (e.g. we can port our user-space
hardening technologies -- CFI and SafeStack)
Michael is on a different team and has own reasons to do this.
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 5 lines · Dmitry Vyukov
OK, I guess should not have referenced the llvm-linux page. So here are reasons on our side that I am ready to vouch:
- clang make it possible to implement KMSAN (dynamic detection of
uses of uninit memory)
How does GCC make this impossible?
Quoted 1 line
- better code coverage for fuzzing
How so? Why can't the same be achieved using GCC?
Quoted 2 lines
- why simpler and faster development (e.g. we can port our user-space
hardening technologies -- CFI and SafeStack)
That's just because you've already implemented this in clang, right? So less work for you. Not because its impossible.
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 8 lines · Peter Zijlstra
Quoted 5 lines · Dmitry Vyukov
OK, I guess should not have referenced the llvm-linux page. So here are reasons on our side that I am ready to vouch:
- clang make it possible to implement KMSAN (dynamic detection of
uses of uninit memory)How does GCC make this impossible?
Too complex and too difficult to implement correctly on all corner cases. All other sanitizers were ported to gcc very quickly, but msan wasn't. Nobody is brave enough to even approach it.
Quoted 3 lines
Quoted 1 line
- better code coverage for fuzzing
How so? Why can't the same be achieved using GCC?
Same reason.
Quoted 5 lines
Quoted 2 lines
- why simpler and faster development (e.g. we can port our user-space
hardening technologies -- CFI and SafeStack)That's just because you've already implemented this in clang, right? So less work for you. Not because its impossible.
I am not saying that it's impossible. It would just require unreasonable amount of time, and then perpetual maintenance to fix corner cases and regressions.
For background: I implemented the current fuzzing coverage (KCOV) in gcc, and user-space tsan instrumentation in gcc.
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
In reply to Dmitry VyukovHi Dmitry,
Quoted 16 lines · Dmitry Vyukov
Quoted 15 lines · Borislav Petkov
Quoted 4 lines · Dmitry Vyukov
This problem is more general and is not specific to clang. It equally applies to different versions of gcc, different arches and different configs (namely, anything else than what a developer used for testing).
I guess. We do carry a bunch of gcc workarounds along with the cc-* macros in
scripts/Kbuild.include.Quoted 5 lines
A known, reasonably well working solution to this problem is a system of try bots that test patches before commit with different compilers/configs/archs. We already have such system in the form of 0-day bots. It would be useful to extend it with clang as soon as kernel builds.
Quoted 3 lines
Quoted 1 line
Has someone actually already talked to Fengguang about it?
+Fengguang
I've actually tried clang long time ago. It quickly fails the build for vanilla kernel. So it really depends on when the various clang build fix patches can be accepted into mainline kernel.
Thanks,
Fengguang
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
In reply to Alexander PotapenkoQuoted 34 lines · Alexander Potapenko
Quoted 24 lines · Peter Zijlstra
Quoted 23 lines · Michael Davidson
Replace a variable length array in a struct by allocating the memory for the entire struct in a char array on the stack.
Signed-off-by Michael Davidson <[email protected]>--- drivers/md/raid10.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)drivers/md/raid10.c +2
-5@@ -4654,11 +4654,10 @@ static int handle_reshape_read_error(struct mddev *mddev,/* Use sync reads to get the blocks from somewhere else */ int sectors = r10_bio->sectors;struct r10conf *conf = mddev->private; -struct { -struct r10bio r10_bio; -struct r10dev devs[conf->copies]; - } on_stack; -struct r10bio *r10b = &on_stack.r10_bio; +char on_stack_r10_bio[sizeof (struct r10bio ) + + conf->copies *sizeof (struct r10dev )]Quoted 8 lines
drivers/md/raid10.c static int handle_reshape_read_error(struct mddev *mddev, · line 4663
Quoted 5 lines
drivers/md/raid10.c +2
-0+__aligned(__alignof__(struct r10bio)) ; +struct r10bio *r10b = (struct r10bio *)on_stack_r10_bio;int slot =0 ;int idx =0 ;struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;That's disgusting. Why not fix LLVM to support this?
Quoted 4 lines
IIUC there's only a handful of VLAIS instances in LLVM code, why not just drop them for the sake of better code portability? (To quote Linus, "this feature is an abomination":
https://lkml.org/lkml/2013/9/23/500)
Be that as it may; what you construct above is disgusting. Surely the code can be refactored to not look like dog vomit?
Also; its not immediately obvious conf->copies is 'small' and this doesn't blow up the stack; I feel that deserves a comment somewhere.
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 7 lines · Peter Zijlstra
Be that as it may; what you construct above is disgusting. Surely the code can be refactored to not look like dog vomit?
Also; its not immediately obvious
conf->copiesis 'small' and this doesn't blow up the stack; I feel that deserves a comment somewhere.
I agree that the code is horrible.
It is, in fact, exactly the same solution that was used to remove variable length arrays in structs from several of the crypto drivers a few years ago - see the definition of SHASH_DESC_ON_STACK() in "crypto/hash.h" - I did not, however, hide the horrors in a macro preferring to leave the implementation visible as a warning to whoever might touch the code next.
I believe that the actual stack usage is exactly the same as it was previously.
I can certainly wrap this up in a macro and add comments with appropriately dire warnings in it if you feel that is both necessary and sufficient.
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 18 lines · Michael Davidson
Quoted 7 lines · Peter Zijlstra
Be that as it may; what you construct above is disgusting. Surely the code can be refactored to not look like dog vomit?
Also; its not immediately obvious
conf->copiesis 'small' and this doesn't blow up the stack; I feel that deserves a comment somewhere.I agree that the code is horrible.
It is, in fact, exactly the same solution that was used to remove variable length arrays in structs from several of the crypto drivers a few years ago - see the definition of
SHASH_DESC_ON_STACK()in "crypto/hash.h" - I did not, however, hide the horrors in a macro preferring to leave the implementation visible as a warning to whoever might touch the code next.
Quoted 5 lines
I believe that the actual stack usage is exactly the same as it was previously.
I can certainly wrap this up in a macro and add comments with appropriately dire warnings in it if you feel that is both necessary and sufficient.
We got away with ugly in the past, so we should get to do it again?
[email protected]
reviewerRe: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 24 lines · Peter Zijlstra
Quoted 1 line · Michael Davidson
On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra
Quoted 2 lines · <[email protected]> wrote:
Quoted 2 lines
Be that as it may; what you construct above is disgusting. Surely
the
Quoted 4 lines
Quoted 4 lines
code can be refactored to not look like dog vomit?
Also; its not immediately obvious
conf->copiesis 'small' and this doesn't blow up the stack; I feel that deserves a commentsomewhere.
Quoted 6 lines
Quoted 1 line
I agree that the code is horrible.
It is, in fact, exactly the same solution that was used to remove variable length arrays in structs from several of the crypto drivers
a
Quoted 3 lines
few years ago - see the definition of
SHASH_DESC_ON_STACK()in "crypto/hash.h" - I did not, however, hide the horrors in a macro preferring to leave the implementation visible as a warning towhoever
Quoted 2 lines
might touch the code next.
Quoted 8 lines
Quoted 1 line
I believe that the actual stack usage is exactly the same as it was
previously.
Quoted 4 lines
I can certainly wrap this up in a macro and add comments with appropriately dire warnings in it if you feel that is both necessary and sufficient.
We got away with ugly in the past, so we should get to do it again?
Seriously, you should have taken the hack the first time that this needs to be fixed. Just because this is a fairly uncommon construct in the kernel doesn't mean it is not in userspace.
I would like to say this falls in the category of "fix your compiler this time". Once is one thing, twice is unacceptable.
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 28 lines · On Fri, Mar 17, 2017 at 9:04 PM, <[email protected]> wrote:
Quoted 27 lines · Peter Zijlstra
Quoted 1 line · Michael Davidson
On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra
Quoted 2 lines · <[email protected]> wrote:
Quoted 2 lines
Be that as it may; what you construct above is disgusting. Surely
the
Quoted 4 lines
Quoted 4 lines
code can be refactored to not look like dog vomit?
Also; its not immediately obvious
conf->copiesis 'small' and this doesn't blow up the stack; I feel that deserves a commentsomewhere.
Quoted 6 lines
Quoted 1 line
I agree that the code is horrible.
It is, in fact, exactly the same solution that was used to remove variable length arrays in structs from several of the crypto drivers
a
Quoted 3 lines
few years ago - see the definition of
SHASH_DESC_ON_STACK()in "crypto/hash.h" - I did not, however, hide the horrors in a macro preferring to leave the implementation visible as a warning towhoever
Quoted 3 lines
might touch the code next.
I believe that the actual stack usage is exactly the same as it was
previously.
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
I can certainly wrap this up in a macro and add comments with appropriately dire warnings in it if you feel that is both necessary and sufficient.
We got away with ugly in the past, so we should get to do it again?
Seriously, you should have taken the hack the first time that this needs to be fixed. Just because this is a fairly uncommon construct in the kernel doesn't mean it is not in userspace.
There is a reason why it is fairly uncommon in kernel. Initially it was used more widely, but then there was a decision to drop all uses of this feature. Namely:
Linus: "We should definitely drop it. The feature is an abomination".
https://lkml.org/lkml/2013/9/23/500
I really don't understand why you cling onto this last use of the feature. Having a single use of a compiler extension on an error path of a non-mandatory driver does not look like a great idea to me. Let's just kill it off outside of clang discussion.
Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array
Quoted 5 lines · Dmitry Vyukov
Quoted 3 lines
Seriously, you should have taken the hack the first time that this needs to be fixed. Just because this is a fairly uncommon construct in the kernel doesn't mean it is not in userspace.
There is a reason why it is fairly uncommon in kernel.
So first off; its not entirely clear that the code as it exists it correct. From a cursory reading of it and surrounding code, there is no actual upper limit on the variable. If I were stupid enough to make a raid with 64 devices I'd get a huge on-stack structure.
Since you're touching it; you should check these things.
And secondly, refactor the code to not look like dog vomit. You can do more than the absolute minimal patch to make it compile, I'm sure.
PATCH 7/7 crypto, x86, LLVM: aesni - fix token pasting
In reply to Michael Davidsonaes_ctrby8_avx-x86_64.S uses the C preprocessor for token pasting of character sequences that are not valid preprocessor tokens. While this is allowed when preprocessing assembler files it exposes an incompatibilty between the clang and gcc preprocessors where clang does not strip leading white space from macro parameters, leading to the CONCAT(%xmm, i) macro expansion on line 96 resulting in a token with a space character embedded in it.
While this could be fixed by deleting the offending space character, the assembler is perfectly capable of handling the token pasting correctly for itself so it seems preferable to let it do so and to get rid or the CONCAT(), DDQ() and XMM() preprocessor macros.
--- arch/x86/crypto/aes_ctrby8_avx-x86_64.S | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
arch/x86/crypto/aes_ctrby8_avx-x86_64.S +2 -5
@@ -65,7 +65,6 @@ #include <linux/linkage.h> #include <asm/inst.h> -#define CONCAT(a,b) a##b #define VMOVDQ vmovdqu #define xdata0 %xmm0
@@ -92,8 +91,6 @@ #define num_bytes %r8 #define tmp %r10 -#define DDQ(i) CONCAT(ddq_add_,i) -#define XMM(i) CONCAT(%xmm, i) #define DDQ_DATA 0 #define XDATA 1 #define KEY_128 1
@@ -131,12 +128,12 @@ ddq_add_8: /* generate a unique variable for ddq_add_x */ .macro setddq n - var_ddq_add = DDQ(\n) + var_ddq_add = ddq_add_\n .endm /* generate a unique variable for xmm register */ .macro setxdata n - var_xdata = XMM(\n) + var_xdata = %xmm\n .endm /* club the numeric 'id' to the symbol 'name' */
Signature · 1 line
2.12.0.367.g23dc2f6d3c-goog
Re: [PATCH 7/7] crypto, x86, LLVM: aesni - fix token pasting
El Thu, Mar 16, 2017 at 05:15:20PM -0700 Michael Davidson ha dit:
Quoted 44 lines
aes_ctrby8_avx-x86_64.Suses the C preprocessor for token pasting of character sequences that are not valid preprocessor tokens. While this is allowed when preprocessing assembler files it exposes an incompatibilty between the clang and gcc preprocessors where clang does not strip leading white space from macro parameters, leading to theCONCAT(%xmm, i)macro expansion on line 96 resulting in a token with a space character embedded in it.While this could be fixed by deleting the offending space character, the assembler is perfectly capable of handling the token pasting correctly for itself so it seems preferable to let it do so and to get rid or the
CONCAT(),DDQ()andXMM()preprocessor macros.Signed-off-by Michael Davidson <[email protected]>--- arch/x86/crypto/aes_ctrby8_avx-x86_64.S | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)arch/x86/crypto/aes_ctrby8_avx-x86_64.S +0
-4@@ -65,7 +65,6 @@ #include <linux/linkage.h> #include <asm/inst.h> -#define CONCAT(a,b) a##b #define VMOVDQ vmovdqu #define xdata0 %xmm0@@ -92,8 +91,6 @@ #define num_bytes %r8 #define tmp %r10 -#define DDQ(i) CONCAT(ddq_add_,i) -#define XMM(i) CONCAT(%xmm, i) #define DDQ_DATA 0 #define XDATA 1 #define KEY_128 1@@ -131,12 +128,12 @@ ddq_add_8: /* generate a unique variable for ddq_add_x */ .macro setddq n - var_ddq_add = DDQ(\n)
arch/x86/crypto/aes_ctrby8_avx-x86_64.S ddq_add_8: · line 139
Quoted 10 lines
arch/x86/crypto/aes_ctrby8_avx-x86_64.S +2
-1+ var_ddq_add = ddq_add_\n .endm /* generate a unique variable for xmm register */ .macro setxdata n - var_xdata = XMM(\n) + var_xdata = %xmm\n .endm /* club the numeric 'id' to the symbol 'name' */
Any feedback on this patch?
Thanks
Matthias
Quoted 8 lines · Michael Davidson
This patch set is sufficient to get the
x86_64kernel to build and boot correctly with clang-3.8 or greater.The resulting build still has about 300 warnings, very few of which appear to be significant. Most of them should be fixable with some minor code refactoring although a few of them, such as the complaints about implict conversions between enumerated types may be candidates for just being disabled.
Thanks, Michael!
This will help us a lot with KMSAN (uninit use detector) and code coverage.
Quoted 21 lines
Michael Davidson (7):
Makefile, LLVM: add -no-integrated-as to KBUILD_[AC]FLAGSMakefile, x86, LLVM: disable unsupported optimization flags x86, LLVM: suppress clang warnings about unaligned accesses x86, boot, LLVM: #undef memcpy etc instring.cx86, boot, LLVM: Use regparm=0 for memcpy and memset md/raid10, LLVM: get rid of variable length array crypto, x86, LLVM: aesni - fix token pastingMakefile | 4 ++++ arch/x86/Makefile | 7 +++++++ arch/x86/boot/copy.S | 15 +++++++++++++-- arch/x86/boot/string.c | 9 +++++++++ arch/x86/boot/string.h | 13 +++++++++++++ arch/x86/crypto/aes_ctrby8_avx-x86_64.S | 7 ++----- drivers/md/raid10.c | 9 ++++----- 7 files changed, 52 insertions(+), 12 deletions(-)--
2.12.0.367.g23dc2f6d3c-goog