[PATCH v4 0/3] x86: stack alignment for boot code and clang

[PATCH v4 0/3] x86: stack alignment for boot code and clang

[Matthias Kaehlcke]

This series fixes an issue with the stack of the x86 boot code not
being aligned as intended. Further it adapts the Makefile to account
for the fact that clang uses a different option to configure the
stack alignment than gcc (-mstack-alignment=N vs
-mpreferred-stack-boundary=N)

Collaterally the series adds the new kbuild macro __cc-option and
refactors the macros cc-option and hostcc-option to make use of
__cc-option.

Matthias Kaehlcke (3):
  kbuild: Add __cc-option macro
  x86/build: Use __cc-option for boot code compiler options
  x86/build: Specify stack alignment for clang

 Makefile               |  2 +-
 arch/x86/Makefile      | 33 +++++++++++++++++++++++++--------
 scripts/Kbuild.include | 14 ++++++++++++--
 scripts/Makefile.host  |  6 ------
 4 files changed, 38 insertions(+), 17 deletions(-)

-- 
2.13.1.518.g3df882009-goog

[PATCH v4 1/3] kbuild: Add __cc-option macro

[Matthias Kaehlcke]

cc-option uses KBUILD_CFLAGS and KBUILD_CPPFLAGS when it determines
whether an option is supported or not. This is fine for options used to
build the kernel itself, however some components like the x86 boot code
use a different set of flags.

Add the new macro __cc-option which is a more generic version of
cc-option with additional parameters. One parameter is the compiler
with which the check should be performed, the other the compiler options
to be used instead KBUILD_C*FLAGS.

Refactor cc-option and hostcc-option to use __cc-option and move
hostcc-option to scripts/Kbuild.include.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v4:
- Remove extra space before alternative option in cc-option and
  hostcc-option

 Makefile               |  2 +-
 scripts/Kbuild.include | 14 ++++++++++++--
 scripts/Makefile.host  |  6 ------
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index 83f6d9972cab..b234bba6d652 100644
--- a/Makefile
+++ b/Makefile
@@ -303,7 +303,7 @@ CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
 
 HOSTCC       = gcc
 HOSTCXX      = g++
