[PATCH v2 1/2] kbuild: Also evaluate alternative option passed to cc-option, etc

[PATCH v2 1/2] kbuild: Also evaluate alternative option passed to cc-option, etc

[Matthias Kaehlcke]

The macro cc-option receives two parameters (the second may be empty). It
returns the first parameter if it is a valid compiler option, otherwise
the second one. It is not evaluated if the second parameter is a valid
compiler option. This seems to be fine in virtually all cases, however
there are scenarios where the second parameter needs to be evaluated
too, and an empty value (or a third option) should be returned if it is
not valid.

The new macro try-run-opt receives a 'base command' and two options as
parameters. It runs 'base command' + option 1 and returns option 1
upon success. In case of failure 'base command' + option 2 is executed,
in case of success option 2 is returned, otherwise an empty string.

Rework [__]cc-option, ld-option, and cc-ldoption to use try-run-opt
instead of try-run to make sure the alternative option is evaluated.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- add try-run-opt instead of cc-option-3
- use try-run-opt for [__]cc-option, ld-option and ccld-option
- updated subject (was 'kbuild: Add macros cc-option-3 and __cc-option-3')
  and commit message

 scripts/Kbuild.include | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index dd8e2dde0b34..48a6d0e9b073 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -85,8 +85,8 @@ TMPOUT := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/)
 
 # try-run
 # Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
-# Exit code chooses option. "$$TMP" is can be used as temporary file and
-# is automatically cleaned up.
+# Runs the command $1, exit code chooses option. "$$TMP" is used as temporary
+# file and is automatically cleaned up.
 try-run = $(shell set -e;		\
 	TMP="$(TMPOUT).$$$$.tmp";	\
 	TMPO="$(TMPOUT).$$$$.o";	\
@@ -96,6 +96,23 @@ try-run = $(shell set -e;		\
 	fi;				\
 	rm -f "$$TMP" "$$TMPO")
 
+# try-run-opt
+# Usage: option = $(call try-run-opt, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Runs the command '$1 $2' and outputs $2 if the execution was successful.
+# Otherwise runs '$1 $3' (if $3 is not empty) and outputs $3 upon success. No
+# output if both commands fail. "$$TMP" is used as temporary file and is
+# automatically cleaned up.
+try-run-opt = $(shell set -e;           \
+        TMP="$(TMPOUT).$$$$.tmp";       \
+        TMPO="$(TMPOUT).$$$$.o";        \
+        if ($(1) $(2)) >/dev/null 2>&1; \
+        then echo "$(2)";               \
+        elif [ -n "$(3)" ] && ($(1) $(3)) >/dev/null 2>&1;      \
+        then echo "$(3)";               \
+        else echo "";                   \
+        fi;                             \
+        rm -f "$$TMP" "$$TMPO")
+
 # as-option
 # Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
 
@@ -110,8 +127,8 @@ as-instr = $(call try-run,\
 
 # __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))
+__cc-option = $(call try-run-opt,\
+	$(1) -Werror $(2) -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.)
@@ -159,13 +176,13 @@ cc-if-fullversion = $(shell [ $(cc-fullversion) $(1) $(2) ] && echo $(3) || echo
 
 # cc-ldoption
 # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
-cc-ldoption = $(call try-run,\
-	$(CC) $(1) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2))
+cc-ldoption = $(call try-run-opt,\
+	$(CC) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2))
 
 # ld-option
 # Usage: LDFLAGS += $(call ld-option, -X)
-ld-option = $(call try-run,\
-	$(CC) -x c /dev/null -c -o "$$TMPO" ; $(LD) $(1) "$$TMPO" -o "$$TMP",$(1),$(2))
+ld-option = $(call try-run-opt,\
+	$(CC) -x c /dev/null -c -o "$$TMPO" ; $(LD) "$$TMPO" -o "$$TMP",$(1),$(2))
 
 # ar-option
 # Usage: KBUILD_ARFLAGS := $(call ar-option,D)
-- 
2.14.0.434.g98096fd7a8-goog

[PATCH v2 2/2] x86/build: Fix stack alignment for CLang

[Matthias Kaehlcke]

Commit:

  d77698df39a5 ("x86/build: Specify stack alignment for clang")

intended to use the same stack alignment for clang as with gcc.

The two compilers use different options to configure the stack alignment
(gcc: -mpreferred-stack-boundary=n, clang: -mstack-alignment=n).

The above commit assumes that the clang option uses the same parameter
type as gcc, i.e. that the alignment is specified as 2^n. However clang
interprets the value of this option literally to use an alignment of n,
in consequence the stack remains misaligned.

Change the values used with -mstack-alignment to be the actual alignment
instead of a power of two.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- use the reworked [__]cc-option instead of cc-option-3, the macro now
  also evaluates the alternative option 

 arch/x86/Makefile | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 1e902f926be3..c7b25ba2d2fa 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -11,14 +11,6 @@ else
         KBUILD_DEFCONFIG := $(ARCH)_defconfig
 endif
 
-# For gcc stack alignment is specified with -mpreferred-stack-boundary,
-# clang has the option -mstack-alignment for that purpose.
-ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
-        cc_stack_align_opt := -mpreferred-stack-boundary
-else ifneq ($(call cc-option, -mstack-alignment=4),)
-        cc_stack_align_opt := -mstack-alignment
-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.
 #
@@ -36,7 +28,9 @@ 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), $(cc_stack_align_opt)=2)
+REALMODE_CFLAGS += $(call __cc-option, $(CC), $(REALMODE_CFLAGS),\
+	-mpreferred-stack-boundary=2,-mstack-alignment=4)
+
 export REALMODE_CFLAGS
 
 # BITS is used as extension for files which are available in a 32 bit
