Re: [PATCH v1 2/2] perf libdw: Optimize fallback logic for missing DWARF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The modified return logic for `-EINVAL` fatally aborts all unwinder fallbacks on initialization errors and truncates partial callchains.
- [High] Unsupported fallback styles incorrectly abort the entire address resolution loop.
- [Medium] The patch modifies error handling for `dwfl_module_getdwarf()` instead of `dwfl_module_getsrc()`, failing to bypass the fallback loop as intended.

Pre-existing issues:
- [High] Passing potentially `NULL` pointer to `%s` format specifier causes a segmentation fault under musl libc.
--

--- Patch [2]: [PATCH v1 2/2] perf libdw: Optimize fallback logic for missing DWARF ---
commit 5f18acea82554e76916d774c6228aad56add9f6a
Author: Ian Rogers <irogers@google.com>

perf libdw: Optimize fallback logic for missing DWARF

This commit attempts to optimize fallback logic for missing DWARF information
in perf. It modifies libdw__addr2line() to return -1 on "no DWARF" error to
bypass the subprocess fallback in addr2line(). It also updates
libdw__get_entries() to return -EINVAL if unwinding fails due to missing DWARF
information.

> diff --git a/tools/perf/util/libdw.c b/tools/perf/util/libdw.c
> index f53caee980b0b..7c681b03c6543 100644
> --- a/tools/perf/util/libdw.c
> +++ b/tools/perf/util/libdw.c
> @@ -186,8 +186,15 @@ int libdw__addr2line(const char *dso_name, u64 addr, char **file, unsigned int *
>  	 * between the regular ELF addr2line addresses and those to use with
>  	 * libdw.
>  	 */
> -	if (!dwfl_module_getdwarf(mod, &bias))
> -		return 0;
> +	if (!dwfl_module_getdwarf(mod, &bias)) {
> +		const char *err = dwfl_errmsg(-1);
> +
> +		/*
> +		 * Abort fallbacks specifically when DWARF is completely missing,
> +		 * but allow alternative backends to try if parsing failed.
> +		 */
> +		return (err && strstr(err, "no DWARF")) ? -1 : 0;
> +	}

[Severity: Medium]
The commit message mentions that when dwfl_module_getsrc() encounters an issue,
it returns 0 and causes redundant addr2line forks. However, this patch applies
the new error checking and -1 return to dwfl_module_getdwarf() instead of
dwfl_module_getsrc() on line 196:

	dwline = dwfl_module_getsrc(mod, addr + bias);
	if (!dwline)
		return 0;

Because the unmodified check for dwfl_module_getsrc() still returns 0, does this
miss the intended optimization described in the commit message and continue to
trigger subprocess fallbacks when line maps are missing?

[ ... ]

> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index 8e954f0c860a0..8283a741c0186 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -176,6 +176,8 @@ static int addr2line(const char *dso_name, u64 addr, char **file, unsigned int *
>  		}
>  		if (ret > 0)
>  			return ret;
> +		if (ret < 0)
> +			return 0;
>  	}
>  
>  	return 0;

[Severity: High]
Unsupported fallback styles, like llvm__addr2line() or libbfd__addr2line() when
not compiled in, return -1 as stubs.

If a user has an uncompiled backend configured early in their
symbol_conf.addr2line_style list, won't this new (ret < 0) condition
incorrectly match the -1 from the stub and abort the entire resolution loop,
bypassing any subsequent valid fallbacks like A2L_STYLE_CMD?

[ ... ]

> diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
> index 63a5c2253174f..69d4585c870c8 100644
> --- a/tools/perf/util/unwind-libdw.c
> +++ b/tools/perf/util/unwind-libdw.c
> @@ -441,8 +441,17 @@ int libdw__get_entries(unwind_entry_cb_t cb, void *arg,
>  	}
>  
>   out:
> -	if (err)
> -		pr_debug("unwind: failed with '%s'\n", dwfl_errmsg(-1));
> +	if (err) {
> +		const char *msg = dwfl_errmsg(-1);
> +
> +		pr_debug("unwind: failed with '%s'\n", msg);

[Severity: High]
This is a pre-existing issue, but if perf_reg_value() fails earlier in this
function, err is set to -EINVAL and it jumps to out. 

Since no libdw function failed, dwfl_errmsg(-1) can return NULL. The msg pointer
is then immediately passed to pr_debug with a '%s' format specifier before the
subsequent msg NULL check. Under musl libc, won't passing a NULL pointer to %s
cause a segmentation fault?

> +		/*
> +		 * Abort fallbacks specifically when unwinding information is completely
> +		 * missing, but allow fallback unwinders to try if parsing simply failed.
> +		 */
> +		if (msg && strstr(msg, "no DWARF"))
> +			err = -EINVAL;
> +	}
>  
>  	for (i = 0; i < ui->idx; i++)
>  		map_symbol__exit(&ui->entries[i].ms);
> @@ -460,6 +469,6 @@ int libdw__get_entries(unwind_entry_cb_t cb, void *arg,
>  	 *  < 0 : fatal error (e.g. -ENOMEM). Aborts unwinding entirely.
>  	 */
>  	if (err)
> -		return (err == -ENOMEM) ? -ENOMEM : (entries > 0 ? 1 : 0);
> +		return (err == -ENOMEM || err == -EINVAL) ? err : (entries > 0 ? 1 : 0);
>  	return entries;
>  }

[Severity: High]
If perf_reg_value() or dwfl_begin() fail earlier during initialization, err is
set to -EINVAL.

With this new return logic, returning -EINVAL (< 0) signals a fatal error to the
unwinder caller. Does this completely abort unwinding fallbacks and truncate
partial callchains instead of allowing subsequent fallbacks (like kernel
callchains) to proceed, which returning 0 would previously allow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824062841.1529489-1-irogers@google.com?part=2