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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Apr 3, 2021 at 1:20 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 4/3/21 11:52 AM, David Blaikie wrote:
> > On Sat, Apr 3, 2021 at 11:42 AM Yonghong Song <yhs@xxxxxx> 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.
> >> ---
> >>   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) {
> >
> >
> > ^ this change is probably overly narrow. DW_OP_addrx could appear at
> > other locations in a DWARF expression. So whatever code is responsible
> > for doing general evaluation of DWARF expressions should probably be
> > the place to handle this so it can deal with the full generality of
> > expressions, not only this specific case of a DW_OP_addrx as being the
> > first operation in the expression?
>
> We really have a narrow usage here. This code path (DW_OP_addr and
> DW_OP_addrx) is triggered and later on only used when converting
> dwarf to BTF for certain category of global variables and that is
> why address is needed. The above code is only called for
> DW_TAG_variable.
>
> The previous code to handle DW_OP_addr also only took the first expression.
>
>          if (attr_location(die, &location->expr, &location->exprlen) != 0)
>                  scope = VSCOPE_OPTIMIZED;
>          else if (location->exprlen != 0) {
>                  Dwarf_Op *expr = location->expr;
>                  switch (expr->atom) {
>                  case DW_OP_addr:
>                          scope = VSCOPE_GLOBAL;
>                          *addr = expr[0].number;
>                          break;
>         ......

Fair enough - if it's that limited, as you say - this is nothing new then.

> Do you think we could have multiple OPs for an *external* variable location?

With LTO I believe Clang can merge globals, but looks like only
internal linkage variables & I can't quite tickle the codepath at a
quick glance right now.




>
> >
> >>
> >> +                               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;
> >> --
> >> 2.30.2
> >>



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux