Re: [PATCH dwarves] dwarf_loader: handle DWARF5 DW_OP_addrx properly

[Sedat Dilek <sedat.dilek@gmail.com>]

On Sun, Apr 4, 2021 at 6:45 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 4/3/21 11:41 AM, Yonghong Song wrote:
> > Currently, when DWARF5 is enabled in kernel, DEBUG_INFO_BTF
> > needs to be disabled. I hacked the kernel to enable DEBUG_INFO_BTF
> > like:
> >    --- a/lib/Kconfig.debug
> >    +++ b/lib/Kconfig.debug
> >    @@ -286,7 +286,6 @@ config DEBUG_INFO_DWARF5
> >            bool "Generate DWARF Version 5 debuginfo"
> >            depends on GCC_VERSION >= 50000 || CC_IS_CLANG
> >            depends on CC_IS_GCC || $(success,$(srctree)/scripts/test_dwarf5_support.sh $(CC) $(CLANG_FLAGS))
> >    -       depends on !DEBUG_INFO_BTF
> >            help
> > and tried DWARF5 with latest trunk clang, thin-lto and no lto.
> > In both cases, I got a few additional failures like:
> >    $ ./test_progs -n 55/2
> >    ...
> >    libbpf: extern (var ksym) 'bpf_prog_active': failed to find BTF ID in kernel BTF(s).
> >    libbpf: failed to load object 'kfunc_call_test_subprog'
> >    libbpf: failed to load BPF skeleton 'kfunc_call_test_subprog': -22
> >    test_subprog:FAIL:skel unexpected error: 0
> >    #55/2 subprog:FAIL
> >
> > Here, bpf_prog_active is a percpu global variable and pahole is supposed to
> > put into BTF, but it is not there.
> >
> > Further analysis shows this is due to encoding difference between
> > DWARF4 and DWARF5. In DWARF5, a new section .debug_addr
> > and several new ops, e.g. DW_OP_addrx, are introduced.
> > DW_OP_addrx is actually an index into .debug_addr section starting
> > from an offset encoded with DW_AT_addr_base in DW_TAG_compile_unit.
> >
> > For the above 'bpf_prog_active' example, with DWARF4, we have
> >    0x02281a96:   DW_TAG_variable
> >                    DW_AT_name      ("bpf_prog_active")
> >                    DW_AT_decl_file ("/home/yhs/work/bpf-next/include/linux/bpf.h")
> >                    DW_AT_decl_line (1170)
> >                    DW_AT_decl_column       (0x01)
> >                    DW_AT_type      (0x0226d171 "int")
> >                    DW_AT_external  (true)
> >                    DW_AT_declaration       (true)
> >
> >    0x02292f04:   DW_TAG_variable
> >                    DW_AT_specification     (0x02281a96 "bpf_prog_active")
> >                    DW_AT_decl_file ("/home/yhs/work/bpf-next/kernel/bpf/syscall.c")
> >                    DW_AT_decl_line (45)
> >                    DW_AT_location  (DW_OP_addr 0x28940)
> > For DWARF5, we have
> >    0x0138b0a1:   DW_TAG_variable
> >                    DW_AT_name      ("bpf_prog_active")
> >                    DW_AT_type      (0x013760b9 "int")
> >                    DW_AT_external  (true)
> >                    DW_AT_decl_file ("/home/yhs/work/bpf-next/kernel/bpf/syscall.c")
> >                    DW_AT_decl_line (45)
> >                    DW_AT_location  (DW_OP_addrx 0x16)
> >
> > This patch added support for DW_OP_addrx. With the patch, the above
> > failing bpf selftest and other similar failed selftests succeeded.
>
> Arnaldo, sorry, I just found that I forgot signed-off. Here it is,
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
>
> If no further change is needed for this patch, maybe you can help add
> it? Otherwise, I can add it in v2. Thanks!
>

I see a S-o-b when using b4 tool:

link="https://lore.kernel.org/bpf/20210403184158.2834387-1-yhs@fb.com"
b4 -d am $link

$ grep Signed-off-by:
20210403_yhs_dwarf_loader_handle_dwarf5_dw_op_addrx_properly.mbx
71:Signed-off-by: Yonghong Song <yhs@fb.com>

- Sedat -
> > ---
> >   dwarf_loader.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > NOTE: with this patch, at least for clang trunk, all bpf selftests
> >        are fine for DWARF5 w.r.t. compiler and pahole. Hopefully
> >        after pahole 1.21 release, we can remove DWARF5 dependence
> >        with !DEBUG_INFO_BTF.
> >
> > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > index 82d7131..49336ac 100644
> > --- a/dwarf_loader.c
> > +++ b/dwarf_loader.c
> > @@ -401,8 +401,19 @@ static int attr_location(Dwarf_Die *die, Dwarf_Op **expr, size_t *exprlen)
> >   {
> >       Dwarf_Attribute attr;
> >       if (dwarf_attr(die, DW_AT_location, &attr) != NULL) {
> > -             if (dwarf_getlocation(&attr, expr, exprlen) == 0)
> > +             if (dwarf_getlocation(&attr, expr, exprlen) == 0) {
> > +                     /* DW_OP_addrx needs additional lookup for real addr. */
> > +                     if (*exprlen != 0 && expr[0]->atom == DW_OP_addrx) {
> > +                             Dwarf_Attribute addr_attr;
> > +                             dwarf_getlocation_attr(&attr, expr[0], &addr_attr);
> > +
> > +                             Dwarf_Addr address;
> > +                             dwarf_formaddr (&addr_attr, &address);
> > +
> > +                             expr[0]->number = address;
> > +                     }
> >                       return 0;
> > +             }
> >       }
> >
> >       return 1;
> > @@ -626,6 +637,7 @@ static enum vscope dwarf__location(Dwarf_Die *die, uint64_t *addr, struct locati
> >               Dwarf_Op *expr = location->expr;
> >               switch (expr->atom) {
> >               case DW_OP_addr:
> > +             case DW_OP_addrx:
> >                       scope = VSCOPE_GLOBAL;
> >                       *addr = expr[0].number;
> >                       break;
> >