diff mbox series

kbuild/x86: Use $(KBUILD_AFLAGS) in Kbuild's version of $(as-instr)

Message ID 20240411-as-instr-opt-wrussq-v1-1-99b1a1a99f93@gmail.com (mailing list archive)
State New
Headers show
Series kbuild/x86: Use $(KBUILD_AFLAGS) in Kbuild's version of $(as-instr) | expand

Commit Message

Dmitry Safonov via B4 Relay April 10, 2024, 11:24 p.m. UTC 
From: Dmitry Safonov <0x7f454c46@gmail.com>

At Arista some products use compatible 32-bit userspace running on x86.
As a part of disto build for ia32 it also compiles the 64-bit kernel.
While the toolchain for the kernel build is yet the same, with 64-bit gcc:
> / @Bru-na-Boinne% file /usr/bin/gcc-11
> /usr/bin/gcc-11: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=6571ad50d8f12eece053f1bac7a95a2c767f32c9, for GNU/Linux 3.2.0, stripped

It seems that gcc is being smart and detects that it's running in
a 32-bit container (personality flag? 32-bit mmap base? something else
inherited post-exec?  haven't yet figured it out) and by default tries
to build 32-bit binaries.

That results in a failing toolchain check:
> / @Bru-na-Boinne% printf "%b\n" "wrussq %rax, (%rbx)" | /usr/bin/gcc-11 -Wa,--fatal-warnings -c -x assembler-with-cpp -o /dev/null -
> <stdin>: Assembler messages:
> <stdin>:1: Error: `wrussq' is only supported in 64-bit mode

Which passes when -m64 is directly specify for the build check:
> / @Bru-na-Boinne% printf "%b\n" "wrussq %rax, (%rbx)" | /usr/bin/gcc-11 -m64 -Wa,--fatal-warnings -c -x assembler-with-cpp -o /dev/null -
> / @Bru-na-Boinne% echo $?
> 0

As a result, kbuild produces different value for CONFIG_AS_WRUSS
for native 64-bit containers and ia32 containers with 64-bit gcc,
which produces different kernels with enabled/disabled
CONFIG_X86_USER_SHADOW_STACK.

arch/x86/Makefile already properly defines KBUILD_AFLAGS += -m64,
which is luckly already available at the point of toolchain check
in arch/x86/Kconfig.assembler

By hacking around Kbuild variable the following way:
> --- a/scripts/Kconfig.include
> +++ b/scripts/Kconfig.include
> @@ -13,7 +13,8 @@ left_paren  := (
>
>  # $(if-success,<command>,<then>,<else>)
>  # Return <then> if <command> exits with 0, <else> otherwise.
> -if-success = $(shell,{ $(1); } >/dev/null 2>&1 && echo "$(2)" || echo "$(3)")
> +if-success = $(shell,echo '$(1)' 1>&2;{ $(1); } >/dev/null 2>&1 && echo "$(2)" || echo "$(3)")

I got the following output for the toolchain check, before:
> linux @Bru-na-Boinne% make ARCH=x86_64 oldconfig V=1 2>&1 | grep wrus
> printf "%b\n" "wrussq %rax,(%rbx)" | gcc  -c -x assembler-with-cpp -o /dev/null -

and after:
> linux @Bru-na-Boinne% make ARCH=x86_64 oldconfig V=1 2>&1 | grep wrus
> printf "%b\n" "wrussq %rax,(%rbx)" | gcc -D__ASSEMBLY__ -fno-PIE -m64 -c -x assembler-with-cpp -o /dev/null -

Which seems appropriate to me.
This also reflects the existing definition in scripts/Makefile.compiler
for $(as-instr) that already has $(KBUILD_AFLAGS).

Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
---
 scripts/Kconfig.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: 2c71fdf02a95b3dd425b42f28fd47fb2b1d22702
change-id: 20240410-as-instr-opt-wrussq-48ec37e36204

Best regards,

Comments

Dmitry Safonov April 10, 2024, 11:41 p.m. UTC | #1 
Please, ignore this version.

the previous one was to add a helper function and that seemed to work,
but on rework to actually use KBUILD_AFLAGS instead of adding a new
function I certainly haven't tested it enough.
Going to prepare version 2, sorry for the noise.

On Thu, 11 Apr 2024 at 00:24, Dmitry Safonov via B4 Relay
<devnull+0x7f454c46.gmail.com@kernel.org> wrote:
>
> From: Dmitry Safonov <0x7f454c46@gmail.com>
>
> At Arista some products use compatible 32-bit userspace running on x86.
> As a part of disto build for ia32 it also compiles the 64-bit kernel.
> While the toolchain for the kernel build is yet the same, with 64-bit gcc:
> > / @Bru-na-Boinne% file /usr/bin/gcc-11
> > /usr/bin/gcc-11: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=6571ad50d8f12eece053f1bac7a95a2c767f32c9, for GNU/Linux 3.2.0, stripped
>
> It seems that gcc is being smart and detects that it's running in
> a 32-bit container (personality flag? 32-bit mmap base? something else
> inherited post-exec?  haven't yet figured it out) and by default tries
> to build 32-bit binaries.
>
> That results in a failing toolchain check:
> > / @Bru-na-Boinne% printf "%b\n" "wrussq %rax, (%rbx)" | /usr/bin/gcc-11 -Wa,--fatal-warnings -c -x assembler-with-cpp -o /dev/null -
> > <stdin>: Assembler messages:
> > <stdin>:1: Error: `wrussq' is only supported in 64-bit mode
>
> Which passes when -m64 is directly specify for the build check:
> > / @Bru-na-Boinne% printf "%b\n" "wrussq %rax, (%rbx)" | /usr/bin/gcc-11 -m64 -Wa,--fatal-warnings -c -x assembler-with-cpp -o /dev/null -
> > / @Bru-na-Boinne% echo $?
> > 0
>
> As a result, kbuild produces different value for CONFIG_AS_WRUSS
> for native 64-bit containers and ia32 containers with 64-bit gcc,
> which produces different kernels with enabled/disabled
> CONFIG_X86_USER_SHADOW_STACK.
>
> arch/x86/Makefile already properly defines KBUILD_AFLAGS += -m64,
> which is luckly already available at the point of toolchain check
> in arch/x86/Kconfig.assembler
>
> By hacking around Kbuild variable the following way:
> > --- a/scripts/Kconfig.include
> > +++ b/scripts/Kconfig.include
> > @@ -13,7 +13,8 @@ left_paren  := (
> >
> >  # $(if-success,<command>,<then>,<else>)
> >  # Return <then> if <command> exits with 0, <else> otherwise.
> > -if-success = $(shell,{ $(1); } >/dev/null 2>&1 && echo "$(2)" || echo "$(3)")
> > +if-success = $(shell,echo '$(1)' 1>&2;{ $(1); } >/dev/null 2>&1 && echo "$(2)" || echo "$(3)")
>
> I got the following output for the toolchain check, before:
> > linux @Bru-na-Boinne% make ARCH=x86_64 oldconfig V=1 2>&1 | grep wrus
> > printf "%b\n" "wrussq %rax,(%rbx)" | gcc  -c -x assembler-with-cpp -o /dev/null -
>
> and after:
> > linux @Bru-na-Boinne% make ARCH=x86_64 oldconfig V=1 2>&1 | grep wrus
> > printf "%b\n" "wrussq %rax,(%rbx)" | gcc -D__ASSEMBLY__ -fno-PIE -m64 -c -x assembler-with-cpp -o /dev/null -
>
> Which seems appropriate to me.
> This also reflects the existing definition in scripts/Makefile.compiler
> for $(as-instr) that already has $(KBUILD_AFLAGS).
>
> Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
> ---
>  scripts/Kconfig.include | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
> index 3ee8ecfb8c04..d8574c1faf2d 100644
> --- a/scripts/Kconfig.include
> +++ b/scripts/Kconfig.include
> @@ -33,7 +33,7 @@ ld-option = $(success,$(LD) -v $(1))
>
>  # $(as-instr,<instr>)
>  # Return y if the assembler supports <instr>, n otherwise
> -as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -Wa$(comma)--fatal-warnings -c -x assembler-with-cpp -o /dev/null -)
> +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) $(KBUILD_AFLAGS) -Wa$(comma)--fatal-warnings -c -x assembler-with-cpp -o /dev/null -)
>
>  # check if $(CC) and $(LD) exist
>  $(error-if,$(failure,command -v $(CC)),C compiler '$(CC)' not found)
>
> ---
> base-commit: 2c71fdf02a95b3dd425b42f28fd47fb2b1d22702
> change-id: 20240410-as-instr-opt-wrussq-48ec37e36204
>
> Best regards,
> --
> Dmitry Safonov <0x7f454c46@gmail.com>
>
>
Dmitry Safonov April 11, 2024, 4:34 a.m. UTC | #2 
On Thu, 11 Apr 2024 at 00:41, Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>
> Please, ignore this version.
>
> the previous one was to add a helper function and that seemed to work,
> but on rework to actually use KBUILD_AFLAGS instead of adding a new
> function I certainly haven't tested it enough.
> Going to prepare version 2, sorry for the noise.

