Received-SPF: pass (google.com: domain of linux-kernel+bounces-202981-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 5 Jun 2024 10:39:41 -0700
From: Namhyung Kim <namhyung@kernel.org>
To: "Steinar H. Gunderson" <sesse@google.com>
Cc: acme@kernel.org, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, irogers@google.com,
	Arnaldo Carvalho de Melo <acme@redhat.com>
Subject: Re: [PATCH v8 1/3] perf report: Support LLVM for addr2line()
Message-ID: <ZmCi3Z940zDxF487@google.com>
References: <20240602204208.735793-1-sesse@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240602204208.735793-1-sesse@google.com>

On Sun, Jun 02, 2024 at 10:42:06PM +0200, Steinar H. Gunderson wrote:
> In addition to the existing support for libbfd and calling out to
> an external addr2line command, add support for using libllvm directly.
> This is both faster than libbfd, and can be enabled in distro builds
> (the LLVM license has an explicit provision for GPLv2 compatibility).
> Thus, it is set as the primary choice if available.
> 
> As an example, running perf report on a medium-size profile with
> DWARF-based backtraces took 58 seconds with LLVM, 78 seconds with
> libbfd, 153 seconds with external llvm-addr2line, and I got tired
> and aborted the test after waiting for 55 minutes with external
> bfd addr2line (which is the default for perf as compiled by distributions
> today). Evidently, for this case, the bfd addr2line process needs
> 18 seconds (on a 5.2 GHz Zen 3) to load the .debug ELF in question,
> hits the 1-second timeout and gets killed during initialization,
> getting restarted anew every time. Having an in-process addr2line
> makes this much more robust.
> 
> As future extensions, libllvm can be used in many other places where
> we currently use libbfd or other libraries:
> 
>  - Symbol enumeration (in particular, for PE binaries).
>  - Demangling (including non-Itanium demangling, e.g. Microsoft
>    or Rust).
>  - Disassembling (perf annotate).
> 
> However, these are much less pressing; most people don't profile
> PE binaries, and perf has non-bfd paths for ELF. The same with
> demangling; the default _cxa_demangle path works fine for most
> users, and while bfd objdump can be slow on large binaries,
> it is possible to use --objdump=llvm-objdump to get the speed benefits.
> (It appears LLVM-based demangling is very simple, should we want
> that.)
> 
> Tested with LLVM 14, 15, 16, 18 and 19. For some reason, LLVM 12 was not
> correctly detected using feature_check, and thus was not tested.
> 
> Signed-off-by: Steinar H. Gunderson <sesse@google.com>
> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
[SNIP]
> +static int addr2line(const char *dso_name, u64 addr,
> +		     char **file, unsigned int *line, struct dso *dso,
> +		     bool unwind_inlines, struct inline_node *node,
> +		     struct symbol *sym)
> +{
> +	struct llvm_a2l_frame *inline_frames = NULL;
> +	int num_frames = llvm_addr2line(dso_name, addr, file, line,
> +					node && unwind_inlines, &inline_frames);
> +
> +	if (num_frames == 0 || !inline_frames) {
> +		/* Error, or we didn't want inlines. */
> +		return num_frames;
> +	}
> +
> +	for (int i = 0; i < num_frames; ++i) {
> +		struct symbol *inline_sym =
> +			new_inline_sym(dso, sym, inline_frames[i].funcname);
> +		char *srcline = NULL;
> +
> +		if (inline_frames[i].filename)
> +			srcline = srcline_from_fileline(
> +				inline_frames[i].filename,
> +				inline_frames[i].line);

Please fix the coding style.  Also you may add a pair of brackets to
multi-line code even if it's a single statement.

Thanks,
Namhyung


> +		if (inline_list__append(inline_sym, srcline, node) != 0) {
> +			free_llvm_inline_frames(inline_frames, num_frames);
> +			return 0;
> +		}
> +	}
> +	free_llvm_inline_frames(inline_frames, num_frames);
> +
> +	return num_frames;
> +}
> +
> +void dso__free_a2l(struct dso *)
> +{
> +	/* Nothing to free. */
> +}
> +
> +#elif defined(HAVE_LIBBFD_SUPPORT)
>  
>  /*
>   * Implement addr2line using libbfd.
> -- 
> 2.45.1
>