Re: [PATCH v3 2/2] perf unwind: Don't look up symbols when the caller doesn't want them

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf unwind: dso__load_symsrc() in entry() runs after libunwind stepping
--

commit a0b5e533925de51fda6cc2ade28ec95e77a4ee79
Author: Matthew Wood <thepacketgeek@gmail.com>

perf unwind: Don't look up symbols when the caller doesn't want them

This commit passes a symbols flag down to the DWARF unwinders (libdw and
libunwind) so callers that only need maps can unwind callchains without
loading and demangling DSO symbol tables. When symbols is false, the
unwinders call dso__load_symsrc() to locate build-ids and separate
debuginfo without loading symbols.

> diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
> index 73d191ce51a55..3c5acc052715a 100644
> --- a/tools/perf/util/unwind-libunwind.c
> +++ b/tools/perf/util/unwind-libunwind.c
> @@ -589,14 +589,26 @@ void unwind__finish_access(struct maps *maps)
>  	libunwind_arch__finish_access(maps);
>  }
>  
> -static int entry(uint64_t ip, struct thread *thread, unwind_entry_cb_t cb, void *arg)
> +static int entry(uint64_t ip, struct thread *thread, bool symbols,
> +		 unwind_entry_cb_t cb, void *arg)
>  {
>  	struct unwind_entry e;
>  	struct addr_location al;
>  	int ret;
>  
>  	addr_location__init(&al);
> -	e.ms.sym = thread__find_symbol(thread, PERF_RECORD_MISC_USER, ip, &al);
> +	if (symbols) {
> +		thread__find_symbol(thread, PERF_RECORD_MISC_USER, ip, &al);
> +	} else {
> +		/*
> +		 * Still find the DSO's separate debuginfo, which loading
> +		 * the symbols would have, for read_unwind_spec_debug_frame().
> +		 */
> +		thread__find_map(thread, PERF_RECORD_MISC_USER, ip, &al);
> +		if (al.map)
> +			dso__load_symsrc(map__dso(al.map));
> +	}
> +	e.ms.sym = al.sym;
>  	e.ip     = ip;
>  	e.ms.map = al.map;
>  	e.ms.thread = thread__get(al.thread);
> @@ -614,7 +626,7 @@ static int entry(uint64_t ip, struct thread *thread, unwind_entry_cb_t cb, void
>  int libunwind__get_entries(unwind_entry_cb_t cb, void *arg,
>  			 struct thread *thread,
>  			 struct perf_sample *sample, int max_stack,
> -			 bool best_effort)
> +			 bool symbols, bool best_effort)
>  {
>  	struct unwind_info *ui;
>  	uint64_t first_ip;
> @@ -640,7 +652,7 @@ int libunwind__get_entries(unwind_entry_cb_t cb, void *arg,
>  
>  	if (max_stack == 1) {
>  		/* Special case for a single entry. */
> -		ret = entry(first_ip, thread, cb, arg);
> +		ret = entry(first_ip, thread, symbols, cb, arg);
>  		return ret ? (ret == -ENOMEM ? -ENOMEM : 0) : 1;
>  	}
>  
> @@ -662,7 +674,7 @@ int libunwind__get_entries(unwind_entry_cb_t cb, void *arg,
>  		int j = callchain_param.order == ORDER_CALLEE ? i : ui->cur_ip - i - 1;
>  
>  		if (ui->ips[j]) {
> -			ret = entry(ui->ips[j], thread, cb, arg);
> +			ret = entry(ui->ips[j], thread, symbols, cb, arg);

[Severity: Medium]
Does calling dso__load_symsrc() inside entry() happen too late for
read_unwind_spec_debug_frame() during unwinding?

In libunwind__get_entries(), all stack stepping runs to completion before
entry() is called on the collected IPs:

tools/perf/util/unwind-libunwind.c:libunwind__get_entries() {
	do {
		ret = libunwind_arch__unwind_step(ui);
		if (ret < 0)
			goto out;

	} while (ret);

	/*
	 * Display what we got based on the order setup.
	 */
	for (i = 0; i < ui->cur_ip; i++) {
		int j = callchain_param.order == ORDER_CALLEE ? i : ui->cur_ip - i - 1;

		if (ui->ips[j]) {
			ret = entry(ui->ips[j], thread, symbols, cb, arg);
			if (ret)
				break;
			entries++;
		}
	}
}

While libunwind_arch__unwind_step() is stepping the stack,
__libunwind__find_proc_info() looks up the map via find_map() (which only
calls thread__find_map()) and then invokes read_unwind_spec_debug_frame():

tools/perf/util/unwind-libunwind.c:__libunwind__find_proc_info() {
	if (ret < 0 && !read_unwind_spec_debug_frame(dso, ui->machine, &segbase)) {
		...
		symfile = dso__symsrc_filename(dso) ?: dso__name(dso);
}

Inside read_unwind_spec_debug_frame(), it attempts to open
dso__symsrc_filename(dso):

tools/perf/util/unwind-libunwind.c:read_unwind_spec_debug_frame() {
		if (ofs <= 0) {
			fd = open(dso__symsrc_filename(dso), O_RDONLY);
			if (fd >= 0) {
				ofs = elf_section_offset(fd, ".debug_frame");
				close(fd);
			}
		}
}

When callers pass symbols=false (such as build_id__mark_dso_hit(),
perf_event__inject_buildid(), get_leaf_frame_caller_aarch64(), or
test_dwarf_unwind__thread()), neither dso__load() nor dso__load_symsrc()
has been called before libunwind_arch__unwind_step() runs. As a result,
dso__symsrc_filename(dso) is still NULL when read_unwind_spec_debug_frame()
and __libunwind__find_proc_info() check it, so unwinding fails for frames
that rely on .debug_frame in separate debuginfo discovered by
dso__load_symsrc() (and if libunwind_arch__unwind_step() returns a negative
error, the goto out path skips the entry() loop entirely).

Should dso__load_symsrc(dso) be called in __libunwind__find_proc_info() or
read_unwind_spec_debug_frame() before reading dso__symsrc_filename(dso)
instead of in entry()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030193237.1440673-1-thepacketgeek@gmail.com?part=2