Re: [PATCH v4 10/14] perf stat: Implement streaming JSON formatting callbacks

[sashiko-bot@kernel.org]

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

New issues:
- [High] The JSON output completely ignores cgroup identification and statistical variance data for events.
- [Low] The state structs contain fields that are initialized but never used by any print callbacks.

Pre-existing issues:
- [High] Missing bounds and NULL check on `evsel->core.threads` in `print_aggr_id_json` when formatting `AGGR_THREAD` JSON output.
- [High] Passing a potentially NULL string from `perf_thread_map__comm()` to the `%s` format specifier causes undefined behavior and leads to a crash on musl libc systems.
--

--- Patch [10]: [PATCH v4 10/14] perf stat: Implement streaming JSON formatting callbacks ---
commit 77f0723329ce102d572ddc7616c0e8a06c6c64f1
Author: Ian Rogers <irogers@google.com>

perf stat: Implement streaming JSON formatting callbacks

This patch implements streaming JSON formatting callbacks inside
util/stat-print-json.c, replacing the empty stubs introduced in Commit 1. It
delivers a highly optimized, zero-allocation, and streaming print engine for
JSON normal and metric-only modes.

Assisted-by: Antigravity:gemini-3.5-flash
Acked-by: Chun-Tse Shao <ctshao@google.com>

> diff --git a/tools/perf/util/stat-print-json.c b/tools/perf/util/stat-print-json.c
> index 4690c4286fd2a..7ed00aae89f4b 100644
> --- a/tools/perf/util/stat-print-json.c
> +++ b/tools/perf/util/stat-print-json.c
[ ... ]
> +struct json_print_state {
> +	FILE *fp;
> +	char timestamp[64];
> +};
> +
> +/**
> + * struct json_metric_only_print_state - Metric-only print state context for JSON output.
> + * @fp: File descriptor to output to.
> + * @timestamp: Formatted interval timestamp (optional).
> + * @evlist: Evlist to query entries from.
> + * @last_aggr_idx: The aggregation index of the last printed metric.
> + * @first_in_group: Whether the current metric is the first in its group.
> + */
> +struct json_metric_only_print_state {
> +	FILE *fp;
> +	char timestamp[64];
> +	struct evlist *evlist;

[Severity: Low]
Are the fp field in json_print_state and the evlist field in
json_metric_only_print_state needed? It appears they are initialized in
perf_stat__print_json but never actually read or used by any print callbacks,
which introduces unnecessary dead state.

> +	int last_aggr_idx;
> +	bool first_in_group;
> +};
[ ... ]
> +	case AGGR_THREAD:
> +		fprintf(output, "\"thread\" : \"%s-%d\", ",
> +			perf_thread_map__comm(evsel->core.threads, id.thread_idx),
> +			perf_thread_map__pid(evsel->core.threads, id.thread_idx));

[Severity: High]
This is a pre-existing issue, but is it possible for evsel->core.threads to
be NULL or id.thread_idx to be out of bounds here? Calling
perf_thread_map__comm without validating the index can lead to a deterministic
segmentation fault.

[Severity: High]
This isn't a bug introduced by this patch, but does perf_thread_map__comm
safely return a non-NULL string in all cases? If the comm string pointer
is NULL, passing it to the %s format specifier in fprintf causes a
segmentation fault on systems using musl libc.

> +		break;
[ ... ]
> +static int json_print_event(void *ctx, struct perf_stat_config *config,
> +			    struct evsel *evsel, int aggr_idx, u64 val,
> +			    u64 ena, u64 run, double stdev_pct __maybe_unused,
> +			    const char *cgrp __maybe_unused)

[Severity: High]
Does ignoring the stdev_pct and cgrp parameters result in a regression of the
JSON output parity? The baseline code formatted cgroup and variance into the
JSON output, so marking these as unused here seems to silently drop standard
deviation and cgroup data, which could break automated profiling pipelines.

> +{

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