@@ -76,7 +70,7 @@ ifeq ($(CONFIG_X86_32),y)
         # 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)
+        KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=2,-mstack-alignment=4)
 
         # 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:
@@ -115,7 +109,7 @@ else
         # 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)
+        KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=3,-mstack-alignment=8)
 
 	# Use -mskip-rax-setup if supported.
 	KBUILD_CFLAGS += $(call cc-option,-mskip-rax-setup)
-- 
2.14.0.434.g98096fd7a8-goog

Re: [PATCH v2 1/2] kbuild: Also evaluate alternative option passed to cc-option, etc

[Masahiro Yamada]

Hi Matthias,


2017-08-15 10:29 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> The macro cc-option receives two parameters (the second may be empty). It
> returns the first parameter if it is a valid compiler option, otherwise
> the second one. It is not evaluated if the second parameter is a valid
> compiler option. This seems to be fine in virtually all cases, however
> there are scenarios where the second parameter needs to be evaluated
> too, and an empty value (or a third option) should be returned if it is
> not valid.
>
> The new macro try-run-opt receives a 'base command' and two options as
> parameters. It runs 'base command' + option 1 and returns option 1
> upon success. In case of failure 'base command' + option 2 is executed,
> in case of success option 2 is returned, otherwise an empty string.
>
> Rework [__]cc-option, ld-option, and cc-ldoption to use try-run-opt
> instead of try-run to make sure the alternative option is evaluated.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - add try-run-opt instead of cc-option-3
> - use try-run-opt for [__]cc-option, ld-option and ccld-option
> - updated subject (was 'kbuild: Add macros cc-option-3 and __cc-option-3')
>   and commit message
>
>  scripts/Kbuild.include | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index dd8e2dde0b34..48a6d0e9b073 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -85,8 +85,8 @@ TMPOUT := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/)
>
>  # try-run
>  # Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
> -# Exit code chooses option. "$$TMP" is can be used as temporary file and
> -# is automatically cleaned up.
> +# Runs the command $1, exit code chooses option. "$$TMP" is used as temporary
> +# file and is automatically cleaned up.
>  try-run = $(shell set -e;              \
>         TMP="$(TMPOUT).$$$$.tmp";       \
>         TMPO="$(TMPOUT).$$$$.o";        \
> @@ -96,6 +96,23 @@ try-run = $(shell set -e;            \
>         fi;                             \
>         rm -f "$$TMP" "$$TMPO")
>
> +# try-run-opt
> +# Usage: option = $(call try-run-opt, $(CC)...-o "$$TMP",option-ok,otherwise)
> +# Runs the command '$1 $2' and outputs $2 if the execution was successful.
> +# Otherwise runs '$1 $3' (if $3 is not empty) and outputs $3 upon success. No
> +# output if both commands fail. "$$TMP" is used as temporary file and is
> +# automatically cleaned up.
> +try-run-opt = $(shell set -e;           \
> +        TMP="$(TMPOUT).$$$$.tmp";       \
> +        TMPO="$(TMPOUT).$$$$.o";        \
> +        if ($(1) $(2)) >/dev/null 2>&1; \
> +        then echo "$(2)";               \
> +        elif [ -n "$(3)" ] && ($(1) $(3)) >/dev/null 2>&1;      \
> +        then echo "$(3)";               \
> +        else echo "";                   \
> +        fi;                             \
> +        rm -f "$$TMP" "$$TMPO")
> +
>  # as-option
>  # Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
>
> @@ -110,8 +127,8 @@ as-instr = $(call try-run,\
>
>  # __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))
> +__cc-option = $(call try-run-opt,\
> +       $(1) -Werror $(2) -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.)
> @@ -159,13 +176,13 @@ cc-if-fullversion = $(shell [ $(cc-fullversion) $(1) $(2) ] && echo $(3) || echo
>
>  # cc-ldoption
>  # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
> -cc-ldoption = $(call try-run,\
> -       $(CC) $(1) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2))
> +cc-ldoption = $(call try-run-opt,\
> +       $(CC) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2))
>
>  # ld-option
>  # Usage: LDFLAGS += $(call ld-option, -X)
> -ld-option = $(call try-run,\
> -       $(CC) -x c /dev/null -c -o "$$TMPO" ; $(LD) $(1) "$$TMPO" -o "$$TMP",$(1),$(2))
> +ld-option = $(call try-run-opt,\
> +       $(CC) -x c /dev/null -c -o "$$TMPO" ; $(LD) "$$TMPO" -o "$$TMP",$(1),$(2))
>
>  # ar-option
>  # Usage: KBUILD_ARFLAGS := $(call ar-option,D)


Thanks for sending the patch,
but I am reluctant to add this to the common script
after consideration.  Sorry.

Currently, *-option and try-run are quite simple
and work for almost all cases.

The stack alignment option is one of rare scenarios, I think.
If we have more and more cases that need the second parameter evaluation,
we will need to support it in the common script somehow.

Otherwise, could you solve the problem in local Makefile?

(I was wondering if we can support multiple evaluation in a generic
try-run implementation,
for example using "for ... do ... done" loop of shell script,
but it would introduce much complexity.)



For example, one very ad-hoc solution:

# For gcc stack alignment is specified with -mpreferred-stack-boundary,
# clang has the option -mstack-alignment for that purpose.
ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
      cc_stack_align4 := -mpreferred-stack-boundary=2
      cc_stack_align8 := -mpreferred-stack-boundary=3
else
      cc_stack_align4 := -mstack-alignment=4
      cc_stack_align8 := -mstack-alignment=8
endif




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 1/2] kbuild: Also evaluate alternative option passed to cc-option, etc

