Re: [PATCH V3 5/6] tools/perf: Add powerpc HTM auxtrace event processing support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The file `powerpc-htm.c` calls `zalloc()` but fails to include `<linux/zalloc.h>`, resulting in an implicit function declaration.
- [Low] The file `powerpc-htm.c` embeds `struct auxtrace_queues` and calls its API but fails to directly include `"auxtrace.h"`, relying on brittle implicit inclusion.
--

--- Patch [5]: [PATCH V3 5/6] tools/perf: Add powerpc HTM auxtrace event processing support ---
commit 306ea78646ef6e5fe95dbad9977aff1241d1027a
Author: Athira Rajeev <atrajeev@linux.ibm.com>

tools/perf: Add powerpc HTM auxtrace event processing support

This commit adds the powerpc-htm.c decode stub for powerpc HTM auxtrace.
It wires the dispatch in perf_event__process_auxtrace_info() to call
powerpc_htm_process_auxtrace_info().

> diff --git a/tools/perf/util/powerpc-htm.c b/tools/perf/util/powerpc-htm.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/perf/util/powerpc-htm.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <inttypes.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <endian.h>
> +#include "util/evsel.h"
> +#include "util/evlist.h"
> +#include "util/session.h"
> +#include "color.h"
> +#include "powerpc-htm.h"
> +#include "debug.h"
> +#include "sample.h"
> +
> +struct powerpc_htm {
> +	struct auxtrace		auxtrace;
> +	struct auxtrace_queues	queues;

[Severity: Low]
Should this file explicitly include "auxtrace.h"?

The code embeds struct auxtrace_queues here, and later calls
auxtrace_queues__init(), both of which are defined in "auxtrace.h". Relying
on implicit inclusion (for example, through "util/evlist.h") currently works
but could break the build if intermediate headers are refactored in the
future.

[ ... ]

> +int powerpc_htm_process_auxtrace_info(union perf_event *event,
> +				      struct perf_session *session)
> +{
> +	struct perf_record_auxtrace_info *auxtrace_info = &event->auxtrace_info;
> +	struct powerpc_htm *htm;
> +	int err;
> +
> +	if (auxtrace_info->header.size < sizeof(struct perf_record_auxtrace_info) +
> +					 HTM_AUXTRACE_PRIV_FIXED)
> +		return -EINVAL;
> +
> +	htm = zalloc(sizeof(struct powerpc_htm));

[Severity: High]
Does this implicit function declaration cause a build failure or pointer
truncation?

The function zalloc() is used here, but <linux/zalloc.h> is not included in
this file. Because the kernel build system uses -Werror, this missing header
could trigger a build failure.

If -Werror were disabled, the compiler would assume zalloc() returns a 32-bit
integer. On 64-bit architectures like PowerPC, this truncates the 64-bit
pointer. When extended back to 64 bits during assignment, the higher bits are
lost, which could result in a segmentation fault upon dereference.

Should <linux/zalloc.h> be included to ensure the return type is properly
defined?

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