On 10/15/24 12:03, Quentin Monnet wrote: > 2024-10-15 08:54 UTC+0200 ~ Viktor Malik <vmalik@xxxxxxxxxx> >> 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@xxxxxxxxxx> >> --- >> 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. Right, that's a good point, I'll expand the comment. Thanks! > > >> + 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@xxxxxxxxxx> >
