Re: [PATCH v7 06/15] perf annotate-arm64: Support store instruction tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf annotate-arm64: Stale type tracking on status register in exclusive stores
--

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

perf annotate-arm64: Support store instruction tracking

Extend update_insn_state_arm64() to handle standard store instructions
(str, stur, stlr, stp). Exclusive stores (e.g., stxr, stxp) are omitted
as they exhibit different operand semantics. The patch also updates
pre-index and post-index addressing modes for the destination register.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index fb48bf95b32ff..54d9928b536fd 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> @@ -661,6 +661,20 @@ static void update_load_insn_state(struct type_state *state,
>  	adjust_reg_index_state(state, dloc, dl, src);
>  }
>  
> +/*
> + * Match standard store variants (str, stur, stlr, stp) that follow
> + * straightforward store semantics: source registers on the left (read-only)
> + * and memory target on the right. Excludes exclusive stores (stxr, stxp, etc.)
> + * which have status/result register side effects.
> + */
> +static bool is_standard_store_insn(const char *name)
> +{
> +	return !strncmp(name, "str", 3)  ||  /* str, strb, strh */
> +	       !strncmp(name, "stur", 4) ||  /* stur, sturb, sturh */
> +	       !strncmp(name, "stlr", 4) ||  /* stlr, stlrb, stlrh */
> +	       !strncmp(name, "stp", 3);     /* stp */
> +}

[Severity: Medium]
Will this result in stale type tracking for the status registers modified
by exclusive store instructions?

Since exclusive stores like stxr are excluded here, they fall through and
are ignored.  However, because arm64__insn_is_target_on_right("stxr")
places the memory operand in dst (so dst->mem_ref is true), the generic
invalidation block in update_insn_state_arm64() is also completely bypassed:

	if (has_reg_type(state, dst->reg1) && !dst->mem_ref &&
	    !is_standard_load_insn(dl->ins.name)) {
		...

For an instruction like "stxr w0, x1, [x2]", the CPU modifies the status
register (w0, parsed into src->reg1). Since it skips both standard store
processing and the generic invalidation block, does the tracker leave the
status register's type state unaltered and allow stale types to propagate?

[ ... ]

> @@ -742,6 +756,17 @@ static void update_insn_state_arm64(struct type_state *state,
>  	/* Memory to register transfers */
>  	if (is_standard_load_insn(dl->ins.name))
>  		update_load_insn_state(state, dloc, dl, src, dst);
> +	/* Register to memory transfers */
> +	else if (is_standard_store_insn(dl->ins.name)) {
> +		/*
> +		 * Ignore transfers since it'd set a value in a struct
> +		 * and won't change the type.
> +		 *
> +		 * Needs to update the pre-index and post-index addressing
> +		 * modes for the destination register.
> +		 */
> +		adjust_reg_index_state(state, dloc, dl, dst);
> +	}
>  }
>  #endif
>

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