| Message ID | 20170619183757.124992-4-mka@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On June 19, 2017 11:37:57 AM PDT, Matthias Kaehlcke <mka@chromium.org> wrote: >For gcc stack alignment is configured with >-mpreferred-stack-boundary=N, >clang has the option -mstack-alignment=N for that purpose. Use the same >alignment as with gcc. > >If the alignment is not specified clang assumes an alignment of >16 bytes, as required by the standard ABI. However as mentioned in >d9b0cde91c60 ("x86-64, gcc: Use -mpreferred-stack-boundary=3 if >supported") the standard kernel entry on x86-64 leaves the stack >on an 8-byte boundary, as a consequence clang will keep the stack >misaligned. > >Signed-off-by: Matthias Kaehlcke <mka@chromium.org> >--- > arch/x86/Makefile | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > >diff --git a/arch/x86/Makefile b/arch/x86/Makefile >index b2dae639f778..9406d3670452 100644 >--- a/arch/x86/Makefile >+++ b/arch/x86/Makefile >@@ -11,6 +11,14 @@ else > KBUILD_DEFCONFIG := $(ARCH)_defconfig > endif > >+# Handle different option names for specifying stack alignment with >gcc and >+# clang. >+ifeq ($(cc-name),clang) >+ cc_stack_align_opt := -mstack-alignment >+else >+ cc_stack_align_opt := -mpreferred-stack-boundary >+endif >+ ># How to compile the 16-bit code. Note we always compile for >-march=i386; > # that way we can complain to the user if the CPU is insufficient. > # >@@ -28,7 +36,7 @@ REALMODE_CFLAGS := $(M16_CFLAGS) -g -Os -D__KERNEL__ >\ > >REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), >-ffreestanding) >REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), >-fno-stack-protector) >-REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), >-mpreferred-stack-boundary=2) >+REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), >$(cc_stack_align_opt)=2) > export REALMODE_CFLAGS > > # BITS is used as extension for files which are available in a 32 bit >@@ -65,8 +73,10 @@ ifeq ($(CONFIG_X86_32),y) > # with nonstandard options > KBUILD_CFLAGS += -fno-pic > >- # prevent gcc from keeping the stack 16 byte aligned >- KBUILD_CFLAGS += $(call >cc-option,-mpreferred-stack-boundary=2) >+ # Align the stack to the register width instead of using the >default >+ # alignment of 16 bytes. This reduces stack usage and the >number of >+ # alignment instructions. >+ KBUILD_CFLAGS += $(call cc-option,$(cc_stack_align_opt)=2) > ># Disable unit-at-a-time mode on pre-gcc-4.0 compilers, it makes gcc >use > # a lot more stack due to the lack of sharing of stacklots: >@@ -98,8 +108,14 @@ else > KBUILD_CFLAGS += $(call cc-option,-mno-80387) > KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387) > >- # Use -mpreferred-stack-boundary=3 if supported. >- KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=3) >+ # By default gcc and clang use a stack alignment of 16 bytes >for x86. >+ # However the standard kernel entry on x86-64 leaves the stack >on an >+ # 8-byte boundary. If the compiler isn't informed about the >actual >+ # alignment it will generate extra alignment instructions for >the >+ # default alignment which keep the stack *mis*aligned. >+ # Furthermore an alignment to the register width reduces stack >usage >+ # and the number of alignment instructions. >+ KBUILD_CFLAGS += $(call cc-option,$(cc_stack_align_opt)=3) > > # Use -mskip-rax-setup if supported. > KBUILD_CFLAGS += $(call cc-option,-mskip-rax-setup) Goddammit. How many times do I have to say NAK to >+ifeq ($(cc-name),clang) ... before it sinks in?
El Mon, Jun 19, 2017 at 01:17:03PM -0700 hpa@zytor.com ha dit: > On June 19, 2017 11:37:57 AM PDT, Matthias Kaehlcke <mka@chromium.org> wrote: > >For gcc stack alignment is configured with > >-mpreferred-stack-boundary=N, > >clang has the option -mstack-alignment=N for that purpose. Use the same > >alignment as with gcc. > > > >If the alignment is not specified clang assumes an alignment of > >16 bytes, as required by the standard ABI. However as mentioned in > >d9b0cde91c60 ("x86-64, gcc: Use -mpreferred-stack-boundary=3 if > >supported") the standard kernel entry on x86-64 leaves the stack > >on an 8-byte boundary, as a consequence clang will keep the stack > >misaligned. > > > >Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > >--- > > arch/x86/Makefile | 26 +++++++++++++++++++++----- > > 1 file changed, 21 insertions(+), 5 deletions(-) > > > >diff --git a/arch/x86/Makefile b/arch/x86/Makefile > >index b2dae639f778..9406d3670452 100644 > >--- a/arch/x86/Makefile > >+++ b/arch/x86/Makefile > >@@ -11,6 +11,14 @@ else > > KBUILD_DEFCONFIG := $(ARCH)_defconfig > > endif > > > >+# Handle different option names for specifying stack alignment with > >gcc and > >+# clang. > >+ifeq ($(cc-name),clang) > >+ cc_stack_align_opt := -mstack-alignment > >+else > >+ cc_stack_align_opt := -mpreferred-stack-boundary > >+endif > >+ > ># How to compile the 16-bit code. Note we always compile for > >-march=i386; > > # that way we can complain to the user if the CPU is insufficient. > > # > >@@ -28,7 +36,7 @@ REALMODE_CFLAGS := $(M16_CFLAGS) -g -Os -D__KERNEL__ > >\ > > > >REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), > >-ffreestanding) > >REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), > >-fno-stack-protector) > >-REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), > >-mpreferred-stack-boundary=2) > >+REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), > >$(cc_stack_align_opt)=2) > > export REALMODE_CFLAGS > > > > # BITS is used as extension for files which are available in a 32 bit > >@@ -65,8 +73,10 @@ ifeq ($(CONFIG_X86_32),y) > > # with nonstandard options > > KBUILD_CFLAGS += -fno-pic > > > >- # prevent gcc from keeping the stack 16 byte aligned > >- KBUILD_CFLAGS += $(call > >cc-option,-mpreferred-stack-boundary=2) > >+ # Align the stack to the register width instead of using the > >default > >+ # alignment of 16 bytes. This reduces stack usage and the > >number of > >+ # alignment instructions. > >+ KBUILD_CFLAGS += $(call cc-option,$(cc_stack_align_opt)=2) > > > ># Disable unit-at-a-time mode on pre-gcc-4.0 compilers, it makes gcc > >use > > # a lot more stack due to the lack of sharing of stacklots: > >@@ -98,8 +108,14 @@ else > > KBUILD_CFLAGS += $(call cc-option,-mno-80387) > > KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387) > > > >- # Use -mpreferred-stack-boundary=3 if supported. > >- KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=3) > >+ # By default gcc and clang use a stack alignment of 16 bytes > >for x86. > >+ # However the standard kernel entry on x86-64 leaves the stack > >on an > >+ # 8-byte boundary. If the compiler isn't informed about the > >actual > >+ # alignment it will generate extra alignment instructions for > >the > >+ # default alignment which keep the stack *mis*aligned. > >+ # Furthermore an alignment to the register width reduces stack > >usage > >+ # and the number of alignment instructions. > >+ KBUILD_CFLAGS += $(call cc-option,$(cc_stack_align_opt)=3) > > > > # Use -mskip-rax-setup if supported. > > KBUILD_CFLAGS += $(call cc-option,-mskip-rax-setup) > > Goddammit. > > How many times do I have to say NAK to > > >+ifeq ($(cc-name),clang) > > ... before it sinks in? The initial version of this patch doesn't have this condition and just uses cc-option to select the appropriate option. Ingo didn't like the duplication and suggested the use of a variable, which kinda implies a check for the compiler name. I also think this is a cleaner solution. but I'm happy to respin the patch if you have another suggestion that is ok for both of you. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Matthias Kaehlcke <mka@chromium.org> wrote: > Ingo didn't like the duplication and suggested the use of a variable, which > kinda implies a check for the compiler name. I don't think it implies that: why cannot cc_stack_align_opt probe for the compiler option and use whichever is available, without hard-coding the compiler name? > I also think this is a cleaner solution. [...] I concur with hpa: hard-coding compiler is awfully fragile and ugly as well. With the proper probing of compiler options it will be possible for compilers to consolidate their options, and it would be possible for a third compiler to use a mixture of GCC and Clang options. With hard-coding none of that flexibility is available. > but I'm happy to respin the patch if you have another suggestion that is ok for > both of you. Please do. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
El Tue, Jun 20, 2017 at 11:20:54AM +0200 Ingo Molnar ha dit: > > * Matthias Kaehlcke <mka@chromium.org> wrote: > > > Ingo didn't like the duplication and suggested the use of a variable, which > > kinda implies a check for the compiler name. > > I don't think it implies that: why cannot cc_stack_align_opt probe for the > compiler option and use whichever is available, without hard-coding the compiler > name? We could do this: ifneq ($(call __cc-option, $(CC), -mno-sse, -mpreferred-stack-boundary=3,),) cc_stack_align_opt := -mpreferred-stack-boundary endif ifneq ($(call cc-option, -mstack-alignment=3,),) cc_stack_align_opt := -mstack-alignment endif If preferred cc-option could be used to probe for -mpreferred-stack-boundary , however it would require REALMODE_CFLAGS to be moved further down in the Makefile. Since this solution also won't win a beauty price please let me know if it is acceptable before respinning the patch or if you have other suggestions. > > I also think this is a cleaner solution. [...] > > I concur with hpa: hard-coding compiler is awfully fragile and ugly as well. > > With the proper probing of compiler options it will be possible for compilers to > consolidate their options, and it would be possible for a third compiler to use a > mixture of GCC and Clang options. With hard-coding none of that flexibility is > available. > > > but I'm happy to respin the patch if you have another suggestion that is ok for > > both of you. > > Please do. > > Thanks, > > Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Matthias Kaehlcke <mka@chromium.org> wrote: > El Tue, Jun 20, 2017 at 11:20:54AM +0200 Ingo Molnar ha dit: > > > > > * Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > Ingo didn't like the duplication and suggested the use of a variable, which > > > kinda implies a check for the compiler name. > > > > I don't think it implies that: why cannot cc_stack_align_opt probe for the > > compiler option and use whichever is available, without hard-coding the compiler > > name? > > We could do this: > > ifneq ($(call __cc-option, $(CC), -mno-sse, -mpreferred-stack-boundary=3,),) > cc_stack_align_opt := -mpreferred-stack-boundary > endif > ifneq ($(call cc-option, -mstack-alignment=3,),) > cc_stack_align_opt := -mstack-alignment > endif The principle Looks good to me - but I'd make the second probing an 'else' branch, i.e. probe for a suitable compiler option until we find one. That would also not burden the GCC build with probing for different compiler options. Please also add a comment in the code that explains that the first option is a GCC option and the second one is a Clang-ism. > Since this solution also won't win a beauty price please let me know > if it is acceptable before respinning the patch or if you have other > suggestions. This one already looks a lot cleaner to me than any of the previous ones. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/Makefile b/arch/x86/Makefile index b2dae639f778..9406d3670452 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -11,6 +11,14 @@ else KBUILD_DEFCONFIG := $(ARCH)_defconfig endif +# Handle different option names for specifying stack alignment with gcc and +# clang. +ifeq ($(cc-name),clang) + cc_stack_align_opt := -mstack-alignment +else + cc_stack_align_opt := -mpreferred-stack-boundary +endif + # How to compile the 16-bit code. Note we always compile for -march=i386; # that way we can complain to the user if the CPU is insufficient. # @@ -28,7 +36,7 @@ REALMODE_CFLAGS := $(M16_CFLAGS) -g -Os -D__KERNEL__ \ REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), -ffreestanding) REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), -fno-stack-protector) -REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), -mpreferred-stack-boundary=2) +REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS), $(cc_stack_align_opt)=2) export REALMODE_CFLAGS # BITS is used as extension for files which are available in a 32 bit @@ -65,8 +73,10 @@ ifeq ($(CONFIG_X86_32),y) # with nonstandard options KBUILD_CFLAGS += -fno-pic - # prevent gcc from keeping the stack 16 byte aligned - KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=2) + # Align the stack to the register width instead of using the default + # alignment of 16 bytes. This reduces stack usage and the number of + # alignment instructions. + KBUILD_CFLAGS += $(call cc-option,$(cc_stack_align_opt)=2) # Disable unit-at-a-time mode on pre-gcc-4.0 compilers, it makes gcc use # a lot more stack due to the lack of sharing of stacklots: @@ -98,8 +108,14 @@ else KBUILD_CFLAGS += $(call cc-option,-mno-80387) KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387) - # Use -mpreferred-stack-boundary=3 if supported. - KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=3) + # By default gcc and clang use a stack alignment of 16 bytes for x86. + # However the standard kernel entry on x86-64 leaves the stack on an + # 8-byte boundary. If the compiler isn't informed about the actual + # alignment it will generate extra alignment instructions for the + # default alignment which keep the stack *mis*aligned. + # Furthermore an alignment to the register width reduces stack usage + # and the number of alignment instructions. + KBUILD_CFLAGS += $(call cc-option,$(cc_stack_align_opt)=3) # Use -mskip-rax-setup if supported. KBUILD_CFLAGS += $(call cc-option,-mskip-rax-setup)
For gcc stack alignment is configured with -mpreferred-stack-boundary=N, clang has the option -mstack-alignment=N for that purpose. Use the same alignment as with gcc. If the alignment is not specified clang assumes an alignment of 16 bytes, as required by the standard ABI. However as mentioned in d9b0cde91c60 ("x86-64, gcc: Use -mpreferred-stack-boundary=3 if supported") the standard kernel entry on x86-64 leaves the stack on an 8-byte boundary, as a consequence clang will keep the stack misaligned. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- arch/x86/Makefile | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)