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

[sashiko-bot@kernel.org]

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.

> 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