Re: [RFC PATCH v3 3/3] perf tools: Support sparc user register samples and dwarf unwinding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf/unwind: missing endian swap for sparc window registers
- [Low] perf/sparc: missing explicit stddef.h inclusion for NULL
--

commit 58ddb5306f26894ea36bdfc46ec5335a5abbcee6
Author: Stian Halseth <stian@itx.no>

perf tools: Support sparc user register samples and dwarf unwinding

Add the sparc perf_regs tables and the EM_SPARC/EM_SPARCV9 dispatch for
register names, masks, ip/sp and the perf-to-DWARF register mapping,
plus the uapi header copy.

> diff --git a/tools/perf/util/perf-regs-arch/perf_regs_sparc.c b/tools/perf/util/perf-regs-arch/perf_regs_sparc.c
> new file mode 100644
> index 0000000000000..9ee8e496ab8cc
> --- /dev/null
> +++ b/tools/perf/util/perf-regs-arch/perf_regs_sparc.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "../perf_regs.h"
> +#include "../../arch/sparc/include/perf_regs.h"
> +
> +uint64_t __perf_reg_mask_sparc(bool intr __maybe_unused)
> +{
> +	return PERF_REGS_MASK;
> +}
> +
> +const char *__perf_reg_name_sparc(int id)
> +{
> +	switch (id) {

[ ... ]

> +	case PERF_REG_SPARC_Y:
> +		return "y";
> +	default:
> +		return NULL;
> +	}
> +
> +	return NULL;

[Severity: Low]
Does this rely on implicit header inclusion for NULL?

The NULL macro is used here, but neither <stddef.h> nor <stdlib.h> is
explicitly included in this file. It appears to rely on the implicit inclusion
of <stdlib.h> via the ../../arch/sparc/include/perf_regs.h header.

Should this explicitly include <stddef.h> or <stdlib.h>?

> +}

[ ... ]

> diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
> index 63a5c2253174f..d2adf7f2d8bb3 100644
> --- a/tools/perf/util/unwind-libdw.c
> +++ b/tools/perf/util/unwind-libdw.c

[ ... ]

> +/*
> + * The sampled window's %l and %i registers (DWARF 16-31) are not in the
> + * sample; the kernel flushed them to the register save area at the start
> + * of the dumped stack.
> + */
> +static void libdw_set_sparc_window_registers(struct unwind_info *ui,
> +					     struct regs_dump *user_regs,
> +					     Dwarf_Word *dwarf_regs)
> +{
> +	struct stack_dump *stack = &ui->sample->user_stack;
> +	u64 sp;
> +
> +	if (perf_reg_value(&sp, user_regs, perf_arch_reg_sp(ui->e_machine)))
> +		return;
> +	if (!(sp & 1) || 16 * sizeof(u64) > stack->size)
> +		return;
> +
> +	memcpy(&dwarf_regs[16], stack->data, 16 * sizeof(u64));

[Severity: Medium]
When analyzing a SPARC perf.data file on a little-endian host, will this
memcpy() write raw big-endian bytes into the little-endian host variables?

The dwarf_regs array is composed of Dwarf_Word (uint64_t) elements and expects
values in host endianness, while the stack dump (stack->data) retains the
target's original memory layout and endianness (SPARC is strictly big-endian).

This can cause the 64-bit integer values read by libdw to be byte-swapped,
breaking cross-architecture DWARF unwinding for SPARC as the stack unwinder
relies on %i6 and %i7 (DWARF registers 30 and 31) to find the caller's frame.

Should this code perform conditional byte-swapping when copying from the stack
dump?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923090429.2026529-1-stian@itx.no?part=3