| Message ID | 9320d88a3a65350d9bfdc5e258742cd0b162f017.1645794882.git.guillaume.tucker@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests, x86: fix how check_cc.sh is being invoked | expand |
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
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)
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.
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
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
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(-)