Re: [PATCH v7 15/15] perf annotate-arm64: Support 'mrs' instruction to track 'current' pointer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf annotate-arm64: missing lifetime cleanup in update_mrs_insn_state()
--

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

perf annotate-arm64: Support 'mrs' instruction to track 'current' pointer

This patch extends update_insn_state_arm64() to handle the mrs instruction.
It enables tracking the current task pointer in the kernel, which is
typically accessed via the sp_el0 system register.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index 5683215f07708..6da558f7e8648 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
[ ... ]
> @@ -1030,6 +1032,71 @@ static void update_adrp_insn_state(struct type_state *state,
>  		     insn_offset, tsr->imm_value, dreg);
>  }
>  
> +static void update_mrs_insn_state(struct type_state *state,
> +				  struct data_loc_info *dloc, Dwarf_Die *cu_die,
> +				  struct disasm_line *dl,
> +				  struct annotated_op_loc *dst)
> +{
> +	struct type_state_reg *tsr;
> +	Dwarf_Die type_die;
> +	u32 insn_offset = dl->al.offset;
> +	int dreg = dst->reg1;
> +	Dwarf_Die func_die;
> +	Dwarf_Attribute attr;
> +	u64 ip, pc;
> +	struct strbuf sb;
> +	char *type_name;
> +
> +	if (!has_reg_type(state, dreg))
> +		return;
> +
> +	tsr = &state->regs[dreg];
> +	tsr->copied_from = -1;
> +
> +	/* Handle case difference: LLVM (SP_EL0) vs objdump (sp_el0) */
> +	if (!dso__kernel(map__dso(dloc->ms->map)) ||
> +	    strcasecmp(dl->ops.source.raw, "sp_el0")) {
> +		invalidate_reg_state(tsr);
> +		return;
> +	}
[ ... ]
> +	if (!type_name || strcmp(type_name, "struct task_struct*")) {
> +		invalidate_reg_state(tsr);
> +		goto out_free;
> +	}
> +
> +	tsr->type = type_die;
> +	tsr->kind = TSR_KIND_TYPE;
> +	tsr->offset = 0;
> +	tsr->imm_value = 0;
> +	tsr->ok = true;

[Severity: Medium]
Are we missing a cleanup of the previous lifetime state for the destination
register here?

If the target register previously held a variable with an active Dwarf
lifetime, the lifetime_active flag remains true. Upon a subsequent function
call, the register tracking logic skips invalidating caller-saved registers
that have an active lifetime.

This could cause the target register (e.g., x0 holding current) to
incorrectly survive the function call in the analysis state, even though
the hardware register is clobbered by the callee.

Should this path clear tsr->lifetime_active and tsr->lifetime_end, or
perhaps call invalidate_reg_state() earlier before assigning the new
properties?

> +
> +	pr_debug_dtp("mrs [%x] sp_el0 -> reg%d", insn_offset, dreg);
> +	pr_debug_type_name(&type_die, tsr->kind);
> +
> +out_free:
> +	free(type_name);
> +}

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