Re: [PATCH] perf stat: Include PMU name and split uncore events per PMU in metric-only JSON output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Bypassing `print_metric_begin` for tool events produces invalid, corrupted JSON and CSV output in `--metric-only` mode.

Pre-existing issues:
- [High] Missing bounds check on `aggr_idx` after searching `config->aggr_map` results in an out-of-bounds array read.
--

--- Patch [1]: [PATCH] perf stat: Include PMU name and split uncore events per PMU in metric-only JSON output ---
commit 418eb65331465e855519851f0438c3a1fe047563
Author: Chun-Tse Shao <ctshao@google.com>

perf stat: Include PMU name and split uncore events per PMU in metric-only JSON output

This commit modifies perf stat to include the PMU name in the metric-only JSON
output when running in no-aggr mode. It ensures that when an uncore event spans
multiple PMU instances, each instance is printed on a separate line with the
correct PMU identifier. The JSON linting script is also updated to recognize
the new PMU key.

> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index f94f1324d24ac..d7f1feb638784 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
[ ... ]
> @@ -1146,10 +1149,24 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>  
>  			os->evsel = counter;
>  			os->id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
> -			if (first) {
> -				print_metric_begin(config, evlist, os, aggr_idx);
> -				first = false;
> +
> +			if (config->metric_only) {
> +				struct perf_pmu *pmu = counter->pmu;
> +
> +				if (!evsel__is_tool(counter)) {

[Severity: Medium]
Does bypassing the print_metric_begin() initialization for tool events cause
invalid JSON output?

If a metric uses a tool event like duration_time as the metric leader,
skipping print_metric_begin() means the opening bracket, timestamp, and CPU ID
are not printed. This can leave the metric key-value pair as a bare string,
followed by a mid-stream bracket when a subsequent hardware PMU event is
processed.

> +					if (config->json_output && line_open && pmu != last_pmu) {
> +						print_metric_end(config, os);
> +						line_open = false;
> +					}
> +					if (!line_open) {
> +						print_metric_begin(config, evlist, os,
> +								   aggr_idx, counter);
> +						line_open = true;
> +						last_pmu = pmu;
> +					}
> +				}
>  			}
> +
>  			val = ps->aggr[aggr_idx].counts.val;

[Severity: High]
This is a pre-existing issue, but does this code read past the end of the
ps->aggr array if the CPU is not found in the aggregation map?

In print_no_aggr_metric(), there is a loop right before this block that
searches for the CPU:

cpu_aggr_map__for_each_idx(aggr_idx, config->aggr_map) {
	if (config->aggr_map->map[aggr_idx].cpu.cpu == cpu.cpu)
		break;
}

If a system lacks complete sysfs CPU topology, aggr_cpu_id__cpu() might return
an empty ID, and the CPU may not be found in config->aggr_map.

Could the loop terminate with aggr_idx == config->aggr_map->nr, causing this
line to unconditionally read out of bounds?

>  			ena = ps->aggr[aggr_idx].counts.ena;
>  			run = ps->aggr[aggr_idx].counts.run;

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