Re: [PATCH v4 01/14] perf stat: Introduce core generic print traversal engine and header stubs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The traversal loops in `perf_stat__print_cb()` incorrectly group `AGGR_NONE` and `AGGR_THREAD` under the `aggr_idx` outer loop, structurally inverting legacy output ordering.
- [Medium] The function `is_basic_shadow_metric()` is defined but never called, leaving the intended metric allowlist filtering completely unimplemented.
- [Medium] The new traversal engine `perf_stat__print_cb()` omits the `should_skip_zero_counter()` check, which will cause it to incorrectly emit unwanted zero-valued and unsupported counters once the formatting callbacks are implemented.
- [Medium] `perf_stat__get_aggr_id_char()` unconditionally writes to `buf[0]` in its default fallback path without validating if `buf_size > 0`.
- [Medium] The newly added `--new` command-line flag silently suppresses all performance counter output.
--

--- Patch [1]: [PATCH v4 01/14] perf stat: Introduce core generic print traversal engine and header stubs ---
commit 0d5894df730b71c5e2e9d44337a7acb89dd22010
Author: Ian Rogers <irogers@google.com>

perf stat: Introduce core generic print traversal engine and header stubs

This patch introduces the initial infrastructure for decoupling the perf stat
printing API. It implements the generic traversal driver and format-agnostic
shared helpers, and adds empty format-specific stubs.

> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index a04466ea3b0a0..6768ba256a5a2 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
[ ... ]
> @@ -1091,7 +1094,10 @@ static void print_counters(struct timespec *ts, int argc, const char **argv)
>  	if (quiet)
>  		return;
>  
> -	evlist__print_counters(evsel_list, &stat_config, &target, ts, argc, argv);
> +	if (use_perf_stat_print)
> +		perf_stat__print(evsel_list, &stat_config, &target, ts, argc, argv);

[Severity: Medium]
The --new command-line flag routes execution to the new perf_stat__print() API,
which currently relies on empty stubs like perf_stat__print_std() that
immediately return 0. If a user runs perf stat --new, will the tool execute
the workload but silently print absolutely no counter statistics?

> +	else
> +		evlist__print_counters(evsel_list, &stat_config, &target, ts, argc, argv);
>  }
[ ... ]
> diff --git a/tools/perf/util/stat-print.c b/tools/perf/util/stat-print.c
> new file mode 100644
> index 0000000000000..419fecfd91988
> --- /dev/null
> +++ b/tools/perf/util/stat-print.c
[ ... ]
> +int perf_stat__get_aggr_id_char(const struct perf_stat_config *config, struct evsel *evsel,
> +				struct aggr_cpu_id id, char *buf, size_t buf_size)
> +{
> +	switch (config->aggr_mode) {
[ ... ]
> +	case AGGR_GLOBAL:
> +	case AGGR_UNSET:
> +	case AGGR_MAX:
> +	default:
> +		break;
> +	}
> +	buf[0] = '\0';

[Severity: Medium]
In the default fallback path, buf[0] is written unconditionally without
verifying if buf_size > 0. If a caller passes buf_size == 0 or buf == NULL,
could this lead to an out-of-bounds write or a NULL pointer dereference?

> +	return -1;
> +}
[ ... ]
> +static bool __maybe_unused is_basic_shadow_metric(const char *name)
> +{

[Severity: Medium]
The commit message mentions implementing the basic metrics allowlist filter
using is_basic_shadow_metric(), but the function is marked static, silenced
with __maybe_unused, and never called in the traversal logic. Will this
leave the intended filtering logic completely unimplemented?

> +	static const char *const basic_metrics[] = {
> +		"insn_per_cycle",   "branch_miss_rate",	      "branch_frequency",
> +		"cycles_frequency", "page_faults_per_second", "migrations_per_second",
> +		"cs_per_second",    "CPUs_utilized",
> +	};
[ ... ]
> +int perf_stat__print_cb(struct evlist *evlist, struct perf_stat_config *config,
> +			const struct target *target __maybe_unused,
> +			const struct timespec *ts __maybe_unused, int argc __maybe_unused,
> +			const char **argv __maybe_unused,
> +			const struct perf_stat_print_callbacks *cb, void *ctx)
> +{
[ ... ]
> +	switch (config->aggr_mode) {
> +	case AGGR_GLOBAL:
> +	case AGGR_NONE:
> +	case AGGR_SOCKET:
> +	case AGGR_DIE:
> +	case AGGR_CLUSTER:
> +	case AGGR_CACHE:
> +	case AGGR_CORE:
> +	case AGGR_THREAD:
> +	case AGGR_NODE:
> +		if (config->aggr_map) {
> +			cpu_aggr_map__for_each_idx(aggr_idx, config->aggr_map) {
> +				evlist__for_each_entry(evlist, counter) {

[Severity: High]
In the legacy implementation, AGGR_NONE and AGGR_THREAD modes iterate over
the events in the outer loop and the CPUs/threads in the inner loop,
grouping the output by event.

Here, they are grouped under the same block as AGGR_CORE, placing CPUs/threads
in the outer loop and events in the inner loop.

Will this structurally invert the output ordering and break backward
compatibility for tools parsing the standard or CSV output?

> +					struct perf_stat_evsel *ps = counter->stats;
> +					u64 val = 0, ena = 0, run = 0;
[ ... ]
> +					if (perf_stat__skip_metric_event(counter))
> +						continue;
> +
> +					if (cb->print_event) {

[Severity: Medium]
The legacy implementation uses should_skip_zero_counter() to suppress
output for zero-valued counters that are disabled, unsupported, or explicitly
hidden. In the new perf_stat__print_cb() engine, this check appears to be
missing before calling cb->print_event().

Could this cause the output to be polluted with zeroes and '<not counted>'
strings once the callbacks are implemented?

> +						double stdev_pct = 0.0;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716070303.507066-1-irogers@google.com?part=1