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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect usage of the elfutils libasm API causes severely truncated disassembly output and incorrect error handling.
--

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

perf annotate: Implement elfutils libasm disassembler backend

This commit implements the core disassembler backend in libasm.c utilizing
elfutils' libasm API. It hooks the backend into perf annotate disassembler
routing and configures libasm as the first-choice disassembler backend.

> diff --git a/tools/perf/util/libasm.c b/tools/perf/util/libasm.c
> new file mode 100644
> index 0000000000000..6daac121b8f81
> --- /dev/null
> +++ b/tools/perf/util/libasm.c

[ ... ]

> +struct disasm_output_arg {
> +	char *buf;
> +	size_t size;
> +};
> +
> +static int disasm_output_cb(char *str, size_t len, void *arg)
> +{
> +	struct disasm_output_arg *oa = arg;
> +	size_t to_copy = len < oa->size - 1 ? len : oa->size - 1;
> +
> +	memcpy(oa->buf, str, to_copy);
> +	oa->buf[to_copy] = '\0';
> +	return 1;

[Severity: High]
Does returning 1 here cause the elfutils libasm API to abort parsing the
instruction? If elfutils expects 0 for success, this non-zero return
value might indicate an error and cause disassembly to stop prematurely.

Also, does this correctly append successive chunks to the buffer, or does it
overwrite the beginning each time it is called? It appears that oa->buf and
oa->size are not advanced by to_copy, which might cause successive strings
(like mnemonics and operands) to repeatedly overwrite the buffer at index 0.

> +}

[ ... ]

> +	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) {

[Severity: High]
If disasm_cb propagates the 1 returned by the callback, does checking for
ret != 1 instead of ret != 0 cause us to treat an aborted disassembly
as a success? This might mask the truncation error occurring in the
callback.

> +			/* Disassembly failed or got stuck */
> +			break;
> +		}
> +
> +		args->offset = offset;
> +		args->line = disasm_buf;

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