Re: [PATCH 01/14] perf c2c: extract shared data structures into c2c.h

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Fri, Jun 26, 2026 at 07:13:52AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Medium] Missing feature fallback inline stubs for SLANG-dependent functions in header.
> - [Medium] Asymmetric memory management API leading to potential resource leaks.
> - [Medium] Global namespace pollution with generic function names.
> --
> 
> commit e451cf21befaafcbe2925718151023dd5ccba2cc
> Author: Jiebin Sun <jiebin.sun@intel.com>
> 
> perf c2c: extract shared data structures into c2c.h
> 
> This patch moves c2c_hists, compute_stats, c2c_hist_entry, and perf_c2c
> structure definitions from builtin-c2c.c into a new shared header c2c.h.
> This allows the upcoming function view browser to reuse these types.
> 
> > diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> > index c9584dbedf77a..33271554e354b 100644
> > --- a/tools/perf/builtin-c2c.c
> > +++ b/tools/perf/builtin-c2c.c
> [ ... ]
> > @@ -141,9 +76,9 @@ static const struct option c2c_options[] = {
> >  	OPT_END()
> >  };
> >  
> > -static struct perf_c2c c2c;
> > +struct perf_c2c c2c;
> >  
> > -static void *c2c_he_zalloc(size_t size)
> > +void *c2c_he_zalloc(size_t size)
> >  {
> >  	struct c2c_hist_entry *c2c_he;
> 
> [Severity: Medium]
> By exporting c2c_he_zalloc(), is there a risk of memory leaks since a
> corresponding free function is not exported?
> 
> Looking at the rest of c2c_he_zalloc() in builtin-c2c.c, it allocates several
> sub-structures:
> 
>     c2c_he->cpuset = bitmap_zalloc(c2c.cpus_cnt);
>     ...
>     c2c_he->nodeset = bitmap_zalloc(c2c.nodes_cnt);
>     ...
>     c2c_he->node_stats = calloc(c2c.nodes_cnt, sizeof(*c2c_he->node_stats));
> 
> Any external module (like the new function view) that uses this allocator
> won't have an encapsulated way to clean these up, forcing manual duplication
> of the teardown logic to avoid leaks.
> 
> [ ... ]
> > @@ -1928,7 +1816,7 @@ static struct c2c_dimension *dimensions[] = {
> >  	NULL,
> >  };
> >  
> > -static void fmt_free(struct perf_hpp_fmt *fmt)
> > +void fmt_free(struct perf_hpp_fmt *fmt)
> >  {
> >  	struct c2c_fmt *c2c_fmt;
> >  
> > @@ -1936,7 +1824,7 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> >  	free(c2c_fmt);
> >  }
> >  
> > -static bool fmt_equal(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b)
> > +bool fmt_equal(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b)
> 
> [Severity: Medium]
> Could the removal of the static keyword here lead to symbol collisions?
> 
> Since fmt_free() and fmt_equal() lack a subsystem prefix (like c2c_ or
> perf_c2c_), exporting these generic names to the global namespace increases
> the risk of collisions during static linking or Link-Time Optimization with
> other perf tool objects.

I think this is a meaningful.  Even without collisions, it'd be better
to have a prefix for global functions.

Thanks,
Namhyung

> 
> > diff --git a/tools/perf/c2c.h b/tools/perf/c2c.h
> > new file mode 100644
> > index 0000000000000..3e974dd1d7ee6
> > --- /dev/null
> > +++ b/tools/perf/c2c.h
> [ ... ]
> > +#ifdef HAVE_SLANG_SUPPORT
> > +int perf_c2c__browse_cacheline(struct hist_entry *he);
> > +#endif
> 
> [Severity: Medium]
> Should this header provide a fallback stub for perf_c2c__browse_cacheline()
> when HAVE_SLANG_SUPPORT is not defined?
> 
> The perf subsystem build guidelines require header files to provide
> compatible dummy inline stubs (e.g., returning -ENOTSUPP) when feature
> defines are absent. Omitting the #else block here might lead to missing
> functionality or build breakages on systems lacking SLANG.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260626070355.1556721-1-jiebin.sun@intel.com?part=1