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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Metric-only CSV formatting uses an arbitrary event from the evlist to generate aggregation ID prefixes for all metrics, ignoring the metric's actual parent event.
- [High] In metric-only mode, missing or failing metrics cause CSV column misalignment.
- [High] CSV output silently drops the stdev_pct (noise) and cgroup columns, breaking backward compatibility for CSV parsers.
- [Medium] Casting away const to mutate config state.
- [Medium] csv_print_event() fails to filter out zero-valued events using should_skip_zero_counter(), leading to a flood of useless 0-count lines for uncore events.
- [Medium] The aggr_idx field of struct queued_metric is left uninitialized when allocated in csv_print_metric.
- [Low] Missing direct POSIX header inclusions for <time.h> and <stdbool.h>.

Pre-existing issues:
- [High] Potential null pointer dereference in print_aggr_id_csv when thread map or comm string is missing.
--

--- Patch [7]: [PATCH v3 07/14] perf stat: Implement CSV formatting callbacks ---
commit 09929d045ae8af174aed0263f453cda8542e4ec1
Author: Ian Rogers <irogers@google.com>

perf stat: Implement CSV formatting callbacks

This patch implements CSV output formatting callbacks inside
util/stat-print-csv.c, replacing the empty stubs. It defines formatting
structures to buffer traversal streams and fully encapsulates CSV queued
lists lifecycle and deallocations.

> diff --git a/tools/perf/util/stat-print-csv.c b/tools/perf/util/stat-print-csv.c
> index ee86d07636ea6..35cd4505c6c16 100644
> --- a/tools/perf/util/stat-print-csv.c
> +++ b/tools/perf/util/stat-print-csv.c

[ ... ]

> +	case AGGR_THREAD:
> +		fprintf(output, "%s-%d%s",
> +			perf_thread_map__comm(evsel->core.threads, id.thread_idx),

[Severity: High]
This isn't a bug introduced by this patch, but can this unconditionally
dereference evsel->core.threads? If the thread map or comm string is
missing, it could cause a null pointer dereference.

> +			perf_thread_map__pid(evsel->core.threads, id.thread_idx), sep);
> +		break;

[ ... ]

> +static int csv_print_event(void *ctx, const struct perf_stat_config *config __maybe_unused,
> +			   struct evsel *evsel, int aggr_idx, u64 val, u64 ena, u64 run,
> +			   double stdev_pct __maybe_unused)

[Severity: Medium]
Should this code filter out zero-valued events using should_skip_zero_counter()
before enqueueing the event? Without it, uncore zero-counts might bypass
filtering and pollute the output.

[Severity: High]
Does this silently drop the stdev_pct (noise) and cgroup columns? The noise
variance is explicitly ignored here, which could break backward compatibility
for downstream CSV parsers.

> +{
> +	struct csv_print_state *ps = ctx;
> +	struct queued_event *ev = malloc(sizeof(*ev));

[ ... ]

> +static int csv_print_metric(void *ctx, const struct perf_stat_config *config __maybe_unused,
> +			    struct evsel *evsel __maybe_unused, int aggr_idx __maybe_unused,
> +			    const char *name, const char *unit, double val,
> +			    enum metric_threshold_classify thresh __maybe_unused)
> +{
> +	struct csv_print_state *ps = ctx;
> +	struct queued_metric *b;
> +
> +	if (!ps->current_event)
> +		return 0;
> +
> +	if (evsel != ps->current_event->evsel) {
> +		pr_err("decoupled print engine: temporal coupling violation: evsel mismatch!\n");
> +		return -EINVAL;
> +	}
> +
> +	b = malloc(sizeof(*b));

[Severity: Medium]
Is the aggr_idx field of struct queued_metric left uninitialized after
allocation? Leaving this uninitialized could cause unexpected behavior if
accessed later.

> +	if (!b)
> +		return -ENOMEM;
> +
> +	b->name = strdup(name);

[ ... ]

> +		fprintf(output, "%s", ev->name);
> +
> +		if (ev->run != ev->ena)
> +			enabled_percent = 100.0 * ev->run / ev->ena;
> +		fprintf(output, "%s%" PRIu64 "%s%.2f", sep, ev->run, sep, enabled_percent);

[Severity: High]
Are we missing the stdev_pct (noise) and cgroup columns in the final output?
Event printing finishes here without including them, which could break
backward compatibility for CSV parsers.

> +
> +		/* Print metrics */
> +		has_metrics = false;

[ ... ]

> +static int csv_metric_only_print_metric(void *ctx,
> +					const struct perf_stat_config *config __maybe_unused,
> +					struct evsel *evsel __maybe_unused, int aggr_idx,

[Severity: High]
Is the actual evsel parameter ignored here rather than being saved? This seems
to discard the metric's actual parent event.

> +					const char *name, const char *unit, double val,
> +					enum metric_threshold_classify thresh __maybe_unused)
> +{
> +	struct csv_metric_only_print_state *ps = ctx;

[ ... ]

> +				fprintf(output, "%s%s", header_name, sep);
> +				free(header_name);
> +			}
> +		}
> +		fprintf(output, "\n");
> +		((struct perf_stat_config *)config)->metric_only_headers_printed = true;

[Severity: Medium]
Is it safe to cast away const to mutate the config state here? This modifies
metric_only_headers_printed on a const pointer, which might cause undefined
behavior.

> +	}
> +
> +	/* Print values */
> +	list_for_each_entry_safe(b, tmp, &ps->queued_metrics, list) {
> +		if (b->aggr_idx != current_aggr) {
> +			if (current_aggr != -2)
> +				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) {
> +				struct aggr_cpu_id id = config->aggr_map->map[current_aggr];
> +				struct evsel *mock_evsel = list_first_entry(
> +					&ps->evlist->core.entries, struct evsel, core.node);

[Severity: High]
Does this code use an arbitrary event from the evlist to generate aggregation
ID prefixes for all metrics, ignoring the metric's actual parent event?
Relying on a mocked event's thread maps could result in incorrectly generated
CSV aggregation headers for heterogeneous evlists.

> +				int aggr_nr = 0;
> +
> +				if (mock_evsel->stats && mock_evsel->stats->aggr)
> +					aggr_nr = mock_evsel->stats->aggr[current_aggr].nr;
> +
> +				print_aggr_id_csv(config, output, mock_evsel, id, aggr_nr);
> +			}
> +		}
> +		fprintf(output, "%.1f%s", b->val, sep);

[Severity: High]
Will missing or failing metrics cause CSV column misalignment here? Values
seem to be printed sequentially without column matching, which could shift
all subsequent row elements leftward if a dynamic metric is omitted.

> +
> +		list_del(&b->list);
> +		free(b->name);

[ ... ]

> +int perf_stat__print_csv(struct evlist *evlist, const struct perf_stat_config *config,
> +			 const struct target *target, const struct timespec *ts, int argc,
> +			 const char **argv)
> +{
> +	if (config->metric_only) {
> +		struct csv_metric_only_print_state ps = {
> +			.fp = config->output,
> +			.sep = config->csv_sep,
> +			.evlist = evlist,
> +		};
> +		if (config->interval && ts) {
> +			scnprintf(ps.timestamp, sizeof(ps.timestamp), "%lu.%09lu%s",
> +				  (unsigned long)ts->tv_sec, ts->tv_nsec, config->csv_sep);

[Severity: Low]
Are we missing direct POSIX header inclusions for <time.h> and <stdbool.h>?
struct timespec and boolean types are used in this file without explicit
inclusion, which violates musl libc compilation requirements.

> +		} else {
> +			ps.timestamp[0] = '\0';
> +		}

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