Re: [PATCH bpf-next v2 6/8] bpftool: Add LLVM as default library for disassembling JIT-ed programs

[Yonghong Song <yhs@fb.com>]

On 9/16/22 2:09 PM, Daniel Borkmann wrote:
> On 9/11/22 10:14 PM, Quentin Monnet wrote:
>> To disassemble instructions for JIT-ed programs, bpftool has relied on
>> the libbfd library. This has been problematic in the past: libbfd's
>> interface is not meant to be stable and has changed several times. For
>> building bpftool, we have to detect how the libbfd version on the system
>> behaves, which is why we have to handle features disassembler-four-args
>> and disassembler-init-styled in the Makefile. When it comes to shipping
>> bpftool, this has also caused issues with several distribution
>> maintainers unwilling to support the feature (see for example Debian's
>> page for binutils-dev, which ships libbfd: "Note that building Debian
>> packages which depend on the shared libbfd is Not Allowed." [0]).
>>
>> For these reasons, we add support for LLVM as an alternative to libbfd
>> for disassembling instructions of JIT-ed programs. Thanks to the
>> preparation work in the previous commits, it's easy to add the library
>> by passing the relevant compilation options in the Makefile, and by
>> adding the functions for setting up the LLVM disassembler in file
>> jit_disasm.c.
> 
> Could you add more context around the LLVM lib? The motivation is that 
> libbfd's
> interface is not meant to be stable and has changed several times. How 
> does this
> look on the LLVM's library side? Also, for the 2nd part, what is 
> Debian's stance
> related to the LLVM lib? Would be good if both is explained in the 
> commit message.
> Right now it mainly reads 'that libbfd has all these issues, so we're 
> moving to
> something else', so would be good to provide more context to the ready 
> why the
> 'something else' is better than current one.

It will be good to mention that e.g., llvm development package
(e.g., llvm-devel for fedora) is needed for bpftool build with llvm.

> 
>> Naturally, the display of disassembled instructions comes with a few
>> minor differences. Here is a sample output with libbfd (already
>> supported before this patch):
>>
>>      # bpftool prog dump jited id 56
>>      bpf_prog_6deef7357e7b4530:
>>         0:   nopl   0x0(%rax,%rax,1)
>>         5:   xchg   %ax,%ax
>>         7:   push   %rbp
>>         8:   mov    %rsp,%rbp
>>         b:   push   %rbx
>>         c:   push   %r13
>>         e:   push   %r14
>>        10:   mov    %rdi,%rbx
>>        13:   movzwq 0xb4(%rbx),%r13
>>        1b:   xor    %r14d,%r14d
>>        1e:   or     $0x2,%r14d
>>        22:   mov    $0x1,%eax
>>        27:   cmp    $0x2,%r14
>>        2b:   jne    0x000000000000002f
>>        2d:   xor    %eax,%eax
>>        2f:   pop    %r14
>>        31:   pop    %r13
>>        33:   pop    %rbx
>>        34:   leave
>>        35:   ret
>>
>> LLVM supports several variants that we could set when initialising the
>> disassembler, for example with:
>>
>>      LLVMSetDisasmOptions(*ctx,
>>                           LLVMDisassembler_Option_AsmPrinterVariant);
>>
>> but the default printer is used for now. Here is the output with LLVM:
>>
>>      # bpftool prog dump jited id 56
>>      bpf_prog_6deef7357e7b4530:
>>         0:   nopl    (%rax,%rax)
>>         5:   nop
>>         7:   pushq   %rbp
>>         8:   movq    %rsp, %rbp
>>         b:   pushq   %rbx
>>         c:   pushq   %r13
>>         e:   pushq   %r14
>>        10:   movq    %rdi, %rbx
>>        13:   movzwq  180(%rbx), %r13
>>        1b:   xorl    %r14d, %r14d
>>        1e:   orl     $2, %r14d
>>        22:   movl    $1, %eax
>>        27:   cmpq    $2, %r14
>>        2b:   jne     0x2f
>>        2d:   xorl    %eax, %eax
>>        2f:   popq    %r14
>>        31:   popq    %r13
>>        33:   popq    %rbx
>>        34:   leave
>>        35:   retq
>>
>> The LLVM disassembler comes as the default choice, with libbfd as a
>> fall-back.
>>
>> Of course, we could replace libbfd entirely and avoid supporting two
>> different libraries. One reason for keeping libbfd is that, right now,
>> it works well, we have all we need in terms of features detection in the
>> Makefile, so it provides a fallback for disassembling JIT-ed programs if
>> libbfd is installed but LLVM is not. The other motivation is that libbfd
>> supports nfp instruction for Netronome's SmartNICs and can be used to
>> disassemble offloaded programs, something that LLVM cannot do. If
>> libbfd's interface breaks again in the future, we might reconsider
>> keeping support for it.
>>
>> [0] 
>> https://packages.debian.org/buster/binutils-dev
>> Signed-off-by: Quentin Monnet <quentin@isovalent.com>
>> Tested-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> 
> Thanks,
> Daniel