Re: [PATCH v3 08/21] perf annotate: Introduce extract_op_location callback for arch-specific parsing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Refactoring changes the error propagation behavior for memory operands with unrecognized registers, causing valid instructions to be incorrectly marked as bad.
- [Low] Missing explicit inclusion of `<stdlib.h>` for the POSIX libc function `strtol()`, breaking musl compatibility.
--

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

perf annotate: Introduce extract_op_location callback for arch-specific parsing

This patch refactors operand parsing by moving it from the central
annotate_get_insn_location() into architecture-specific callbacks. By
introducing extract_op_location callbacks for PowerPC and x86, it aims to
simplify the core logic and improve maintainability without making functional
changes.

> diff --git a/tools/perf/util/annotate-arch/annotate-x86.c b/tools/perf/util/annotate-arch/annotate-x86.c
> index 7e61365363938..6a4b4bdcd7a7c 100644
> --- a/tools/perf/util/annotate-arch/annotate-x86.c
> +++ b/tools/perf/util/annotate-arch/annotate-x86.c
> @@ -2,6 +2,7 @@
>  #include <string.h>
>  #include <linux/compiler.h>
>  #include <linux/zalloc.h>
> +#include <linux/ctype.h>
>  #include <assert.h>
>  #include <inttypes.h>
>  #include "../annotate-data.h"

[Severity: Low]
Does this file need an explicit include of <stdlib.h> for musl libc
compatibility?

It appears strtol() is used later in extract_op_location_x86() without
the required <stdlib.h> header being directly included here, which might
lead to implicit function declaration errors on musl-based systems.

[ ... ]

> @@ -809,6 +810,86 @@ static void update_insn_state_x86(struct type_state *state,
>  }
>  #endif
>  
> +/*
> + * Get register number and access offset from the given instruction.
> + * It assumes AT&T x86 asm format like OFFSET(REG).  Maybe it needs
> + * to revisit the format when it handles different architecture.
> + * Fills @reg and @offset when return 0.
> + */
> +static int extract_reg_offset(const struct arch *arch, const char *str,
> +			      struct annotated_op_loc *op_loc)
> +{

[ ... ]

> +static int extract_op_location_x86(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 0;
> +
> +	if (strchr(op_str, arch->objdump.memory_ref_char)) {
> +		op_loc->mem_ref = true;
> +		return extract_reg_offset(arch, op_str, op_loc);

[Severity: Medium]
Does returning the result of extract_reg_offset() here alter the error
propagation behavior?

In the previous annotate_get_insn_location() logic, the return value of
extract_reg_offset() was safely ignored:

        } else if (strchr(insn_str, arch->objdump.memory_ref_char)) {
                op_loc->mem_ref = true;
                op_loc->multi_regs = multi_regs;
                extract_reg_offset(arch, insn_str, op_loc);
        } else {

By returning it here, extract_op_location_x86() might return -1 if the
register is unrecognized. This regression is now propagated back to the
caller in annotate_get_insn_location():

        if (arch->extract_op_location) {
                ret = arch->extract_op_location(arch, dl, insn_str, i, op_loc);
                if (ret)
                        return ret;
        }

Could this cause valid instructions to be completely skipped and
incorrectly marked as bad?

> +	}
> +
> +	/* FIXME: Handle other segment registers */
> +	if (!strncmp(op_str, "%gs:", 4)) {
> +		op_loc->segment = INSN_SEG_X86_GS;
> +		op_loc->offset = strtol(op_str + 4,
> +					&p, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701035355.752944-1-wutengda@huaweicloud.com?part=8