[Matthias Kaehlcke]

El Thu, Aug 17, 2017 at 12:03:05AM +0900 Masahiro Yamada ha dit:

> Hi Matthias,
> 
> 
> 2017-08-15 10:29 GMT+09:00 Matthias Kaehlcke <mka@chromium.org>:
> > The macro cc-option receives two parameters (the second may be empty). It
> > returns the first parameter if it is a valid compiler option, otherwise
> > the second one. It is not evaluated if the second parameter is a valid
> > compiler option. This seems to be fine in virtually all cases, however
> > there are scenarios where the second parameter needs to be evaluated
> > too, and an empty value (or a third option) should be returned if it is
> > not valid.
> >
> > The new macro try-run-opt receives a 'base command' and two options as
> > parameters. It runs 'base command' + option 1 and returns option 1
> > upon success. In case of failure 'base command' + option 2 is executed,
> > in case of success option 2 is returned, otherwise an empty string.
> >
> > Rework [__]cc-option, ld-option, and cc-ldoption to use try-run-opt
> > instead of try-run to make sure the alternative option is evaluated.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v2:
> > - add try-run-opt instead of cc-option-3
> > - use try-run-opt for [__]cc-option, ld-option and ccld-option
> > - updated subject (was 'kbuild: Add macros cc-option-3 and __cc-option-3')
> >   and commit message
> >
> >  scripts/Kbuild.include | 33 +++++++++++++++++++++++++--------
> >  1 file changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> > index dd8e2dde0b34..48a6d0e9b073 100644
> > --- a/scripts/Kbuild.include
> > +++ b/scripts/Kbuild.include
> > @@ -85,8 +85,8 @@ TMPOUT := $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/)
> >
> >  # try-run
> >  # Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
> > -# Exit code chooses option. "$$TMP" is can be used as temporary file and
> > -# is automatically cleaned up.
> > +# Runs the command $1, exit code chooses option. "$$TMP" is used as temporary
> > +# file and is automatically cleaned up.
> >  try-run = $(shell set -e;              \
> >         TMP="$(TMPOUT).$$$$.tmp";       \
> >         TMPO="$(TMPOUT).$$$$.o";        \
> > @@ -96,6 +96,23 @@ try-run = $(shell set -e;            \
> >         fi;                             \
> >         rm -f "$$TMP" "$$TMPO")
> >
> > +# try-run-opt
> > +# Usage: option = $(call try-run-opt, $(CC)...-o "$$TMP",option-ok,otherwise)
> > +# Runs the command '$1 $2' and outputs $2 if the execution was successful.
> > +# Otherwise runs '$1 $3' (if $3 is not empty) and outputs $3 upon success. No
> > +# output if both commands fail. "$$TMP" is used as temporary file and is
> > +# automatically cleaned up.
> > +try-run-opt = $(shell set -e;           \
> > +        TMP="$(TMPOUT).$$$$.tmp";       \
> > +        TMPO="$(TMPOUT).$$$$.o";        \
> > +        if ($(1) $(2)) >/dev/null 2>&1; \
> > +        then echo "$(2)";               \
> > +        elif [ -n "$(3)" ] && ($(1) $(3)) >/dev/null 2>&1;      \
> > +        then echo "$(3)";               \
> > +        else echo "";                   \
> > +        fi;                             \
> > +        rm -f "$$TMP" "$$TMPO")
> > +
> >  # as-option
> >  # Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
> >
> > @@ -110,8 +127,8 @@ as-instr = $(call try-run,\
> >
> >  # __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))
> > +__cc-option = $(call try-run-opt,\
> > +       $(1) -Werror $(2) -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.)
> > @@ -159,13 +176,13 @@ cc-if-fullversion = $(shell [ $(cc-fullversion) $(1) $(2) ] && echo $(3) || echo
> >
> >  # cc-ldoption
> >  # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
> > -cc-ldoption = $(call try-run,\
> > -       $(CC) $(1) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2))
> > +cc-ldoption = $(call try-run-opt,\
> > +       $(CC) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2))
> >
> >  # ld-option
> >  # Usage: LDFLAGS += $(call ld-option, -X)
> > -ld-option = $(call try-run,\
> > -       $(CC) -x c /dev/null -c -o "$$TMPO" ; $(LD) $(1) "$$TMPO" -o "$$TMP",$(1),$(2))
> > +ld-option = $(call try-run-opt,\
> > +       $(CC) -x c /dev/null -c -o "$$TMPO" ; $(LD) "$$TMPO" -o "$$TMP",$(1),$(2))
> >
> >  # ar-option
> >  # Usage: KBUILD_ARFLAGS := $(call ar-option,D)
> 
> 
> Thanks for sending the patch,
> but I am reluctant to add this to the common script
> after consideration.  Sorry.
> 
> Currently, *-option and try-run are quite simple
> and work for almost all cases.
> 
> The stack alignment option is one of rare scenarios, I think.

Yes, it's clearly not a common case.

> If we have more and more cases that need the second parameter evaluation,
> we will need to support it in the common script somehow.
> 
> Otherwise, could you solve the problem in local Makefile?

It can definitely be solved in the Makefile, another question is
whether the maintainers of that Makefile accept the solution. I'll
give it a try.

> (I was wondering if we can support multiple evaluation in a generic
> try-run implementation,
> for example using "for ... do ... done" loop of shell script,
> but it would introduce much complexity.)
> 
> 
> 
> For example, one very ad-hoc solution:
> 
> # For gcc stack alignment is specified with -mpreferred-stack-boundary,
> # clang has the option -mstack-alignment for that purpose.
> ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
>       cc_stack_align4 := -mpreferred-stack-boundary=2
>       cc_stack_align8 := -mpreferred-stack-boundary=3
> else
>       cc_stack_align4 := -mstack-alignment=4
>       cc_stack_align8 := -mstack-alignment=8
> endif

Thanks for the suggestion, something like this should work, with an
additional cc-option check in the else branch, for the case of older
gcc versions without support for the -mpreferred-stack-boundary
option.