Re: [PATCH] perf srcline: Implement addr2line using libdw

[Martin Rodriguez Reboredo <yakoyoku@gmail.com>]

On 4/1/24 1:56 PM, Ian Rogers wrote:
> On Mon, Apr 1, 2024 at 9:08 AM Martin Rodriguez Reboredo
> <yakoyoku@gmail.com> wrote:
> [...]
> 
> Awesome sauce! Namhyung was just mentioning the idea to do this to me.
> I wonder when this lands we can just work to remove all of the
> BUILD_NONDISTRO options, namely libbfd, libiberty, etc. I suspect we
> have dead/broken code hiding there.

I thought about the same, though I think there's some disassembler
things to tackle, otherwise it'd be easy to do.

> [...]
> 
> What does SYMBOLIZER mean in this context? Shouldn't the code be gated
> on say a HAVE_LIBDW?

Accourding to LLVM `addr2line` is a "symbolizer" program, that
`SYMBOLIZER` means that we have a library for translating an address or
a symbol with an offset into a source file and line.

> [...]
> 
> Perhaps libdw_a2l_data to avoid confusion with data used for forked
> addr2line. Could you comment the variables? Names like "input" are
> fairly generic so you could provide an example of what their values
> are. It is also useful to comment when something like a string is
> owned by the struct, so that cleaning it up can be checked.

I've left out some unused and suboptimal fields, a mistake from my part.
Though `filename` and `funcname` come as read-only from `dwfl` so they
don't have to be copied.

>> +       const char *input;
>> +       Dwarf_Addr addr;
>> +       Dwarf_Addr bias;
>> +
>> +       bool found;
>> +       const char *filename;
>> +       const char *funcname;
>> +       int line;
> 
> Moving "found" and "line" later will avoid padding. As this data is
> attached to a DSO, does there need to be some kind of locking protocol
> for >1 symbolizing the same DSO? Perhaps these should be filled in as
> out arguments to avoid this kind of complexity.

Maybe not, I'm not sure about it.

> Also, there is some DSO clean up happening in:
> https://lore.kernel.org/lkml/CAM9d7chqnsDBCVFoK2hSs=22QrXBS=13Px5hGA4hM=ho7CZd2g@mail.gmail.com/
> where accessor functions are used for the sake of reference count checking:
> https://perf.wiki.kernel.org/index.php/Reference_Count_Checking
> which may cause some minor conflicts with this patch.

Will rebase it, then.

> [...]
> 
>> +               Dwarf_Die subroutine;
>> +               Dwarf_Off dieoff = dwarf_dieoffset(&scopes[0]);
>> +               dwarf_offdie(dwfl_module_getdwarf(a2l->mod, &bias), dieoff,
>> +                            &subroutine);
>> +               free(scopes);
>> +               scopes = NULL;
> 
> Is this dead code?

I don't think so, as the scopes could probably differ in each call, I
will have to investigate.

>> +
>> +               nscopes = dwarf_getscopes_die(&subroutine, &scopes);
>> +               if (nscopes > 1) {
> 
> Similar early return comment to above to avoid indentation.
> 
> Thanks,
> Ian
> [...]