Yeah, in the end I debugged why it doesn't work, for the same machine
as in the patch message:
$ grep KBUILD_AFLAGS include/config/auto.conf.cmd
ifneq "$(KBUILD_AFLAGS)" "-D__ASSEMBLY__ -fno-PIE -m64 "

Notice the trailing space? That's the thing that doesn't match with
the $(KBUILD_AFLAGS) in top-level Makefile.

Adding some more debug:

> --- a/scripts/kconfig/Makefile
> +++ b/scripts/kconfig/Makefile
> @@ -80,8 +80,10 @@ simple-targets := oldconfig allnoconfig allyesconfig allmodconfig \
>         helpnewconfig yes2modconfig mod2yesconfig mod2noconfig
>
>
>  PHONY += $(simple-targets)
> +$(info KBUILD_AFLAGS is "$(KBUILD_AFLAGS)")
>
>  $(simple-targets): $(obj)/conf
> +       $(info KBUILD_AFLAGS for $@ "$(KBUILD_AFLAGS)")
>         $(Q)$< $(silent) --$@ $(Kconfig)
>
>  PHONY += savedefconfig defconfig
> --- a/scripts/kconfig/preprocess.c
> +++ b/scripts/kconfig/preprocess.c
> @@ -79,6 +79,8 @@ static char *env_expand(const char *name)
>         if (!value)
>                 return NULL;
>
> +       fprintf(stderr, "\tvariable '%s' = '%s'\n", name, value);
> +
>         /*
>          * We need to remember all referenced environment variables.
>          * They will be written out to include/config/auto.conf.cmd

Results in
>          variable 'KBUILD_AFLAGS' = '-D__ASSEMBLY__ -fno-PIE -m64 '
(with the trailing space),
> KBUILD_AFLAGS is "-D__ASSEMBLY__ -fno-PIE -m64"
> KBUILD_AFLAGS for syncconfig "-D__ASSEMBLY__ -fno-PIE -m64 "

Ok, so the variable is actually being updated by a common rule in
> scripts/Makefile.lib:KBUILD_AFLAGS += $(subdir-asflags-y)

Which adds a trailing space to $(KBUILD_AFLAGS) and as a result breaks
the comparison in include/config/auto.conf.cmd

So, I see 3 options for the patch in the thread:
1. Simple and ugly: add ifneq to scripts/Makefile.lib and update
KBUILD_AFLAGS only if $(subdir-asflags-y) is not empty.
2. Maybe a better one: remove trailing (and potentially leading, and
duplicate) spaces for expanded variables in
scripts/kconfig/preprocess.c
3. If the issue in the patch description is not worth it: I could just
work that around in the build system.

Please, advise what's your opinion/views.

Thanks,
             Dmitry
diff mbox series

Patch

diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include
index 3ee8ecfb8c04..d8574c1faf2d 100644
--- a/scripts/Kconfig.include
+++ b/scripts/Kconfig.include
@@ -33,7 +33,7 @@  ld-option = $(success,$(LD) -v $(1))
 
 # $(as-instr,<instr>)
 # Return y if the assembler supports <instr>, n otherwise
-as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -Wa$(comma)--fatal-warnings -c -x assembler-with-cpp -o /dev/null -)
+as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) $(KBUILD_AFLAGS) -Wa$(comma)--fatal-warnings -c -x assembler-with-cpp -o /dev/null -)
 
 # check if $(CC) and $(LD) exist
 $(error-if,$(failure,command -v $(CC)),C compiler '$(CC)' not found)