Re: [PATCH bpf-next 2/3] bpftool: Prevent setting duplicate _GNU_SOURCE in Makefile

[Quentin Monnet <qmo@kernel.org>]

2024-10-15 08:54 UTC+0200 ~ Viktor Malik <vmalik@redhat.com>
> When building selftests with CFLAGS set via env variable, the value of
> CFLAGS is propagated into bpftool Makefile (called from selftests
> Makefile). This makes the compilation fail as _GNU_SOURCE is defined two
> times - once from selftests Makefile (by including lib.mk) and once from
> bpftool Makefile (by calling `llvm-config --cflags`):
> 
>      $ CFLAGS="" make -C tools/testing/selftests/bpf
>      [...]
>      CC      /bpf-next/tools/testing/selftests/bpf/tools/build/bpftool/btf.o
>      <command-line>: error: "_GNU_SOURCE" redefined [-Werror]
>      <command-line>: note: this is the location of the previous definition
>      cc1: all warnings being treated as errors
>      [...]
> 
> Let bpftool Makefile check if _GNU_SOURCE is already defined and if so,
> do not let llvm-config add it again.
> 
> Signed-off-by: Viktor Malik <vmalik@redhat.com>
> ---
>   tools/bpf/bpftool/Makefile | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index ba927379eb20..2b5a713d71d8 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -147,7 +147,13 @@ ifeq ($(feature-llvm),1)
>     # If LLVM is available, use it for JIT disassembly
>     CFLAGS  += -DHAVE_LLVM_SUPPORT
>     LLVM_CONFIG_LIB_COMPONENTS := mcdisassembler all-targets
> -  CFLAGS  += $(shell $(LLVM_CONFIG) --cflags)
> +  # When bpftool build is called from another Makefile which already sets
> +  # -D_GNU_SOURCE, do not let llvm-config add it again as it will cause conflict.


Thanks! Can you please make your comment more explicit and mention the 
file tools/testing/selftests/lib.mk and/or the use case addressed 
(building bpftool from selftests), given that you match on the exact 
string "-D_GNU_SOURCE="? Your check won't skip adding the duplicate 
definition if someone passes "-D_GNU_SOURCE", without the "=", by 
calling the Makefile from another path; that's fine, but I don't want 
users to read the Makefile and expect it to remove the second definition 
in such a case.


> +  ifneq ($(filter -D_GNU_SOURCE=,$(CFLAGS)),)
> +    CFLAGS += $(filter-out -D_GNU_SOURCE,$(shell $(LLVM_CONFIG) --cflags))
> +  else
> +    CFLAGS += $(shell $(LLVM_CONFIG) --cflags)
> +  endif

Looks good otherwise:

Reviewed-by: Quentin Monnet <qmo@kernel.org>