Re: [PATCH v2 6/6] perf c2c: print memory region data with stdio output

On Tue, 2026-07-14 at 00:57 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [Medium] The perf c2c tool structure omits the `.feature` callback,
> breaking `perf_env` initialization in pipe mode.
> - [Low] Implicit reliance on POSIX libc headers violates musl build
> compatibility rules (missing `<stdio.h>`).
> --
> 
> --- Patch [6]: [PATCH v2 6/6] perf c2c: print memory region data with
> stdio output ---
> commit 210b29f2c6ae9c7cc910fd9994d1e0b92078eec2
> Author: Thomas Falcon <[email protected]>
> 
>     perf c2c: print memory region data with stdio output
>     
>     Print memory range data in perf-c2c subcommand. Only available in
> stdio
>     mode currently. If memory ranges are not supported or present,
> print
>     nothing.
> 
> > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > index cba2e9f21ae7c..68348a93bd331 100644
> > --- a/tools/perf/builtin-c2c.c
> > +++ b/tools/perf/builtin-c2c.c
> > @@ -2634,6 +2634,34 @@ static void print_c2c_info(FILE *out, struct
> > perf_session *session)
> >  	fprintf(out, "  Cacheline data grouping           : %s\n",
> > c2c.cl_sort);
> >  }
> >  
> > +
> > +static void print_memory_ranges_info(FILE *out, struct
> > perf_session *session)
> > +{
> 
> [Severity: Low]
> Does this code rely on implicit header inclusion?
> 
> The new print_memory_ranges_info() function uses FILE and fprintf,
> but the
> patch doesn't explicitly include <stdio.h>. Can this cause the code
> to fail to
> compile under musl libc due to missing declarations?

Existing functions (print_c2c__display_stats(),
print_shared_cacheline_info(), print_c2c_info()) use FILE and fprintf()
without any issue.

> 
> > +	struct perf_env *env = perf_session__env(session);
> > +	int nr_ranges = 0;
> > +
> > +	if (!perf_header__has_feat(&session->header,
> > HEADER_MEMORY_RANGES))
> > +		return;
> > +	nr_ranges = env->nr_memory_ranges;
> > +	if (nr_ranges == 0) {
> > +		pr_debug("No memory ranges found, skipping\n");
> > +		return;
> > +	}
> > +
> > +	fprintf(out, "\n");
> > +	fprintf(out,
> > "=================================================\n");
> > +	fprintf(out, "                  Memory
> > Ranges                  \n");
> > +	fprintf(out,
> > "=================================================\n");
> > +
> > +	for (int i = 0; i < nr_ranges; i++) {
> > +		struct memory_range *r = &env->memory_ranges[i];
> > +
> > +		fprintf(out, "Range %d: [0x%016" PRIx64 "-0x%016"
> > PRIx64 "] Node %d, local region id %u, remote region id %u\n",
> > +			i, r->base, r->base + r->length - 1, r-
> > >node,
> > +			r->local_region_id, r->remote_region_id);
> > +	}
> > +}
> 
> [Severity: Medium]
> Will this silently fail to display memory ranges when perf is run in
> pipe
> mode?

Pipe mode is not supported on perf-c2c.

Thanks,
Tom

> 
> The tool initializes its callbacks in perf_c2c__report() like this:
> 
> 	perf_tool__init(&c2c.tool, /*ordered_events=*/true);
> 	c2c.tool.sample		= process_sample_event;
> 	c2c.tool.mmap		= perf_event__process_mmap;
> 
> Because the .feature callback is missing here, feature events are
> dropped in
> pipe mode, so perf_env is not populated with memory ranges. Although
> the
> callback omission was pre-existing, doesn't it actively break the
> functionality introduced in this patch?
> 
> [ ... ]
>