Re: [RFC PATCH v3 4/5] perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Memory leak of page_records and page_rec_sizes arrays when extend allocation fails.
- [Medium] The --to-trace-dat conversion silently ignores the --time filtering option.
- [Low] Missing explicit POSIX header inclusions in trace-dat.h and data-convert-trace.c causes build failures on musl libc.
- [Low] Missing dummy inline stub for trace_convert__perf2dat when HAVE_LIBTRACEEVENT is absent.
--

--- Patch [4]: [RFC PATCH v3 4/5] perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format ---
commit 3f0a7b06f263871cd1c6bbd15fd1c2bd5d454bae
Author: Tanushree Shah <tshah@linux.ibm.com>
perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format

This commit adds a new command-line option to perf data convert for generating
trace.dat output files. It provides a trace_convert__perf2dat function to
handle tracepoint events and write them in a format compatible with trace-cmd
and KernelShark.

> diff --git a/tools/perf/util/data-convert-trace.c b/tools/perf/util/data-convert-trace.c
> new file mode 100644
> index 0000000000000..8dbdb2c9caa4f
> --- /dev/null
> +++ b/tools/perf/util/data-convert-trace.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2026, IBM Corporation
> + * Author: Tanushree Shah <tshah@linux.ibm.com>
> + *
> + * data-convert-trace.c
> + *
> + * Implements perf.data to trace.dat format conversion for tracepoint events.
> + */
> +
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <linux/compiler.h>
> +#include <linux/err.h>
> +
> +#include "data-convert.h"

[ ... ]

> +/* Convert perf.data tracepoint events to trace.dat format */
> +int trace_convert__perf2dat(const char *input, const char *to_trace,
> +			   struct perf_data_convert_opts *opts)
> +{
> +	struct perf_session *session;

[Severity: Medium]
Does this function ignore the opts->time_str filtering option? In other format
conversions, the time parameter is parsed to filter samples within the specified
time ranges, but it appears to be unused here when converting to trace.dat.

> +	struct trace_convert tc = {
> +		.events_count = 0,
> +	};

[ ... ]

> +	if (!opts->force) {
> +		int fd = open(to_trace, O_WRONLY | O_CREAT | O_EXCL, 0644);
> +
> +		if (fd < 0) {
> +			if (errno == EEXIST)
> +				pr_err("Output file '%s' already exists. Use --force to overwrite.\n",
> +				       to_trace);
> +			else
> +				pr_err("Failed to open output file '%s': %s\n",
> +				       to_trace, strerror(errno));
> +			return -errno;
> +		}
> +		trace_dat_fp = fdopen(fd, "wb");

[Severity: Low]
Should <stdio.h> be explicitly included at the top of this file? Standard I/O
functions like fdopen() and fopen() are used here, which could cause build
failures on musl libc without the direct header inclusion.

> diff --git a/tools/perf/util/data-convert.h b/tools/perf/util/data-convert.h
> index a96240f156712..f041c23252263 100644
> --- a/tools/perf/util/data-convert.h
> +++ b/tools/perf/util/data-convert.h
> @@ -19,4 +19,8 @@ int bt_convert__perf2ctf(const char *input_name, const char *to_ctf,
>  int bt_convert__perf2json(const char *input_name, const char *to_ctf,
>  			 struct perf_data_convert_opts *opts);
>  
> +#ifdef HAVE_LIBTRACEEVENT
> +int trace_convert__perf2dat(const char *input, const char *to_trace,
> +			   struct perf_data_convert_opts *opts);
> +#endif /* HAVE_LIBTRACEEVENT */

[Severity: Low]
Is a dummy inline stub needed here for when HAVE_LIBTRACEEVENT is disabled?
Without an #else block providing a fallback stub, it could cause future build
failures if new callers are added that aren't guarded by the feature macro.

> diff --git a/tools/perf/util/trace-dat.c b/tools/perf/util/trace-dat.c
> new file mode 100644
> index 0000000000000..5598496c9a61c
> --- /dev/null
> +++ b/tools/perf/util/trace-dat.c

[ ... ]

> +static int trace_dat__write_cpu_dat(FILE *fp, struct tep_handle *pevent,
> +		int cpu, unsigned long long *file_offset_out)
> +{
> +	struct cpu_events *cpu_events = &trace_cpu_data[cpu];

[ ... ]

> +	for (i = 0; i < cpu_events->count; i++) {
> +		struct cpu_event *event = &cpu_events->events[i];

[ ... ]

> +		/* Emit TIME_EXTEND when delta does not fit in 27 bits */
> +		if (time_delta > TRACE_DAT_RECORD_TIME_MASK) {
> +			unsigned int extend_hdr;
> +			unsigned int delta_upper;
> +
> +			extend_size = TRACE_DAT_RECORD_TIME_EXTEND_SIZE;
> +			extend = calloc(1, extend_size);
> +			if (!extend)
> +				return -ENOMEM;

[Severity: Medium]
Can this early return cause a memory leak? If the allocation for extend fails,
returning -ENOMEM directly skips the out_free label at the end of the function,
which is responsible for freeing the dynamically resized page_records and
page_rec_sizes arrays.

> diff --git a/tools/perf/util/trace-dat.h b/tools/perf/util/trace-dat.h
> new file mode 100644
> index 0000000000000..b63d4e8c1e2d7
> --- /dev/null
> +++ b/tools/perf/util/trace-dat.h
> @@ -0,0 +1,113 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

[ ... ]

> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <event-parse.h>
> +#include <byteswap.h>
> +#include "util.h"
> +
> +/* trace.dat file format version */
> +#define TRACE_DAT_VERSION "7"

[ ... ]

> +static inline uint16_t to_file_u16(struct tep_handle *pevent, uint16_t val)
> +{
> +	return tep_read_number(pevent, &val, 2);
> +}

[Severity: Low]
Does this file need to explicitly include <stdint.h> to ensure compatibility
when compiling with musl libc? Fixed-width integer types like uint16_t are used
here but the standard POSIX header for them is missing.

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