Re: [PATCH 07/14] perf c2c: add dimension definitions and format creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The c2c_se_cmp wrapper improperly forces fmt->sort to use dim->se->se_cmp instead of dim->se->se_sort.
--

commit 160d3ea9bf317fa78359c422d21e9cf0a3470f5a
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: add dimension definitions and format creation

This patch adds function view dimension definitions that bind column names
to their rendering and comparison functions. It also introduces lookup and
format creation helpers for the function view.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index 172e1aa86ecab..794a023239ce1 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c

[ ... ]

> @@ -390,6 +390,125 @@ total_stores_cmp(struct perf_hpp_fmt *fmt __maybe_unused,

[ ... ]

> +/* Wrappers so sort_entry-backed dimensions sort/collapse via their se. */
> +static int64_t c2c_se_cmp(struct perf_hpp_fmt *fmt,
> +			  struct hist_entry *a, struct hist_entry *b)
> +{
> +	struct c2c_fmt *c2c_fmt = container_of(fmt, struct c2c_fmt, fmt);
> +	struct c2c_dimension *dim = c2c_fmt->dim;
> +
> +	return dim->se->se_cmp(a, b);
> +}
> +
> +static int64_t c2c_se_collapse(struct perf_hpp_fmt *fmt,
> +			       struct hist_entry *a, struct hist_entry *b)
> +{
> +	struct c2c_fmt *c2c_fmt = container_of(fmt, struct c2c_fmt, fmt);
> +	struct c2c_dimension *dim = c2c_fmt->dim;
> +	int64_t (*collapse_fn)(struct hist_entry *a, struct hist_entry *b);
> +
> +	collapse_fn = dim->se->se_collapse ?: dim->se->se_cmp;
> +	return collapse_fn(a, b);
> +}
> +
> +static __maybe_unused struct c2c_fmt *get_function_format(const char *name)
> +{
> +	struct c2c_dimension *dim = get_function_dimension(name);
> +	struct c2c_fmt *c2c_fmt;
> +	struct perf_hpp_fmt *fmt;
> +
> +	if (!dim)
> +		return NULL;
> +
> +	c2c_fmt = zalloc(sizeof(*c2c_fmt));
> +	if (!c2c_fmt)
> +		return NULL;
> +
> +	fmt = &c2c_fmt->fmt;
> +
> +	c2c_fmt->dim = dim;
> +	INIT_LIST_HEAD(&fmt->list);
> +	INIT_LIST_HEAD(&fmt->sort_list);
> +
> +	fmt->cmp	= dim->se ? c2c_se_cmp : dim->cmp;
> +	fmt->sort	= dim->se ? c2c_se_cmp : dim->cmp;

[Severity: Medium]
Does this assignment incorrectly bypass the sort_entry's se_sort callback?

Because c2c_se_cmp strictly calls dim->se->se_cmp:

static int64_t c2c_se_cmp(struct perf_hpp_fmt *fmt,
                          struct hist_entry *a, struct hist_entry *b)
{
        ...
        return dim->se->se_cmp(a, b);
}

Using c2c_se_cmp for fmt->sort means we will use se_cmp instead of se_sort.
For example, sorting by the symbol column might sort by memory address offset
rather than alphabetically by symbol name.

Should there be a separate c2c_se_sort wrapper that delegates to
dim->se->se_sort, or similar logic?

> +	fmt->color	= dim->color;
> +	fmt->entry	= dim->entry;
> +	fmt->header	= c2c_header;

[ ... ]

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