[PATCH] perf trace: Add --bitmask-list option and correct cpumask formatting

[PATCH] perf trace: Add --bitmask-list option and correct cpumask formatting

[Aaron Tomlin]

Currently, dynamic non-array fields such as 'cpumask_t' are mishandled
in 'perf trace', causing the raw length and offset descriptors to be
interpreted and displayed as a literal integer (e.g., 'cpumask: 524320'
instead of the actual mask data).

This patch corrects the parsing of dynamic fields that do not have the
TEP_FIELD_IS_ARRAY flag set. By doing so, it ensures the pointer to the
raw bits is properly resolved within the payload.

Furthermore, it improves the display formatting for 'cpumask' types:
    - By default, the cpumask is now correctly output as a zero-suppressed
      hexadecimal string, resolving the aforementioned integer display
      anomaly

    - A new '--bitmask-list' command-line option has been introduced.
      When specified, this delegates formatting to 'bitmap_scnprintf()',
      enabling the cpumask to be rendered as a condensed, human-readable
      list (e.g., '0,2-5,7')

Fixes: c5e006cdbd27 ("perf trace: Support tracepoint dynamic char arrays")
Signed-off-by: Aaron Tomlin <atomlin@atomlin.com>
---
 tools/perf/builtin-trace.c | 70 ++++++++++++++++++++++++++++++++------
 tools/perf/util/evsel.c    | 29 ++++++++++++++++
 tools/perf/util/evsel.h    |  3 ++
 3 files changed, 92 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index ba0f8749fc7d..cd75ac4e5169 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -226,6 +226,7 @@ struct trace {
 	bool			force;
 	bool			vfs_getname;
 	bool			force_btf;
+	bool			bitmask_list;
 	bool			summary_bpf;
 	int			trace_pgfaults;
 	char			*perfconfig_events;
@@ -3216,7 +3217,7 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct perf_sample *
 	const struct tep_event *tp_format = evsel__tp_format(evsel);
 	struct tep_format_field *field = tp_format ? tp_format->format.fields : NULL;
 	struct syscall_arg_fmt *arg = __evsel__syscall_arg_fmt(evsel);
-	size_t printed = 0, btf_printed;
+	size_t printed = 0, btf_printed, saved_printed;
 	unsigned long val;
 	u8 bit = 1;
 	struct syscall_arg syscall_arg = {
@@ -3238,17 +3239,65 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct perf_sample *
 		syscall_arg.len = 0;
 		syscall_arg.fmt = arg;
 		if (field->flags & TEP_FIELD_IS_ARRAY) {
-			int offset = field->offset;
-
-			if (field->flags & TEP_FIELD_IS_DYNAMIC) {
-				offset = format_field__intval(field, sample, evsel->needs_swap);
-				syscall_arg.len = offset >> 16;
-				offset &= 0xffff;
-				if (tep_field_is_relative(field->flags))
-					offset += field->offset + field->size;
+			void *ptr = format_field__get_raw_data(field, sample,
+							       evsel->needs_swap,
+							       &syscall_arg.len);
+
+			if (!ptr) {
+				pr_err("Problem processing %s field, skipping...\n", field->name);
+				continue;
+			}
+			val = (uintptr_t)ptr;
+		} else if ((field->flags & TEP_FIELD_IS_DYNAMIC) &&
+			   strstr(field->type, "cpumask")) {
+			void *ptr = format_field__get_raw_data(field, sample,
+							       evsel->needs_swap,
+							       &syscall_arg.len);
+
+			if (!ptr) {
+				pr_err("Problem processing %s field, skipping...\n", field->name);
+				continue;
 			}
+			val = (uintptr_t)ptr;
 
-			val = (uintptr_t)(sample->raw_data + offset);
+			saved_printed = printed;
+
+			printed += scnprintf(bf + printed, size - printed, "%s", printed ? ", " : "");
+			if (trace->show_arg_names)
+				printed += scnprintf(bf + printed, size - printed, "%s: ", field->name);
+
+			if (trace->bitmask_list) {
+				if (syscall_arg.len > 0) {
+					unsigned long *mask = zalloc(BITS_TO_LONGS(syscall_arg.len * 8) * sizeof(unsigned long));
+					if (!mask) {
+						pr_err("Problem processing %s field, skipping...\n", field->name);
+						printed = saved_printed;
+						continue;
+					}
+					memcpy(mask, (void *)val, syscall_arg.len);
+					printed += bitmap_scnprintf(mask, syscall_arg.len * 8,
+								    bf + printed, size - printed);
+					free(mask);
+				}
+			} else {
+				unsigned char *b = (unsigned char *)val;
+				int i;
+				bool skip_zero = true;
+
+				printed += scnprintf(bf + printed, size - printed, "0x");
+				/* Print in little-endian order */
+				for (i = syscall_arg.len - 1; i >= 0; i--) {
+					if (skip_zero && b[i] == 0 && i > 0)
+						continue;
+					if (skip_zero) {
+						printed += scnprintf(bf + printed, size - printed, "%x", b[i]);
+						skip_zero = false;
+					} else {
+						printed += scnprintf(bf + printed, size - printed, "%02x", b[i]);
+					}
+				}
+			}
+			continue;
 		} else
 			val = format_field__intval(field, sample, evsel->needs_swap);
 		/*
@@ -5537,6 +5586,7 @@ int cmd_trace(int argc, const char **argv)
 		     "start"),
 	OPT_BOOLEAN(0, "force-btf", &trace.force_btf, "Prefer btf_dump general pretty printer"
 		       "to customized ones"),
+	OPT_BOOLEAN(0, "bitmask-list", &trace.bitmask_list, "Show bitmask as a human-readable list"),
 	OPT_BOOLEAN(0, "bpf-summary", &trace.summary_bpf, "Summary syscall stats in BPF"),
 	OPT_INTEGER(0, "max-summary", &trace.max_summary,
 		     "Max number of entries in the summary."),
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ea9fa04429f0..0d5eb2cc4ce5 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3933,6 +3933,35 @@ void *perf_sample__rawptr(struct perf_sample *sample, const char *name)
 	return sample->raw_data + offset;
 }
 
+void *format_field__get_raw_data(struct tep_format_field *field, struct
+				 perf_sample *sample, bool needs_swap,
+				 u16 *len_out)
+{
+	int offset = field->offset;
+	int size = field->size;
+
+	if (field->flags & TEP_FIELD_IS_DYNAMIC) {
+		unsigned int dynamic_data;
+
+		if (out_of_bounds(field, field->offset, 4, sample->raw_size))
+			return NULL;
+
+		dynamic_data = format_field__intval(field, sample, needs_swap);
+
+		offset = dynamic_data & 0xffff;
+		size = (dynamic_data >> 16) & 0xffff;
+
+		if (tep_field_is_relative(field->flags))
+			offset += field->offset + field->size;
+	}
+
+	if (out_of_bounds(field, offset, size, sample->raw_size))
+		return NULL;
+
+	*len_out = size;
+	return sample->raw_data + offset;
+}
+
 u64 format_field__intval(struct tep_format_field *field, struct perf_sample *sample,
 			 bool needs_swap)
 {
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 163fc2b6a7ea..1babfecc5d13 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -400,6 +400,9 @@ static inline char *perf_sample__strval(struct perf_sample *sample, const char *
 
 struct tep_format_field;
 
+void *format_field__get_raw_data(struct tep_format_field *field,
+				 struct perf_sample *sample,
+				 bool needs_swap, u16 *len_out);
 u64 format_field__intval(struct tep_format_field *field, struct perf_sample *sample, bool needs_swap);
 
 #ifdef HAVE_LIBTRACEEVENT
-- 
2.54.0

Re: [PATCH] perf trace: Add --bitmask-list option and correct cpumask formatting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The cpumask byte parsing and bitmask rendering ignores the endianness and word size of the trace payload, breaking cross-platform analysis.
--

--- Patch [1]: [PATCH] perf trace: Add --bitmask-list option and correct cpumask formatting ---
commit 3ae8add0cf042fd56dee83eecdf3dd3e5eeef32c
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace: Add --bitmask-list option and correct cpumask formatting

This commit corrects the parsing of dynamic non-array fields like cpumask_t
in perf trace to resolve raw pointers properly. It also updates the display
formatting for cpumasks to show zero-suppressed hex strings by default and
adds a --bitmask-list option for human-readable bitmask formatting.

Fixes: c5e006cdbd27 ("perf trace: Support tracepoint dynamic char arrays")

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index ba0f8749fc7d7..cd75ac4e51693 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3238,17 +3239,65 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct perf_sample *

[ ... ]

> +			if (trace->bitmask_list) {
> +				if (syscall_arg.len > 0) {
> +					unsigned long *mask = zalloc(BITS_TO_LONGS(syscall_arg.len * 8) * sizeof(unsigned long));
> +					if (!mask) {
> +						pr_err("Problem processing %s field, skipping...\n", field->name);
> +						printed = saved_printed;
> +						continue;
> +					}
> +					memcpy(mask, (void *)val, syscall_arg.len);
> +					printed += bitmap_scnprintf(mask, syscall_arg.len * 8,
> +								    bf + printed, size - printed);

[Severity: Medium]
Will this memcpy() and bitmask formatting break cross-platform analysis if the
perf.data file was recorded on an architecture with a different endianness or
word size than the host?

It appears we are doing a direct copy of the target payload bytes into a
host-native unsigned long array without utilizing evsel->needs_swap.

Could this result in garbled or reversed cpumask values when viewing trace
data from a machine with different architectural characteristics?

> +					free(mask);
> +				}
> +			} else {
> +				unsigned char *b = (unsigned char *)val;
> +				int i;
> +				bool skip_zero = true;
> +
> +				printed += scnprintf(bf + printed, size - printed, "0x");
> +				/* Print in little-endian order */
> +				for (i = syscall_arg.len - 1; i >= 0; i--) {

[Severity: Medium]
Does this backward loop assume the trace data is unconditionally little-endian?

If the trace data comes from a big-endian architecture, will this hardcoded
assumption fail to print the values correctly?

> +					if (skip_zero && b[i] == 0 && i > 0)
> +						continue;
> +					if (skip_zero) {
> +						printed += scnprintf(bf + printed, size - printed, "%x", b[i]);
> +						skip_zero = false;
> +					} else {
> +						printed += scnprintf(bf + printed, size - printed, "%02x", b[i]);
> +					}
> +				}
> +			}
> +			continue;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714162947.214270-1-atomlin@atomlin.com?part=1