Received-SPF: pass (google.com: domain of linux-kernel+bounces-188437-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 24 May 2024 10:09:39 +0200
From: "Steinar H. Gunderson" <sesse@google.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	irogers@google.com, Namhyung Kim <namhyung@kernel.org>
Subject: Re: [PATCH v5 1/3] perf report: Support LLVM for addr2line()
Message-ID: <ZlBLQ87edfIHiGRt@google.com>
References: <20240523092920.3355004-1-sesse@google.com>
 <Zk-MpqzRlAecrWA6@x1>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zk-MpqzRlAecrWA6@x1>

On Thu, May 23, 2024 at 03:36:22PM -0300, Arnaldo Carvalho de Melo wrote:
> Makefile.config:982: No libllvm found, slower source file resolution, please install llvm-devel/llvm-dev
> 
> ...                                    llvm: [ OFF ]
> 
> But maybe use "libllvm"?

I think I was trying to be consistent with the previous patch using LLVM
(something with Clang and eBPF, I think?), which used llvm and not
libllvm for the name here :-) And I don't think upstream actually uses
the libllvm name much (e.g. as you can see, it's llvm-devel, not
libllvm-devel). But I can change it if you think it's a better name;
I don't mind much either way.

> So mostly the above, and:
> 
> root@x1:~# perf probe -x /usr/bin/find -L find | grep fts_read
>      44        while ( (errno=0, ent=fts_read (p)) != NULL )
>                /* fts_read returned NULL; distinguish between "finished" and "error". */
> root@x1:~#

This part went a bit over my head, I'll just assume it's good. :-)

> So the libllvm is even producing a better result, showing those inlines
> not seen in the BFD based output.

I think maybe you need to give --inlines to BFD objdump to get it to
show inlines. (It can be useful in perf annotate sometimes.)
But annoyingly enough, LLVM objdump does not understand the flag.

> 100x speedup, looks like a win! 8-)

Great, thanks for testing. :-)

> Thanks a lot, the comments I made on the patch are mostly coding style,
> please consider them, but I wouldn't get in the way for this patch to
> get merged because of that albeit would be nice to try to fit in more
> nicely with the existing source code base.

Sure, I'll make a v6 with the requested changes. Just some
questions/comments below.

>> +		*inline_frames = (llvm_a2l_frame*)malloc(
>> +			sizeof(**inline_frames) * num_frames);
> Do we really need to cast here (my C++ is super rusty), and why not
> calloc()?

Yes, C++ does not have implicit cast-from-void*. The C++ way would be
new[] or use std::vector, but, that would make it impossible for the
caller to free(). I can use calloc, it won't change much either way.

> > +			dst.funcname = strdup(src.FunctionName.c_str());
> If strdup fails, users will cope?

Yes, same as dst.filename. new_inline_sym() (which is where we give this
data to) has an explicit check for if (!funcname).

> > +		Expected<DILineInfo> res_or_err =
> > +			symbolizer->symbolizeCode(dso_name, sectioned_addr);
> same line?

That takes it way over 80 characters, is that OK?

/* Steinar */