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

[Viktor Malik <vmalik@redhat.com>]

On 10/16/24 22:34, Andrii Nakryiko wrote:
> On Mon, Oct 14, 2024 at 11:55 PM Viktor Malik <vmalik@redhat.com> wrote:
>>
>> 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.
>> +  ifneq ($(filter -D_GNU_SOURCE=,$(CFLAGS)),)
>> +    CFLAGS += $(filter-out -D_GNU_SOURCE,$(shell $(LLVM_CONFIG) --cflags))
> 
> why not always do filter-out and avoid this ugly ifneq?

Because in that case, _GNU_SOURCE would not be defined for some builds
(e.g. plain bpftool build). I'm not entirely sure what the implications
are so I wanted to stay on the safe side. Anyways, I gave it a try and
bpftool builds without _GNU_SOURCE just fine so I think that we can drop
the ifneq.

> 
>> +  else
>> +    CFLAGS += $(shell $(LLVM_CONFIG) --cflags)
>> +  endif
>>    LIBS    += $(shell $(LLVM_CONFIG) --libs $(LLVM_CONFIG_LIB_COMPONENTS))
>>    ifeq ($(shell $(LLVM_CONFIG) --shared-mode),static)
>>      LIBS += $(shell $(LLVM_CONFIG) --system-libs $(LLVM_CONFIG_LIB_COMPONENTS))
>> --
>> 2.47.0
>>
>