Re: [PATCH v5 21/26] perf annotate-arm64: Support stack variable tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Invalid register comparison causes unparsed operands to be falsely treated as stack variables.
--

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

perf annotate-arm64: Support stack variable tracking

This commit extends update_insn_state_arm64 to track data types stored on the
stack. It handles stack loads and stores, introducing
propagate_store_reg_state to follow data types between registers and memory.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 9d43e2c2041cd..d520975c59363 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> @@ -542,12 +544,57 @@ static int propagate_load_reg_state(struct type_state *state,

[ ... ]

> +	if (dloc->fb_cfa) {
> +		u64 ip = dloc->ms->sym->start + dl->al.offset;
> +		u64 pc = map__rip_2objdump(dloc->ms->map, ip);
> +
> +		if (die_get_cfa(dloc->di->dbg, pc, &fbreg, &fboff) < 0)
> +			fbreg = -1;
> +	}
> +
>  retry:
>  	if (arch_get_reg_offset(dloc->arch, src, sreg, state, false, &reg_offset))
>  		return -1;
>  
>  	reg_offset += mem_offset;
>  
> +	/* Check stack variables with offset */
> +	if (sreg == fbreg || sreg == state->stack_reg) {

[Severity: Medium]
When an unparsed or unsupported base register leaves sreg as -1, and fbreg is
also -1, does this condition evaluate to true? Could this cause unparsed
source operands to be incorrectly treated as stack loads?

[ ... ]

> +static void propagate_store_reg_state(struct type_state *state,
> +				      struct data_loc_info *dloc,
> +				      struct disasm_line *dl, int sreg,
> +				      struct annotated_op_loc *dst,
> +				      int mem_offset)
> +{

[ ... ]

> +	if (dloc->fb_cfa) {
> +		u64 ip = dloc->ms->sym->start + dl->al.offset;
> +		u64 pc = map__rip_2objdump(dloc->ms->map, ip);
> +
> +		if (die_get_cfa(dloc->di->dbg, pc, &fbreg, &fboff) < 0)
> +			fbreg = -1;
> +	}
> +
> +	/* Check stack variables with offset */
> +	if (dreg == fbreg || dreg == state->stack_reg) {

[Severity: Medium]
Similarly to the load case, if the base register extraction leaves dreg as -1
and fbreg is also -1, does this check evaluate to true? Will this erroneously
treat unparsed target operands as stack stores, potentially corrupting the
stack type state tracking?

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