mbox series

[v2,0/2] selftests/openat2, fchmodat2: fix clang build failures

Message ID 20240601014534.268360-1-jhubbard@nvidia.com
Headers show
Series selftests/openat2, fchmodat2: fix clang build failures | expand

Message

John Hubbard June 1, 2024, 1:45 a.m. UTC
These two subsystems require very similar fixes, so I'm sending them
out together.

Changes since the first version:

1) Rebased onto Linux 6.10-rc1.

2) Added a Reviewed-by tag from Ryan Roberts. See [1] for that.

Related work: I've sent a separate fix that allows "make CC=clang" to
work in addition to "make LLVM=1" [2].

[1] https://lore.kernel.org/518dd1e3-e31a-41c3-b488-9b75a64b6c8a@arm.com
[2] https://lore.kernel.org/20240531183751.100541-2-jhubbard@nvidia.com

John Hubbard (2):
  selftests/openat2: fix clang build failures: -static-libasan,
    LOCAL_HDRS
  selftests/fchmodat2: fix clang build failure due to -static-libasan

 tools/testing/selftests/fchmodat2/Makefile | 11 ++++++++++-
 tools/testing/selftests/openat2/Makefile   | 14 ++++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)


base-commit: cc8ed4d0a8486c7472cd72ec3c19957e509dc68c

Comments

Nathan Chancellor June 3, 2024, 10:38 p.m. UTC | #1
On Fri, May 31, 2024 at 06:45:33PM -0700, John Hubbard wrote:
> When building with clang via:
> 
>     make LLVM=1 -C tools/testing/selftests
> 
> two distinct failures occur:
> 
> 1) gcc requires -static-libasan in order to ensure that Address
> Sanitizer's library is the first one loaded. However, this leads to
> build failures on clang, when building via:
> 
>        make LLVM=1 -C tools/testing/selftests
> 
> However, clang already does the right thing by default: it statically
> links the Address Sanitizer if -fsanitize is specified. Therefore, fix
> this by simply omitting -static-libasan for clang builds. And leave
> behind a comment, because the whole reason for static linking might not
> be obvious.
> 
> 2) clang won't accept invocations of this form, but gcc will:
> 
>     $(CC) file1.c header2.h
> 
> Fix this by using selftests/lib.mk facilities for tracking local header
> file dependencies: add them to LOCAL_HDRS, leaving only the .c files to
> be passed to the compiler.
> 
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>

Assuming that $(LLVM) is the correct conditional to test for:

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  tools/testing/selftests/openat2/Makefile | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
> index 254d676a2689..185dc76ebb5f 100644
> --- a/tools/testing/selftests/openat2/Makefile
> +++ b/tools/testing/selftests/openat2/Makefile
> @@ -1,8 +1,18 @@
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  
> -CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined -static-libasan
> +CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
>  TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>  
> +# gcc requires -static-libasan in order to ensure that Address Sanitizer's
> +# library is the first one loaded. However, clang already statically links the
> +# Address Sanitizer if -fsanitize is specified. Therefore, simply omit
> +# -static-libasan for clang builds.
> +ifeq ($(LLVM),)
> +    CFLAGS += -static-libasan
> +endif
> +
> +LOCAL_HDRS += helpers.h
> +
>  include ../lib.mk
>  
> -$(TEST_GEN_PROGS): helpers.c helpers.h
> +$(TEST_GEN_PROGS): helpers.c
> -- 
> 2.45.1
> 
>
Nathan Chancellor June 3, 2024, 10:39 p.m. UTC | #2
On Fri, May 31, 2024 at 06:45:33PM -0700, John Hubbard wrote:
> When building with clang via:
> 
>     make LLVM=1 -C tools/testing/selftests
> 
> two distinct failures occur:
> 
> 1) gcc requires -static-libasan in order to ensure that Address
> Sanitizer's library is the first one loaded. However, this leads to
> build failures on clang, when building via:
> 
>        make LLVM=1 -C tools/testing/selftests
> 
> However, clang already does the right thing by default: it statically
> links the Address Sanitizer if -fsanitize is specified. Therefore, fix
> this by simply omitting -static-libasan for clang builds. And leave
> behind a comment, because the whole reason for static linking might not
> be obvious.
> 
> 2) clang won't accept invocations of this form, but gcc will:
> 
>     $(CC) file1.c header2.h
> 
> Fix this by using selftests/lib.mk facilities for tracking local header
> file dependencies: add them to LOCAL_HDRS, leaving only the .c files to
> be passed to the compiler.
> 
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>

Assuming that $(LLVM) is the correct conditional to test for:

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  tools/testing/selftests/openat2/Makefile | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
> index 254d676a2689..185dc76ebb5f 100644
> --- a/tools/testing/selftests/openat2/Makefile
> +++ b/tools/testing/selftests/openat2/Makefile
> @@ -1,8 +1,18 @@
>  # SPDX-License-Identifier: GPL-2.0-or-later
>  
> -CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined -static-libasan
> +CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined
>  TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>  
> +# gcc requires -static-libasan in order to ensure that Address Sanitizer's
> +# library is the first one loaded. However, clang already statically links the
> +# Address Sanitizer if -fsanitize is specified. Therefore, simply omit
> +# -static-libasan for clang builds.
> +ifeq ($(LLVM),)
> +    CFLAGS += -static-libasan
> +endif
> +
> +LOCAL_HDRS += helpers.h
> +
>  include ../lib.mk
>  
> -$(TEST_GEN_PROGS): helpers.c helpers.h
> +$(TEST_GEN_PROGS): helpers.c
> -- 
> 2.45.1
> 
>
Shuah Khan June 11, 2024, 9:01 p.m. UTC | #3
On 5/31/24 19:45, John Hubbard wrote:
> These two subsystems require very similar fixes, so I'm sending them
> out together.
> 
> Changes since the first version:
> 
> 1) Rebased onto Linux 6.10-rc1.
> 
> 2) Added a Reviewed-by tag from Ryan Roberts. See [1] for that.
> 
> Related work: I've sent a separate fix that allows "make CC=clang" to
> work in addition to "make LLVM=1" [2].
> 
> [1] https://lore.kernel.org/518dd1e3-e31a-41c3-b488-9b75a64b6c8a@arm.com
> [2] https://lore.kernel.org/20240531183751.100541-2-jhubbard@nvidia.com
> 
> John Hubbard (2):
>    selftests/openat2: fix clang build failures: -static-libasan,
>      LOCAL_HDRS
>    selftests/fchmodat2: fix clang build failure due to -static-libasan
> 
>   tools/testing/selftests/fchmodat2/Makefile | 11 ++++++++++-
>   tools/testing/selftests/openat2/Makefile   | 14 ++++++++++++--
>   2 files changed, 22 insertions(+), 3 deletions(-)
> 
> 
> base-commit: cc8ed4d0a8486c7472cd72ec3c19957e509dc68c

Applied to linux-kselftest fixes for Linux 6.10 next rc

thanks,
-- Shuah