Re: [PATCH 4/9] tools/perf: Add powerpc callback support for arch_record__collect_final_data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Direct access to `evsel->name` without NULL checks causes a segmentation fault.
- [High] Premature loop termination in `arch_perf_record__need_read` due to artificial overwriting of loop variables.
- [High] Unconditional compilation of PowerPC-specific execution logic globally overrides a cross-platform weak symbol.
- [High] The negative error return value from `perf_evsel__read()` is ignored, leading to premature termination of trace data collection.
- [Medium] The functions and callbacks described in the commit message are completely missing from the implemented code.
- [Low] Malformed include path directive `../../../util/record.h` causes a build failure.
--

commit dee0b2f32508f51e2b57675240fda0c4192160d5
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add powerpc callback support for arch_record__collect_final_data

This patch adds an architecture-specific hook to collect additional HTM trace
data before closing events. It reads the HTM event counts to determine if
more data is available to be written to the perf data file.

>     Add arch_record__collect_final_data to collect additional
>     data before closing the event. Define the callback in
>     util/powerpc-htm.c
[ ... ]
>     the event using evlist__enable after reading event using
>     htm_read_data function. The perf_evsel__read returns zero, when

[Severity: Medium]
The commit message mentions adding arch_record__collect_final_data and
reading events using the htm_read_data function. However, the implemented
code defines arch_perf_record__need_read and uses perf_evsel__read instead.
Could the commit message be updated to match the actual implementation?

> diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> index 5e2265018826f..4d79662e88266 100644
> --- a/tools/perf/util/Build
> +++ b/tools/perf/util/Build
> @@ -142,6 +142,7 @@ perf-util-y += hisi-ptt.o
>  perf-util-y += hisi-ptt-decoder/
>  perf-util-y += s390-cpumsf.o
>  perf-util-y += powerpc-vpadtl.o
> +perf-util-y += powerpc-htm.o

[Severity: High]
Does unconditionally adding powerpc-htm.o here cause it to be compiled for
all architectures? Since it defines arch_perf_record__need_read globally,
this could override the weak symbol in builtin-record.c and incorrectly
execute PowerPC-specific logic on other architectures like x86 or ARM.

> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> new file mode 100644
> index 0000000000000..5043ff41a609e
> --- /dev/null
> +++ b/tools/perf/util/powerpc-htm.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HTM support
> + */
> +
> +#include "../../../util/record.h"

[Severity: Low]
Will this include path resolve correctly during the build? It appears to
point outside the normal include directories for the perf tool, which might
result in a compile-time failure.

[ ... ]
> +int arch_perf_record__need_read(struct evlist *evlist)
> +{
> +	struct evsel *evsel;
> +	int found_htm = 0;
> +
> +	/* there was an error during record__open */
> +	if (!evlist)
> +		return 0;
> +
> +	/* First, check if any HTM events exist */
> +	evlist__for_each_entry(evlist, evsel) {
> +		if (strstr(evsel->name, "htm") != NULL)

[Severity: High]
Can evsel->name be NULL here? For raw hardware events or events before
parsing has dynamically assigned a name, passing a NULL pointer to strstr
or strcmp will cause a segmentation fault.

[ ... ]
> +		for (int x = 0; x < xyarray__max_x(xy); x++) {
> +			for (int y = 0; y < xyarray__max_y(xy); y++) {
> +				struct perf_counts_values count;
> +
> +				if (!strcmp(evsel->name, "dummy:u"))
> +					continue;
> +
> +				if (strstr(evsel->name, "htm")) {
> +					perf_evsel__read(&evsel->core, x, y, &count);

[Severity: High]
Does this properly handle failures from perf_evsel__read()? It returns a
negative error code on failure and leaves the count struct uninitialized or
zeroed. If ignored, count.val could evaluate to 0 below, causing the trace
data collection to terminate prematurely.

> +					y = xyarray__max_y(xy);
> +					x = xyarray__max_x(xy);

[Severity: High]
Are these assignments intentionally overwriting the loop iteration variables?
This forcefully terminates both the inner and outer loops after checking
only the very first CPU and thread index (0, 0), preventing the iteration
from checking any of the remaining CPUs.

> +				}
> +				if (!count.val)
> +					return 0;
> +			}
> +		}
> +	}
> +
> +	return 1;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701084115.80383-1-atrajeev@linux.ibm.com?part=4