Re: [PATCH v3 4/6] perf tools: Show memory region in perf-c2c subcommand

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:
- [Medium] Unconditional header span and width changes break the perf c2c U=
I layout for older or unsupported perf.data files.
- [Medium] perf_c2c__report incorrectly returns 0 (success) when asprintf m=
emory allocation fails.
--

--- Patch [4]: [PATCH v3 4/6] perf tools: Show memory region in perf-c2c su=
bcommand ---
commit a204da119426634f593eeb8162efcca17a1e1e85
Author: Dapeng Mi <[email protected]>

perf tools: Show memory region in perf-c2c subcommand

This commit adds a memory region field to the cacheline list view in the
perf c2c subcommand output. It parses the memory region from memory info
and dynamically adds the column if the HEADER_MEMORY_RANGES feature is
present in the perf.data file.

Link: https://lore.kernel.org/all/20260114011750.350569-1-dapeng1.mi@linux.=
intel.com/

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..6954a48c60e46 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -1425,7 +1466,7 @@ cl_idx_empty_entry(struct perf_hpp_fmt *fmt, struct=
 perf_hpp *hpp,
>  	}
> =20
>  static struct c2c_dimension dim_dcacheline =3D {
> -	.header		=3D HEADER_SPAN("--- Cacheline ----", "Address", 2),
> +	.header		=3D HEADER_SPAN("--- Cacheline ----", "Address", 3),

[Severity: Medium]
Will this unconditional header span break the UI layout when the memory reg=
ion
column is excluded?

Because has_mem_regions conditionally adds the column, if the perf.data file
lacks the HEADER_MEMORY_RANGES feature, this static span of 3 might swallow
the headers of subsequent columns.

>  	.name		=3D "dcacheline",
>  	.cmp		=3D dcacheline_cmp,
>  	.entry		=3D dcacheline_entry,

[ ... ]

> @@ -2898,8 +2948,9 @@ static int ui_quirks(void)
>  	/* Fix the zero line for dcacheline column. */
>  	buf =3D fill_line(chk_double_cl ? "Double-Cacheline" : "Cacheline",
>  				dim_dcacheline.width +
> +				dim_dcacheline_mem_region.width +
>  				dim_dcacheline_node.width +
> -				dim_dcacheline_count.width + 4);
> +				dim_dcacheline_count.width + 6);

[Severity: Medium]
Could this width calculation also cause visual artifacts when the memory
region column is absent?

Similar to the header span, the zero-line text width unconditionally includ=
es
the new column's width and spacing, which could pad the zero line excessive=
ly
when the column is not rendered.

>  	if (!buf)
>  		return -ENOMEM;

[ ... ]

> @@ -3370,50 +3370,50 @@ static int perf_c2c__report(int argc, const char =
**argv)
> =20
>  	has_mem_regions =3D perf_header__has_feat(&session->header,
>  						HEADER_MEMORY_RANGES);
> =20
>  	if (c2c.display !=3D DISPLAY_SNP_PEER) {
>  		ret =3D asprintf(&output_str,
>  			       "cl_idx,"
>  			       "dcacheline,"

[ ... ]

>  			       has_mem_regions ? "dcacheline_mem_region," : "");
>  		if (ret < 0)
>  			goto out_mem2node;

[Severity: Medium]
Does this error path accidentally return success when asprintf() fails?

The original code sets err =3D 0 at the beginning of perf_c2c__report(),
and this goto statement jumps to cleanup without setting err =3D ret or sim=
ilar,
so the function will return 0 even though memory allocation failed.

>  	} else {
>  		ret =3D asprintf(&output_str,
>  			       "cl_idx,"
>  			       "dcacheline,"

[ ... ]

>  			       has_mem_regions ? "dcacheline_mem_region," : "");
>  		if (ret < 0)
>  			goto out_mem2node;

[Severity: Medium]
Does this alternative display layout's error path also accidentally return
success when asprintf() fails?

Similar to the previous block, err is not updated before jumping to cleanup.

>  	}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803204540.1003=
[email protected]?part=3D4