Received-SPF: pass (google.com: domain of linux-kernel+bounces-132273-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <6d2666bd-1c86-484d-8b4f-f195f1bf1f7e@gmail.com>
Date: Thu, 4 Apr 2024 20:53:35 -0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] perf srcline: Implement addr2line using libdw
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
 Arnaldo Carvalho de Melo <acme@kernel.org>,
 Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, Adrian Hunter <adrian.hunter@intel.com>,
 linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240401160805.51277-1-yakoyoku@gmail.com>
 <CAP-5=fXu4uM=cU8Q=1JP19sELfarNE9BtBmbFW0Uyq2e_HJ6QA@mail.gmail.com>
Content-Language: en-US
From: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
In-Reply-To: <CAP-5=fXu4uM=cU8Q=1JP19sELfarNE9BtBmbFW0Uyq2e_HJ6QA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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
> [...]