Re: [PATCH bpf-next 4/7] bpftool: Group libbfd defs in Makefile, only pass them if we use libbfd

[Quentin Monnet <quentin@isovalent.com>]

On 07/09/2022 00:31, Song Liu wrote:
> On Tue, Sep 6, 2022 at 6:44 AM Quentin Monnet <quentin@isovalent.com> wrote:
>>
> [...]
> 
>>
>> +# If one of the above feature combinations is set, we support libbfd
>>  ifneq ($(filter -lbfd,$(LIBS)),)
>> -CFLAGS += -DHAVE_LIBBFD_SUPPORT
>> -SRCS += $(BFD_SRCS)
>> +  CFLAGS += -DHAVE_LIBBFD_SUPPORT
>> +
>> +  # Libbfd interface changed over time, figure out what we need
>> +  ifeq ($(feature-disassembler-four-args), 1)
>> +    CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
>> +  endif
>> +  ifeq ($(feature-disassembler-init-styled), 1)
>> +    CFLAGS += -DDISASM_INIT_STYLED
>> +  endif
>> +endif
> 
> 
>> +ifeq ($(filter -DHAVE_LIBBFD_SUPPORT,$(CFLAGS)),)
>> +  # No support for JIT disassembly
>> +  SRCS := $(filter-out jit_disasm.c,$(SRCS))
>>  endif
> 
> This part could just be an else clause for the ifneq above.
> Well, I guess the difference is minimal.

True for this patch, but please see patch 6 with the LLVM support: the
ifneq above gets embedded in an outer if/else block (we only run it if
LLVM is not found), whereas removing jit_disasm.c from the sources
occurs when none of the two libs is available.

Ideally we'd have "if LLVM ... else if libbfd ... else remove
jit_disasm.c", but the check on libbfd involved checking multiple
features so I didn't find a simple way to write that in Makefile syntax
and thought it more readable to have a separate block for jit_disasm.c.

> 
> Acked-by: Song Liu <song@kernel.org>

Thanks for the review!