-HOSTCFLAGS   = -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 -fomit-frame-pointer -std=gnu89
+HOSTCFLAGS   := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 -fomit-frame-pointer -std=gnu89
 HOSTCXXFLAGS = -O2
 
 ifeq ($(shell $(HOSTCC) -v 2>&1 | grep -c "clang version"), 1)
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 61f87a99bf0a..81a58d1f53af 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -108,6 +108,11 @@ as-option = $(call try-run,\
 as-instr = $(call try-run,\
 	printf "%b\n" "$(1)" | $(CC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
 
+# __cc-option
+# Usage: MY_CFLAGS += $(call __cc-option,$(CC),$(MY_CFLAGS),-march=winchip-c6,-march=i586)
+__cc-option = $(call try-run,\
+	$(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))
+
 # Do not attempt to build with gcc plugins during cc-option tests.
 # (And this uses delayed resolution so the flags will be up to date.)
 CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
@@ -115,8 +120,13 @@ CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
 # cc-option
 # Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
 
-cc-option = $(call try-run,\
-	$(CC) -Werror $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
+cc-option = $(call __cc-option, $(CC), $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS),\
+	$(1),$(2))
+
+# hostcc-option
+# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)
+hostcc-option = $(call __cc-option, $(HOSTCC),\
+	$(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))
 
 # cc-option-yn
 # Usage: flag := $(call cc-option-yn,-march=winchip-c6)
diff --git a/scripts/Makefile.host b/scripts/Makefile.host
index 45b5b1aaedbd..9cfd5c84d76f 100644
--- a/scripts/Makefile.host
+++ b/scripts/Makefile.host
@@ -20,12 +20,6 @@
 # Will compile qconf as a C++ program, and menu as a C program.
 # They are linked as C++ code to the executable qconf
 
-# hostcc-option
-# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)
-
-hostcc-option = $(call try-run,\
-	$(HOSTCC) $(HOSTCFLAGS) $(HOST_EXTRACFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
-
 __hostprogs := $(sort $(hostprogs-y) $(hostprogs-m))
 host-cshlib := $(sort $(hostlibs-y) $(hostlibs-m))
 host-cxxshlib := $(sort $(hostcxxlibs-y) $(hostcxxlibs-m))
-- 
2.13.1.518.g3df882009-goog

[PATCH v4 2/3] x86/build: Use __cc-option for boot code compiler options

[Matthias Kaehlcke]

cc-option is used to enable compiler options for the boot code if they
are available. The macro uses KBUILD_CFLAGS and KBUILD_CPPFLAGS for the
check, however these flags aren't used to build the boot code, in
consequence cc-option can yield wrong results. For example
-mpreferred-stack-boundary=2 is never set with a 64-bit compiler,
since the setting is only valid for 16 and 32-bit binaries. This
is also the case for 32-bit kernel builds, because the option -m32 is
added to KBUILD_CFLAGS after the assignment of REALMODE_CFLAGS.

Use __cc-option instead of cc-option for the boot mode options.
The macro receives the compiler options as parameter instead of using
KBUILD_C*FLAGS, for the boot code we pass REALMODE_CFLAGS.

Also use separate statements for the __cc-option checks instead
of performing them in the initial assignment of REALMODE_CFLAGS since
the variable is an input of the macro.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/x86/Makefile | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index bf240b920473..b2dae639f778 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -24,10 +24,11 @@ REALMODE_CFLAGS	:= $(M16_CFLAGS) -g -Os -D__KERNEL__ \
 		   -DDISABLE_BRANCH_PROFILING \
 		   -Wall -Wstrict-prototypes -march=i386 -mregparm=3 \
 		   -fno-strict-aliasing -fomit-frame-pointer -fno-pic \
-		   -mno-mmx -mno-sse \
-		   $(call cc-option, -ffreestanding) \
-		   $(call cc-option, -fno-stack-protector) \
-		   $(call cc-option, -mpreferred-stack-boundary=2)
+		   -mno-mmx -mno-sse
+
+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)
 export REALMODE_CFLAGS
 
 # BITS is used as extension for files which are available in a 32 bit
-- 
2.13.1.518.g3df882009-goog

[PATCH v4 3/3] x86/build: Specify stack alignment for clang

[Matthias Kaehlcke]

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)
-- 
2.13.1.518.g3df882009-goog

Re: [PATCH v4 3/3] x86/build: Specify stack alignment for clang

[hpa]

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?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v4 3/3] x86/build: Specify stack alignment for clang

[Matthias Kaehlcke]

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.

Re: [PATCH v4 3/3] x86/build: Specify stack alignment for clang

[Ingo Molnar]

* 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

Re: [PATCH v4 1/3] kbuild: Add __cc-option macro

[Masahiro Yamada]

Hi Matthias,

2017-06-20 3:37 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> cc-option uses KBUILD_CFLAGS and KBUILD_CPPFLAGS when it determines
> whether an option is supported or not. This is fine for options used to
> build the kernel itself, however some components like the x86 boot code
> use a different set of flags.
>
> Add the new macro __cc-option which is a more generic version of
> cc-option with additional parameters. One parameter is the compiler
> with which the check should be performed, the other the compiler options
> to be used instead KBUILD_C*FLAGS.
>
> Refactor cc-option and hostcc-option to use __cc-option and move
> hostcc-option to scripts/Kbuild.include.
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> Changes in v4:
> - Remove extra space before alternative option in cc-option and
>   hostcc-option
>
>  Makefile               |  2 +-
>  scripts/Kbuild.include | 14 ++++++++++++--
>  scripts/Makefile.host  |  6 ------
>  3 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 83f6d9972cab..b234bba6d652 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -303,7 +303,7 @@ CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
>
>  HOSTCC       = gcc
>  HOSTCXX      = g++
> -HOSTCFLAGS   = -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 -fomit-frame-pointer -std=gnu89
> +HOSTCFLAGS   := -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 -fomit-frame-pointer -std=gnu89
>  HOSTCXXFLAGS = -O2
>
>  ifeq ($(shell $(HOSTCC) -v 2>&1 | grep -c "clang version"), 1)
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index 61f87a99bf0a..81a58d1f53af 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -108,6 +108,11 @@ as-option = $(call try-run,\
>  as-instr = $(call try-run,\
>         printf "%b\n" "$(1)" | $(CC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
>
> +# __cc-option
> +# Usage: MY_CFLAGS += $(call __cc-option,$(CC),$(MY_CFLAGS),-march=winchip-c6,-march=i586)
> +__cc-option = $(call try-run,\
> +       $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))
> +
>  # Do not attempt to build with gcc plugins during cc-option tests.
>  # (And this uses delayed resolution so the flags will be up to date.)
>  CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
> @@ -115,8 +120,13 @@ CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
>  # cc-option
>  # Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
>
> -cc-option = $(call try-run,\
> -       $(CC) -Werror $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
> +cc-option = $(call __cc-option, $(CC), $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS),\
> +       $(1),$(2))


I think this will introduce an extra tab for the true case.

Could you wrap the line after $(CC),?

cc-option = $(call __cc-option, $(CC),\
       $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS),$(1),$(2))


I would not have requested v5 only for this,
but it looks like you have a chance for re-spin to improve 3/3.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 3/3] x86/build: Specify stack alignment for clang

[Matthias Kaehlcke]

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

Re: [PATCH v4 3/3] x86/build: Specify stack alignment for clang

[Ingo Molnar]

* 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