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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Name-based lookup precedence inversion and flawed pointer size heuristic incorrectly classify non-pointer integers and enums as kernel symbols.
- [Medium] Removal of `trace__field_is_ip()` causes oversized IP fields to be formatted as signed base-10 integers.
--

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

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

This commit enhances perf trace to automatically assign the SCA_KSYM
beautifier to tracepoint fields conveying function pointers and call sites.
It identifies these fields by their type signature and via a lookup table of
common generic field names.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
[ ... ]
> @@ -2198,38 +2210,52 @@ syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field
[ ... ]
>  		} else {
>  			const struct syscall_arg_fmt *fmt =
>  				syscall_arg_fmt__find_by_name(field->name);
>  
>  			if (fmt) {
> -				arg->scnprintf = fmt->scnprintf;
> -				arg->strtoul   = fmt->strtoul;
> +				if (fmt->scnprintf == SCA_KSYM) {
> +					int ptr_size = (field->event && field->event->tep) ?
> +						       tep_get_long_size(field->event->tep) :
> +						       (int)sizeof(void *);
> +
> +					if ((field->flags & TEP_FIELD_IS_POINTER) ||
> +					    (field->size == ptr_size && !(field->flags & TEP_FIELD_IS_ARRAY))) {

[Severity: Medium]
Will this size heuristic mistakenly match non-pointer integer and enum fields?

If a tracepoint has a generic-named field like action, location, or handler
that is actually an integer or enum (e.g. 4 bytes), and the architecture's
pointer size is also 4 bytes, this check passes and assigns SCA_KSYM.

If SCA_KSYM is assigned here, the enum BTF resolution logic located in the
subsequent fallback block is entirely skipped:

    if (arg->scnprintf == NULL) {
        ...
        } else if (strstr(field->type, "enum") && use_btf != NULL) {
            *use_btf = true;
            arg->strtoul = STUL_BTF_TYPE;
        }

Could this cause small integer or enum values to be printed as meaningless
hex kernel addresses or NULL instead of their proper symbolic enum names?

> +						arg->scnprintf = fmt->scnprintf;
> +						arg->strtoul   = fmt->strtoul;
> +					}
[ ... ]
> @@ -3404,14 +3424,11 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct perf_sample *
>  		 * Suppress it by default to avoid cluttering the output.
>  		 * If verbose mode is enabled, ensure it is formatted as a
>  		 * hexadecimal memory address rather than a signed integer.
> -		 *
> -		 * caller_ip and call_site are also expected to be instruction
> -		 * pointers and should always be represented in hexadecimal.
>  		 */
>  		is_probe_ip = evsel__is_probe(evsel) && !strcmp(field->name, "__probe_ip");
>  
> -		if (is_probe_ip || trace__field_is_ip(field->name)) {
> -			if (is_probe_ip && !verbose)
> +		if (is_probe_ip) {
> +			if (!verbose)
>  				continue;

[Severity: Medium]
Does removing trace__field_is_ip() cause oversized IP fields to be formatted as
signed base-10 integers?

By relying entirely on the SCA_KSYM assignment in syscall_arg_fmt__init_array(),
we depend on the strict size check:

    if ((field->flags & TEP_FIELD_IS_POINTER) ||
        (field->size == ptr_size && !(field->flags & TEP_FIELD_IS_ARRAY))) {

If a tracepoint defines an IP field like caller_ip or call_site as a fixed
64-bit integer, but the trace data is processed as 32-bit (where ptr_size
is 4), the size check fails. The field receives no beautifier since
arg->scnprintf will be NULL.

Without the trace__field_is_ip() fallback, won't the default formatter in
syscall_arg_fmt__scnprintf_val() print the raw kernel address as a signed
decimal integer instead of a hex pointer?

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