Re: [PATCH] perf stat: Make metric only column line up with header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Corruption of default metricgroup alignment in standard (non-metric-only) output.
- [High] Extraneous trailing space added to CSV output breaks machine readability.
- [Medium] Off-by-one column misalignment in metric-only standard output.
--

commit 29839374a38b0f40e2eb452aeaffa530277247ff
Author: Andi Kleen <ak@linux.intel.com>

perf stat: Make metric only column line up with header

This commit attempts to fix column alignment in the --metric-only output
mode. It simplifies the column width computation and adjusts string
formatting for both standard and CSV outputs.

> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 993f4c4b8f442..402611ecaf423 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -580,16 +580,13 @@ static void print_metricgroup_header_std(struct perf_stat_config *config,
>  					 const char *metricgroup_name)
>  {
[ ... ]
> -	n = fprintf(config->output, " %*s", EVNAME_LEN, metricgroup_name);
> -
> -	fprintf(config->output, "%*s", MGROUP_LEN + config->unit_width + 2 - n, "");
> +	fprintf(config->output, " %*s", config->metric_only_len, metricgroup_name);
>  }

Does this change break tabular alignment for default metricgroups in standard
non-metric-only mode?

The commit message states that EVNAME_LEN and MGROUP_LEN are not useful in
the metric only context. However, print_metricgroup_header_std() is
completely bypassed in --metric-only mode.

In standard mode, this function uses these lengths to pad the metricgroup
name and align it with the preceding counter values. Replacing this padding
with config->metric_only_len could shift all subsequent metrics to the left.

> @@ -599,19 +596,20 @@ static void print_metric_only(struct perf_stat_config *config,
[ ... ]
> +	olen = snprintf(str, sizeof(str), fmt ?: "", val);
>  	color_snprintf(str, sizeof(str), color ?: "", fmt ?: "", val);
> -	fprintf(out, "%*s ", mlen, str);
> +	fprintf(out, "%*s%s", max_t(int, mlen - olen, 1), "", str);
>  	os->first = false;
>  }

Could this lead to an off-by-one misalignment in the data columns?

This new formatting removes the trailing space and outputs exactly mlen
visual characters. However, the header formatting in print_metric_header()
was left unchanged and still outputs a trailing space:

tools/perf/util/stat-display.c:print_metric_header() {
    ...
    fprintf(os->fh, "%*s ", config->metric_only_len, unit);
    ...
}

Because each data column is exactly one character narrower than its header,
will the data rows progressively drift to the left compared to the headers?

> @@ -633,7 +631,7 @@ static void print_metric_only_csv(struct perf_stat_config *config __maybe_unused
>  	while (isdigit(*ends) || *ends == '.')
>  		ends++;
>  	*ends = 0;
> -	fprintf(out, "%s%s", vals, config->csv_sep);
> +	fprintf(out, "%s%s ", vals, config->csv_sep);
>  	os->first = false;
>  }

Does adding this trailing space after the CSV separator break machine
readability for strict CSV parsers?

The CSV header output does not appear to add this space, which might cause a
format mismatch between the headers and the data rows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512234345.554166-1-ak@linux.intel.com?part=1