Re: [PATCH v5 09/26] perf annotate-arm64: Implement extract_op_location() callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Shift and extension modifiers are silently ignored for single-register operands in extract_op_location_arm64().
--

commit e285d74ff460ca1f5c334760a2efdd0b26f56319
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate-arm64: Implement extract_op_location() callback

This commit implements the extract_op_location() callback for the arm64
architecture to handle its specific assembly syntax and addressing modes.
This enables perf annotate to resolve memory locations and register types
for data type profiling.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index a7595a954cfae..d8a5904359b1a 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> +static void extract_op_location_arm64(const struct arch *arch,
> +				      struct disasm_line *dl __maybe_unused,
> +				      const char *op_str, int op_idx __maybe_unused,
> +				      struct annotated_op_loc *op_loc)
> +{
> +	const char *s = op_str;
> +	char *p = NULL;
> +
> +	if (op_str == NULL)
> +		return;
> +
> +	/*
> +	 * Handle immediate operand.
> +	 * e.g., "#0xc600" -> offset = 0xc600, imm = true
> +	 *
> +	 * After parsing, check for post-modifiers like "lsl #16", which
> +	 * typically occur in instructions such as movk, movz, movn, etc.
> +	 */
> +	if (*s == arch->objdump.imm_char) {
> +		op_loc->offset = strtol(s + 1, &p, 0);
> +		if (p && p != s + 1) {
> +			op_loc->imm = true;
> +			s = p;
> +		}
> +		goto check_modifiers;
> +	}
> +

[ ... ]

> +	/* Extract the primary register */
> +	op_loc->reg1 = arch__dwarf_regnum(arch, s);
> +	if (op_loc->reg1 == -1)
> +		return;
> +
> +	/* Move to the next operand field, if present */
> +	s = strchr(s, ',');
> +	if (s == NULL)
> +		return;
> +	s = skip_spaces(s + 1);
> +
> +	/* Extract secondary register or immediate offset */
> +	if (op_loc->multi_regs)
> +		op_loc->reg2 = arch__dwarf_regnum(arch, s);
> +	else if (*s == arch->objdump.imm_char)
> +		op_loc->offset = strtol(s + 1, NULL, 0);
> +
> +check_modifiers:
> +	/*
> +	 * Look for a following shift or extension modifier:
> +	 *   ", lsl #3"   -> extend_type = PERF_EXT_NONE,
> +	 *                   shift_type = PERF_SHIFT_LSL, amount = 3
> +	 *   ", uxtw #3"  -> extend_type = PERF_EXT_UXTW,
> +	 *                   shift_type = PERF_SHIFT_LSL, amount = 3
> +	 *   ", uxtw"     -> extend_type = PERF_EXT_UXTW,
> +	 *                   shift_type = PERF_SHIFT_LSL, amount = 0
> +	 */
> +	s = strchr(s, ',');
> +	if (s == NULL)
> +		return;

[Severity: Medium]
Will this silently ignore shift and extension modifiers for single-register
operands in extract_op_location_arm64()?

When perf annotate processes an instruction like "cmp x0, x1, lsl #3", the
source operand string is parsed as "x1, lsl #3". During primary register
extraction, the pointer 's' is advanced past the first comma to point
directly at "lsl #3".

Because multi_regs is false and the operand is not an immediate, execution
falls through to the check_modifiers block. At this point, the string is
just "lsl #3". The strchr(s, ',') search will fail and the function returns
early without parsing the modifier.

Could this lead to missing annotations and incomplete statistics for these
instructions during data type profiling?

> +	s = skip_spaces(s + 1);
> +
> +	op_loc->extend_type = arm64__check_ext_type(s);
> +	op_loc->shift_type = arm64__check_shift_type(s);
> +	/* ARM64 extended operands are implicitly shifted by LSL. */
> +	if (op_loc->extend_type != PERF_EXT_NONE)
> +		op_loc->shift_type = PERF_SHIFT_LSL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788872630.git.wutengda@huaweicloud.com?part=9