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
> >>
