Re: [PATCH] perf trace: Support --bitmask-list with --libtraceevent

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] perf trace: incomplete AST traversal for TEP_PRINT_HEX_STR nodes
- [Medium] perf trace: --bitmask-list broken for pipe mode streams
- [Low] perf trace: build failure on older libtraceevent due to missing TEP_PRINT_CPUMASK
- [Low] perf trace: future build failures due to -Wswitch-enum on external libtraceevent enum
--

commit f1dd0cf882822a7f067e6b6c6825d55f9ca3811c
Author: Aaron Tomlin <atomlin@atomlin.com>

    perf trace: Support --bitmask-list with --libtraceevent
    
    Commit ab74d1fbe061 ("perf trace: Add --bitmask-list command-line option")
    introduced the --bitmask-list option to display cpumasks and bitmasks as
    condensed, human-readable range lists (e.g. "0,2-5,7") instead of the
    default hexadecimal format.
    
    However, that implementation only augmented trace__fprintf_tp_fields().
    When --libtraceevent is specified, event formatting is delegated to
    event_format__fprintf(), which bypasses trace__fprintf_tp_fields() and
    relies directly on libtraceevent.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index c3c7f1f85c530..57249df65fe31 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -4857,6 +4857,77 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>  	goto out_put_evlist;
>  }
>  
> +#ifdef HAVE_LIBTRACEEVENT
> +static void tep_print_arg__set_bitmask_list(struct tep_print_arg *arg)
> +{
> +	for (; arg; arg = arg->next) {
> +		switch (arg->type) {

[Severity: Low]
Will this cause build failures on future libtraceevent updates?

The commit message mentions adding all enum values of tep_print_arg_type to
satisfy -Wswitch-enum. However, because tep_print_arg_type is an external
enum from libtraceevent, any future updates to libtraceevent that add new
values to this enum will break the perf build due to -Werror.

> +		case TEP_PRINT_BITMASK:
> +			arg->type = TEP_PRINT_CPUMASK;

[Severity: Low]
Can this cause a build failure on older libtraceevent versions?

If TEP_PRINT_CPUMASK is missing from the installed system libtraceevent
headers, this unconditional assignment will fail to compile.

> +			break;
> +		case TEP_PRINT_OP:
> +			tep_print_arg__set_bitmask_list(arg->op.left);
> +			tep_print_arg__set_bitmask_list(arg->op.right);
> +			break;
> +		case TEP_PRINT_FUNC:
> +			tep_print_arg__set_bitmask_list(arg->func.args);
> +			break;
> +		case TEP_PRINT_FLAGS:
> +			tep_print_arg__set_bitmask_list(arg->flags.field);
> +			break;
> +		case TEP_PRINT_SYMBOL:
> +			tep_print_arg__set_bitmask_list(arg->symbol.field);
> +			break;
> +		case TEP_PRINT_HEX:
> +			tep_print_arg__set_bitmask_list(arg->hex.field);
> +			tep_print_arg__set_bitmask_list(arg->hex.size);
> +			break;
> +		case TEP_PRINT_INT_ARRAY:
> +			tep_print_arg__set_bitmask_list(arg->int_array.field);
> +			tep_print_arg__set_bitmask_list(arg->int_array.count);
> +			tep_print_arg__set_bitmask_list(arg->int_array.el_size);
> +			break;
> +		case TEP_PRINT_TYPE:
> +			tep_print_arg__set_bitmask_list(arg->typecast.item);
> +			break;
> +		case TEP_PRINT_DYNAMIC_ARRAY:
> +			tep_print_arg__set_bitmask_list(arg->dynarray.index);
> +			break;
> +		case TEP_PRINT_NULL:
> +		case TEP_PRINT_ATOM:
> +		case TEP_PRINT_FIELD:
> +		case TEP_PRINT_STRING:
> +		case TEP_PRINT_BSTRING:
> +		case TEP_PRINT_DYNAMIC_ARRAY_LEN:
> +		case TEP_PRINT_HEX_STR:

[Severity: Medium]
Does this skip the AST traversal for TEP_PRINT_HEX_STR nodes?

The fallthrough here prevents the recursive traversal of arg->hex.field and
arg->hex.size for TEP_PRINT_HEX_STR nodes, which share the same structural
payload as TEP_PRINT_HEX. Nested bitmask evaluations inside a hex string
length or field argument could be silently ignored.

> +		case TEP_PRINT_CPUMASK:
> +		default:
> +			break;
> +		}
> +	}
> +}
> +

[ ... ]

> @@ -4908,6 +4979,8 @@ static int trace__replay(struct trace *trace)
>  	if (err)
>  		goto out;
>  
> +	trace__init_tp_bitmask_list(trace, session->evlist);

[Severity: Medium]
Is --bitmask-list broken for pipe mode streams?

In pipe mode, the event list is populated dynamically from synthesized
events during perf_session__process_events(), which happens later in this
function. By calling trace__init_tp_bitmask_list() here, the initialization
operates on an empty evlist, skipping the AST transformation.

> +
>  	evsel = evlist__find_tracepoint_by_name(session->evlist, "raw_syscalls:sys_enter");
>  	trace->syscalls.events.sys_enter = evsel;
>  	/* older kernels have syscalls tp versus raw_syscalls */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002213145.333100-1-atomlin@atomlin.com?part=1