[PATCH] selftests, x86: fix how check_cc.sh is being invoked

[PATCH] selftests, x86: fix how check_cc.sh is being invoked

[Guillaume Tucker]

Add quotes around $(CC) when calling check_cc.sh from a Makefile to
pass the value as a single argument to the script even if it has
several words such as "ccache gcc".  Conversely, remove quotes in
check_cc.sh when calling $CC to make it a command with potentially
several arguments again.

Fixes: e9886ace222e ("selftests, x86: Rework x86 target architecture detection")
Tested-by: "kernelci.org bot" <bot@kernelci.org>
Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 tools/testing/selftests/vm/Makefile     | 6 +++---
 tools/testing/selftests/x86/Makefile    | 6 +++---
 tools/testing/selftests/x86/check_cc.sh | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
index 1607322a112c..d934f026ebb5 100644
--- a/tools/testing/selftests/vm/Makefile
+++ b/tools/testing/selftests/vm/Makefile
@@ -49,9 +49,9 @@ TEST_GEN_FILES += split_huge_page_test
 TEST_GEN_FILES += ksm_tests
 
 ifeq ($(MACHINE),x86_64)
-CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh $(CC) ../x86/trivial_32bit_program.c -m32)
-CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh $(CC) ../x86/trivial_64bit_program.c)
-CAN_BUILD_WITH_NOPIE := $(shell ./../x86/check_cc.sh $(CC) ../x86/trivial_program.c -no-pie)
+CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32)
+CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_64bit_program.c)
+CAN_BUILD_WITH_NOPIE := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_program.c -no-pie)
 
 TARGETS := protection_keys
 BINARIES_32 := $(TARGETS:%=%_32)
diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index 8a1f62ab3c8e..53df7d3893d3 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -6,9 +6,9 @@ include ../lib.mk
 .PHONY: all all_32 all_64 warn_32bit_failure clean
 
 UNAME_M := $(shell uname -m)
-CAN_BUILD_I386 := $(shell ./check_cc.sh $(CC) trivial_32bit_program.c -m32)
-CAN_BUILD_X86_64 := $(shell ./check_cc.sh $(CC) trivial_64bit_program.c)
-CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh $(CC) trivial_program.c -no-pie)
+CAN_BUILD_I386 := $(shell ./check_cc.sh "$(CC)" trivial_32bit_program.c -m32)
+CAN_BUILD_X86_64 := $(shell ./check_cc.sh "$(CC)" trivial_64bit_program.c)
+CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh "$(CC)" trivial_program.c -no-pie)
 
 TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
 			check_initial_reg_state sigreturn iopl ioperm \
diff --git a/tools/testing/selftests/x86/check_cc.sh b/tools/testing/selftests/x86/check_cc.sh
index 3e2089c8cf54..aff2c15018b5 100755
--- a/tools/testing/selftests/x86/check_cc.sh
+++ b/tools/testing/selftests/x86/check_cc.sh
@@ -7,7 +7,7 @@ CC="$1"
 TESTPROG="$2"
 shift 2
 
-if "$CC" -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then
+if $CC -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then
     echo 1
 else
     echo 0
-- 
2.30.2

Re: [PATCH] selftests, x86: fix how check_cc.sh is being invoked

[Guillaume Tucker]

