Re: [PATCH v3 3/7] perf annotate: Implement elfutils libasm disassembler backend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing build system integration leaves the libasm backend as completely dead code.
- [High] The implementation incorrectly relies on internal, unexported elfutils functions (`ebl_openbackend` and `ebl_closebackend`), causing linkage failures.
- [High] Incorrect return value check for `disasm_cb` causes immediate loop failure.
- [High] Passing a callback with an incompatible pointer type to `disasm_cb` causes a compilation error.
- [Low] `annotation_options__add_disassemblers_str` is exposed in the API but never hooked up to command line options.
--

commit 10af4235f6a3a3ed8b8874f806432a73221075b5
Author: Ian Rogers <irogers@google.com>

perf annotate: Implement elfutils libasm disassembler backend

This commit implements a new core disassembler backend utilizing elfutils'
libasm API. It hooks the backend into perf annotate routing, sets it as
the default choice when available, and exposes a function to allow custom
disassembler selection.

> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index b22cdc24082a5..f1d5ebb2edfa8 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -12,6 +12,7 @@ perf-util-y += block-range.o
>  perf-util-y += build-id.o
>  perf-util-y += cacheline.o
>  perf-util-$(CONFIG_LIBCAPSTONE) += capstone.o
> +perf-util-$(CONFIG_LIBASM) += libasm.o

[Severity: High]
Does this code require corresponding changes in the build system to actually
enable CONFIG_LIBASM and HAVE_LIBASM_SUPPORT?

Without feature detection in tools/build/feature/test-libasm.c and
Makefile.config, it seems libasm.o won't be built and the fallback inline
stub in libasm.h will be used permanently.

[ ... ]

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 02505222d8c2d..2259ac2c2cd6c 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -2254,8 +2255,8 @@ static void annotation_options__add_disassembler(struct annotation_options *opti
>  	pr_err("Failed to add disassembler %d\n", dis);
>  }
>  
> -static int annotation_options__add_disassemblers_str(struct annotation_options *options,
> -						const char *str)
> +int annotation_options__add_disassemblers_str(struct annotation_options *options,
> +					     const char *str)
>  {
>  	while (str && *str != '\0') {
>  		const char *comma = strchr(str, ',');

[Severity: Low]
Is this function ever hooked up to command line options like in cmd_annotate()?

The commit message mentions exposing it to allow custom disassembler selection,
but without binding it to arguments, it appears to remain uncalled.

[ ... ]

> diff --git a/tools/perf/util/libasm.c b/tools/perf/util/libasm.c
> new file mode 100644
> index 0000000000000..2f0a657335223
> --- /dev/null
> +++ b/tools/perf/util/libasm.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "libasm.h"
[ ... ]
> +#include "namespaces.h"
> +#include "symbol.h"
> +
> +struct ebl;
> +extern struct ebl *ebl_openbackend(Elf *elf);
> +extern void ebl_closebackend(struct ebl *ebl);

[Severity: High]
Are these internal libebl functions guaranteed to be available for linking?

They don't seem to be exposed in public elfutils headers like
<elfutils/libasm.h>. Standard Linux distributions might not export these
symbols in libdw.so or provide libebl.a, which could break compilation
and linkage.

> +
> +struct disasm_output_arg {
> +	char *buf;
> +	size_t size;
> +};
> +
> +static int disasm_output_cb(char *str, size_t len, void *arg)

[Severity: High]
Does the DisasmOutputCB_t signature expect the string argument to be
const char *?

Dropping the const qualifier here might cause a compilation failure since the
perf tool builds with -Werror.

> +{
> +	struct disasm_output_arg *oa = arg;
> +	size_t to_copy = len < oa->size - 1 ? len : oa->size - 1;
[ ... ]
> +	offset = 0;
> +	while (pc < end) {
> +		struct disasm_output_arg oa = {
> +			.buf = disasm_buf,
> +			.size = sizeof(disasm_buf),
> +		};
> +		const uint8_t *prev_pc = pc;
> +
> +		ret = disasm_cb(handle, &pc, end, addr, "%7m %.1o,%.2o,%.3o,%.4o,%.5o",
> +				    disasm_output_cb, &oa, NULL);
> +		if (ret != 1 || pc == prev_pc) {
> +			/* Disassembly failed or got stuck */
> +			break;
> +		}

[Severity: High]
Does disasm_cb() return 0 on success?

If so, evaluating ret != 1 here will evaluate to true on the very first
successful instruction parsing, instantly breaking the loop. This would
guarantee that offset != buf_len, causing the code to discard all lines
and unconditionally fall back to objdump.

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