Re: [RFC PATCH v1 07/14] perf stat: Implement CSV formatting callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The CSV metric-only mode outputs dynamic data values (timestamps and aggregation IDs) instead of static column labels in the header row, and prints this header unconditionally on every interval.
- [Medium] The data row prefix (timestamp and aggregation ID) is completely skipped for the first row if its aggregation index is -1, causing column misalignment.
--

commit 93e85eab3ddfd8a1be43f26569da0f314f8e2adc
Author: Ian Rogers <irogers@google.com>

perf stat: Implement CSV formatting callbacks

This commit implements CSV output formatting callbacks in stat-print-csv.c.
It defines queued DOM nodes to buffer traversal streams and encapsulates CSV
lists lifecycle. It also incorporates interval-mode timestamp printing
support.

> diff --git a/tools/perf/util/stat-print-csv.c b/tools/perf/util/stat-print-csv.c
> --- a/tools/perf/util/stat-print-csv.c
> +++ b/tools/perf/util/stat-print-csv.c
[ ... ]
> +static int csv_metric_only_print_end(void *ctx, const struct perf_stat_config *config)
> +{
> +	struct csv_metric_only_print_state *ps = ctx;
> +	FILE *output = ps->fp;
> +	const char *sep = ps->sep;
> +	struct queued_metric *b, *tmp;
> +	int first_aggr = -1;
> +	int current_aggr = -1;
> +	int ret = 0;
> +	int err;
> +
> +	if (list_empty(&ps->queued_metrics))
> +		return 0;
> +
> +	first_aggr = list_first_entry(&ps->queued_metrics, struct queued_metric, list)->aggr_idx;
> +
> +	/* Print interval timestamp header if configured */
> +	if (config->interval && ps->timestamp[0])
> +		fprintf(output, "%s", ps->timestamp);
> +
> +	/* Print aggregation prefix header in CSV metric-only mode */
> +	if (config->aggr_map && first_aggr >= 0) {
> +		struct aggr_cpu_id id = config->aggr_map->map[first_aggr];
> +		struct evsel *mock_evsel =
> +			list_first_entry(&ps->evlist->core.entries, struct evsel, core.node);
> +		int aggr_nr = 0;
> +
> +		if (mock_evsel->stats && mock_evsel->stats->aggr)
> +			aggr_nr = mock_evsel->stats->aggr[first_aggr].nr;
> +
> +		print_aggr_id_csv(config, output, mock_evsel, id, aggr_nr);
> +	}

[Severity: Medium]
Does this output dynamic data values, such as current timestamps and
runtime aggregation IDs, into the header row instead of static column
labels like time or core?

Also, since this header block runs every time csv_metric_only_print_end()
is called, will the header row be unconditionally printed on every interval
rather than just once at the start?

> +
> +	/* Print headers */
> +	list_for_each_entry(b, &ps->queued_metrics, list) {
[ ... ]
> +	}
> +	fprintf(output, "\n");
> +
> +	/* Print values */
> +	list_for_each_entry_safe(b, tmp, &ps->queued_metrics, list) {
> +		if (b->aggr_idx != current_aggr) {
> +			if (current_aggr != -1)
> +				fprintf(output, "\n");
> +			current_aggr = b->aggr_idx;
> +			if (config->interval && ps->timestamp[0])
> +				fprintf(output, "%s", ps->timestamp);
> +			if (config->aggr_map && current_aggr >= 0) {

[Severity: Medium]
Since current_aggr is initialized to -1, what happens if the first
metric's aggr_idx is also -1, such as with global aggregation?

It appears the condition checking if b->aggr_idx != current_aggr would
evaluate to false, completely bypassing this block.

Could this cause the interval timestamp and aggregation ID prefix to be
skipped for the data row, leading to permanent column misalignment since
the timestamp was already printed in the header?

> +				struct aggr_cpu_id id = config->aggr_map->map[current_aggr];

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