Re: [PATCH v2 08/14] perf c2c: add HPP list parsing for function view histograms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Calling `reset_dimensions()` for every fallback token in `c2c_function_hists__init_output` and `c2c_function_hists__init_sort` clears the global `taken` flags during string parsing, defeating duplicate field protection.
- [Medium] `function_hpp_list__parse` leaks dynamically allocated `c2c_fmt` elements if token parsing fails midway.
- [Low] Missing newline character in error log in `function_hpp_list__add_tokens`.
--

--- Patch [8]: [PATCH v2 08/14] perf c2c: add HPP list parsing for function view histograms ---
commit 7b3a4ebd5658b8b4b899fd6fb0008adeb8109767
Author: Jiebin Sun <jiebin.sun@intel.com>

perf c2c: add HPP list parsing for function view histograms

This commit adds the histogram initialization and column parsing
infrastructure to bridge the function view's custom dimensions with the perf
HPP list infrastructure. This enables dynamic column configuration during
hierarchy construction.

> diff --git a/tools/perf/ui/browsers/c2c-function.c b/tools/perf/ui/browsers/c2c-function.c
> index 7c2bb8f9e1139..0f21cf39f0ce6 100644
> --- a/tools/perf/ui/browsers/c2c-function.c
> +++ b/tools/perf/ui/browsers/c2c-function.c

[ ... ]

> @@ -502,7 +513,156 @@ static __maybe_unused struct c2c_fmt *get_function_format(const char *name)

[ ... ]

> +static int
> +c2c_function_hists__init_output(struct perf_hpp_list *hpp_list, char *name,
> +				struct perf_env *env __maybe_unused)
> +{
> +	struct c2c_fmt *c2c_fmt;
> +	int level = 0;
> +	int ret;
> +
> +	ret = get_function_format(name, &c2c_fmt);
> +	if (ret == -ENOMEM)
> +		return ret;
> +	if (ret == -ENOENT) {
> +		reset_dimensions();

[Severity: Medium]
Does calling reset_dimensions() here wipe out the global taken flag for all
previously processed tokens? If a user provides a string with multiple
fallback tokens, it seems this clears the flags and defeats the duplicate
field protection, which might allow duplicate columns and lead to
double-frees or list corruption during teardown.

> +		return output_field_add(hpp_list, name, &level);
> +	}

[ ... ]

> +static int
> +c2c_function_hists__init_sort(struct perf_hpp_list *hpp_list, char *name,
> +			      struct perf_env *env)
> +{
> +	struct c2c_fmt *c2c_fmt;
> +	int ret;
> +
> +	ret = get_function_format(name, &c2c_fmt);
> +	if (ret == -ENOMEM)
> +		return ret;
> +	if (ret == -ENOENT) {
> +		reset_dimensions();

[Severity: Medium]
Does this repeated call to reset_dimensions() for sort tokens also cause the
same state corruption as in the output field initialization?

> +		return sort_dimension__add(hpp_list, name, /*evlist=*/NULL, env, /*level=*/0);
> +	}

[ ... ]

> +static int function_hpp_list__add_tokens(struct perf_hpp_list *hpp_list, char *list,
> +					 struct perf_env *env, hpp_list_add_fn add)
> +{
> +	char *tok, *tmp;
> +	int ret;
> +
> +	if (!list)
> +		return 0;
> +
> +	for (tok = strtok_r(list, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
> +		ret = add(hpp_list, tok, env);
> +		if (ret) {
> +			if (ret == -EINVAL || ret == -ESRCH)
> +				pr_err("Invalid c2c function-view field: %s", tok);

[Severity: Low]
This isn't a bug, but does this pr_err() need a trailing '\n' to prevent
garbled terminal output? The pr_err() macro in tools/perf wraps eprintf
and does not implicitly append a newline.

> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int
> +function_hpp_list__parse(struct perf_hpp_list *hpp_list,
> +			 const char *output_str,
> +			 const char *sort_str,
> +			 struct perf_env *env)
> +{
> +	char *output = output_str ? strdup(output_str) : NULL;
> +	char *sort   = sort_str   ? strdup(sort_str)   : NULL;
> +	int ret = 0;
> +
> +	if ((output_str && !output) || (sort_str && !sort)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = function_hpp_list__add_tokens(hpp_list, output, env,
> +					    c2c_function_hists__init_output);
> +	if (ret)
> +		goto out;

[Severity: Medium]
If function_hpp_list__add_tokens() fails midway and returns an error, does
this goto out leak the dynamically allocated c2c_fmt elements? It appears
the previously allocated and registered elements in hpp_list are not freed
when bypassing the normal cleanup path.

> +
> +	ret = function_hpp_list__add_tokens(hpp_list, sort, env,
> +					    c2c_function_hists__init_sort);
> +	if (ret)
> +		goto out;

[Severity: Medium]
Does returning early here if the sort string parsing fails also leak the
allocations already made during the output string parsing?

> +
> +	perf_hpp__setup_output_field(hpp_list);
> +out:
> +	free(output);
> +	free(sort);
> +	return ret;
> +}

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