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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing feature fallback stub for `perf_c2c__browse_cacheline`
- [Medium] Namespace pollution and type safety hazard with generically named global functions
--

--- Patch [1]: [PATCH v2 01/14] perf c2c: extract shared data structures into c2c.h ---
commit 2da5df0fe4f2a39ca1036332be2996007bdfe83a
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: extract shared data structures into c2c.h

Move 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 (c2c-function.c) to reuse these
types. Make the global perf_c2c instance 'c2c' non-static and export
perf_c2c__browse_cacheline() so they can be accessed from the new function
view module.

> 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
[ ... ]
> @@ -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);
>  }

[Severity: Medium]
Since this function unconditionally casts perf_hpp_fmt to c2c_fmt, could
exporting it globally with such a generic name risk another subsystem
mistakenly calling it and causing heap corruption during the free() call?

Would it be safer to prefix it with c2c_?

>  
> -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]
Similarly, could exporting this generically named function without a
c2c_ prefix risk namespace collisions or type safety issues if misused
by other subsystems?

[ ... ]
> diff --git a/tools/perf/c2c.h b/tools/perf/c2c.h
> new file mode 100644
> index 0000000000000..5c3f6ae633ea8
> --- /dev/null
> +++ b/tools/perf/c2c.h
[ ... ]
> +void *c2c_he_zalloc(size_t size);
> +void fmt_free(struct perf_hpp_fmt *fmt);
> +bool fmt_equal(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b);
> +
> +#ifdef HAVE_SLANG_SUPPORT
> +int perf_c2c__browse_cacheline(struct hist_entry *he);
> +#endif

[Severity: Medium]
Does this declaration need an #else fallback stub (e.g. returning -ENOTSUPP)
for systems built without SLang support?

If upcoming consumers like c2c-function.c call perf_c2c__browse_cacheline()
unconditionally, missing the dummy inline stub might cause compilation
failures when the feature define is absent.

> +
> +#endif /* _PERF_C2C_H_ */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710084247.3576706-1-jiebin.sun@intel.com?part=1