Re: [PATCH perf-tools-next v2 2/4] perf trace: Auto-assign kernel symbol beautifier to function pointer fields

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The patch's commit message explicitly claims it will "register common function pointer field names within the sorted syscall_arg_fmts__by_name lookup table." However, the code completely omits any modifications to this table and instead hardcodes the string comparisons inline into `syscall_arg_fmt__init_array()`.
- [Medium] The hardcoded `field->size == sizeof(u64)` check breaks 32-bit compatibility and cross-platform analysis for function pointers.
- [Medium] The hardcoded list of function pointer field names misses standard tracepoint fields like 'func' and 'callback'.
--

--- Patch [2]: [PATCH perf-tools-next v2 2/4] perf trace: Auto-assign kernel symbol beautifier to function pointer fields ---
commit b4ca96eef1273e64d4ffedaf8b32cf198586f462
Author: Aaron Tomlin <[email protected]>

perf trace: Auto-assign kernel symbol beautifier to function pointer fields

This commit automatically detects function pointer fields in tracepoints by
name and type. It assigns the SCA_KSYM beautifier to them so they are
symbolised automatically instead of printing as generic hex values.

> Additionally, register common function pointer field names within the
> sorted syscall_arg_fmts__by_name lookup table. This ensures tracepoint
> arguments such as workqueue:workqueue_execute_start.function are
> symbolised automatically without requiring explicit per-event
> configuration.

[Severity: Medium]
Does the patch implement this update to syscall_arg_fmts__by_name?

The commit message explicitly claims to register names within this lookup
table, but the patch omits any changes to it and instead hardcodes the
string comparisons inline in syscall_arg_fmt__init_array().

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 0030489465032..85abae09d328b 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -2198,6 +2198,15 @@ syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field
>  		    ((len >= 4 && strcmp(field->name + len - 4, "name") == 0) ||
>  		     strstr(field->name, "path") != NULL)) {
>  			arg->scnprintf = SCA_FILENAME;
> +		} else if ((field->type && (strstr(field->type, "(*)") != NULL ||
> +					    strstr(field->type, "_func_t") != NULL ||
> +					    strstr(field->type, "_fn") != NULL)) ||
> +			   (((field->flags & TEP_FIELD_IS_POINTER) || field->size == sizeof(u64)) &&

[Severity: Medium]
Does this field->size check cause a regression in 32-bit compatibility and
cross-platform analysis for function pointers?

On 32-bit systems, function pointers are 4 bytes, so field->size is 4. Since
sizeof(u64) is always 8 on the host compiling perf, this check fails on
native 32-bit systems or when a 64-bit host parses a 32-bit perf.data file.

> +			    (strcmp(field->name, "fn") == 0 ||
> +			     strcmp(field->name, "function") == 0 ||
> +			     strcmp(field->name, "callsite") == 0 ||
> +			     strcmp(field->name, "call_site") == 0))) {

[Severity: Medium]
Does this hardcoded list miss standard tracepoint fields like 'func' and
'callback'?

Tracepoints using common standard names for function pointers, such as 'func'
(e.g., in csd:csd_function, rcu, and wireless subsystems) and 'callback'
(e.g., in ipi:ipi_raise), will fail this name check and will not be
automatically symbolized by the beautifier.

> +			arg->scnprintf = SCA_KSYM;
>  		} else if ((field->flags & TEP_FIELD_IS_POINTER) || strstr(field->name, "addr") ||
>  			   field_has_hex_fmt(field, len))
>  			arg->scnprintf = SCA_PTR;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2