On 25/02/2022 13:15, Guillaume Tucker wrote:
> Add quotes around $(CC) when calling check_cc.sh from a Makefile to
> pass the value as a single argument to the script even if it has
> several words such as "ccache gcc".  Conversely, remove quotes in
> check_cc.sh when calling $CC to make it a command with potentially
> several arguments again.
> 
> Fixes: e9886ace222e ("selftests, x86: Rework x86 target architecture detection")
> Tested-by: "kernelci.org bot" <bot@kernelci.org>
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  tools/testing/selftests/vm/Makefile     | 6 +++---
>  tools/testing/selftests/x86/Makefile    | 6 +++---
>  tools/testing/selftests/x86/check_cc.sh | 2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> index 1607322a112c..d934f026ebb5 100644
> --- a/tools/testing/selftests/vm/Makefile
> +++ b/tools/testing/selftests/vm/Makefile
> @@ -49,9 +49,9 @@ TEST_GEN_FILES += split_huge_page_test
>  TEST_GEN_FILES += ksm_tests
>  
>  ifeq ($(MACHINE),x86_64)
> -CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh $(CC) ../x86/trivial_32bit_program.c -m32)
> -CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh $(CC) ../x86/trivial_64bit_program.c)
> -CAN_BUILD_WITH_NOPIE := $(shell ./../x86/check_cc.sh $(CC) ../x86/trivial_program.c -no-pie)
> +CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32)
> +CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_64bit_program.c)
> +CAN_BUILD_WITH_NOPIE := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_program.c -no-pie)
>  
>  TARGETS := protection_keys
>  BINARIES_32 := $(TARGETS:%=%_32)
> diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> index 8a1f62ab3c8e..53df7d3893d3 100644
> --- a/tools/testing/selftests/x86/Makefile
> +++ b/tools/testing/selftests/x86/Makefile
> @@ -6,9 +6,9 @@ include ../lib.mk
>  .PHONY: all all_32 all_64 warn_32bit_failure clean
>  
>  UNAME_M := $(shell uname -m)
> -CAN_BUILD_I386 := $(shell ./check_cc.sh $(CC) trivial_32bit_program.c -m32)
> -CAN_BUILD_X86_64 := $(shell ./check_cc.sh $(CC) trivial_64bit_program.c)
> -CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh $(CC) trivial_program.c -no-pie)
> +CAN_BUILD_I386 := $(shell ./check_cc.sh "$(CC)" trivial_32bit_program.c -m32)
> +CAN_BUILD_X86_64 := $(shell ./check_cc.sh "$(CC)" trivial_64bit_program.c)
> +CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh "$(CC)" trivial_program.c -no-pie)
>  
>  TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
>  			check_initial_reg_state sigreturn iopl ioperm \
> diff --git a/tools/testing/selftests/x86/check_cc.sh b/tools/testing/selftests/x86/check_cc.sh
> index 3e2089c8cf54..aff2c15018b5 100755
> --- a/tools/testing/selftests/x86/check_cc.sh
> +++ b/tools/testing/selftests/x86/check_cc.sh
> @@ -7,7 +7,7 @@ CC="$1"
>  TESTPROG="$2"
>  shift 2
>  
> -if "$CC" -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then
> +if $CC -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then
>      echo 1
>  else
>      echo 0

I see the change in check_cc.sh is already covered by Usama's patch:

  selftests/x86: Add validity check and allow field splitting

-if "$CC" -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then
+if [ -n "$CC" ] && $CC -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then


However, the rest of this patch in the Makefiles still needs to
be applied.  Let me know if I should rebase it on top of Usama's.

Thanks,
Guillaume

RE: [PATCH] selftests, x86: fix how check_cc.sh is being invoked

[David Laight]

From: Guillaume Tucker
> Sent: 25 February 2022 18:50
...
> > -if "$CC" -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then
> > +if $CC -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then
> >      echo 1
> >  else
> >      echo 0
> 
> I see the change in check_cc.sh is already covered by Usama's patch:
> 
>   selftests/x86: Add validity check and allow field splitting
> 
> -if "$CC" -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then
> +if [ -n "$CC" ] && $CC -o /dev/null "$TESTPROG" -O0 "$@" 2>/dev/null; then

Or:
	if ${CC:-false} -o /dev/null ....
There's always one more way...

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] selftests, x86: fix how check_cc.sh is being invoked

[Andrew Morton]

On Fri, 25 Feb 2022 13:15:43 +0000 Guillaume Tucker <guillaume.tucker@collabora.com> wrote:

> Add quotes around $(CC) when calling check_cc.sh from a Makefile to
> pass the value as a single argument to the script even if it has
> several words such as "ccache gcc".  Conversely, remove quotes in
> check_cc.sh when calling $CC to make it a command with potentially
> several arguments again.

This changelog describes the fix, but it fails to describe the problem
which the patch is fixing!

Presumably, we're hitting some form of runtime failure under
undescribed circumstances when running selftests.  But that's just me
reverse-engineering the patch description.  And me reverse-engineering
stuff is a gloriously unreliable thing.  Please spell out the problem.

Re: [PATCH] selftests, x86: fix how check_cc.sh is being invoked

[Guillaume Tucker]

On 26/02/2022 01:03, Andrew Morton wrote:
> On Fri, 25 Feb 2022 13:15:43 +0000 Guillaume Tucker <guillaume.tucker@collabora.com> wrote:
> 
>> Add quotes around $(CC) when calling check_cc.sh from a Makefile to
>> pass the value as a single argument to the script even if it has
>> several words such as "ccache gcc".  Conversely, remove quotes in
>> check_cc.sh when calling $CC to make it a command with potentially
>> several arguments again.
> 
> This changelog describes the fix, but it fails to describe the problem
> which the patch is fixing!
> 
> Presumably, we're hitting some form of runtime failure under
> undescribed circumstances when running selftests.  But that's just me
> reverse-engineering the patch description.  And me reverse-engineering
> stuff is a gloriously unreliable thing.  Please spell out the problem.

Thanks for the review.  I've just sent a v2 which is rebased on
other changes in linux-next and with a reworked commit message
which should hopefully be clearer.  The issue was seen when
building some kselftest binaries and $CC is defined with multiple
arguments such as "ccache gcc".

Guillaume