The clang warning -Wshorten-64-to-32 can be useful to catch
inadvertent truncation. In some instances this truncation can lead to
changing the sign of a result, for example, truncation to return an
int to fit a sort routine. Silence the warning by making the implicit
truncation explicit.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/api-io.c                   |  2 +-
 tools/perf/tests/bp_signal.c                |  6 +++---
 tools/perf/tests/bp_signal_overflow.c       |  6 +++---
 tools/perf/tests/builtin-test.c             |  9 +++++----
 tools/perf/tests/code-reading.c             |  4 ++--
 tools/perf/tests/dso-data.c                 | 10 +++++-----
 tools/perf/tests/mmap-thread-lookup.c       |  2 +-
 tools/perf/tests/openat-syscall-tp-fields.c |  2 +-
 tools/perf/tests/pmu-events.c               |  2 +-
 tools/perf/tests/sigtrap.c                  |  4 ++--
 tools/perf/tests/switch-tracking.c          | 11 ++++++-----
 tools/perf/tests/vmlinux-kallsyms.c         |  4 ++--
 tools/perf/tests/wp.c                       |  4 ++--
 13 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/tools/perf/tests/api-io.c b/tools/perf/tests/api-io.c
index 0ba3d5ccebcf..4aee1cff062e 100644
--- a/tools/perf/tests/api-io.c
+++ b/tools/perf/tests/api-io.c
@@ -74,7 +74,7 @@ static int setup_test(char path[PATH_MAX], const char *contents,
 		unlink(path);
 		return -1;
 	}
-	io__init(io, io->fd, io->buf, buf_size);
+	io__init(io, io->fd, io->buf, (unsigned int)buf_size);
 	return 0;
 }
 
diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c
index 3faeb5b6fe0b..5ecac6c2d8b6 100644
--- a/tools/perf/tests/bp_signal.c
+++ b/tools/perf/tests/bp_signal.c
@@ -64,7 +64,7 @@ static noinline int test_function(void)
 {
 	__test_function(&the_var);
 	the_var++;
-	return time(NULL);
+	return (int)time(NULL);
 }
 
 static void sig_handler_2(int signum __maybe_unused,
@@ -151,11 +151,11 @@ static int wp_event(void *addr, int sig)
 static long long bp_count(int fd)
 {
 	long long count;
-	int ret;
+	ssize_t ret;
 
 	ret = read(fd, &count, sizeof(long long));
 	if (ret != sizeof(long long)) {
-		pr_debug("failed to read: %d\n", ret);
+		pr_debug("failed to read: %zd\n", ret);
 		return TEST_FAIL;
 	}
 
diff --git a/tools/perf/tests/bp_signal_overflow.c b/tools/perf/tests/bp_signal_overflow.c
index ee560e156be6..eb9021ad8177 100644
--- a/tools/perf/tests/bp_signal_overflow.c
+++ b/tools/perf/tests/bp_signal_overflow.c
@@ -33,7 +33,7 @@ static int overflows;
 
 static noinline int test_function(void)
 {
-	return time(NULL);
+	return (int)time(NULL);
 }
 
 static void sig_handler(int signum __maybe_unused,
@@ -46,11 +46,11 @@ static void sig_handler(int signum __maybe_unused,
 static long long bp_count(int fd)
 {
 	long long count;
-	int ret;
+	ssize_t ret;
 
 	ret = read(fd, &count, sizeof(long long));
 	if (ret != sizeof(long long)) {
-		pr_debug("failed to read: %d\n", ret);
+		pr_debug("failed to read: %zd\n", ret);
 		return TEST_FAIL;
 	}
 
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 14d30a5053be..4ea840580876 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -301,7 +301,7 @@ static int print_test_result(struct test_suite *t, int curr_suite, int curr_test
 	return 0;
 }
 
-static void finish_test(struct child_test **child_tests, int running_test, int child_test_num,
+static void finish_test(struct child_test **child_tests, size_t running_test, size_t child_test_num,
 		int width)
 {
 	struct child_test *child_test = child_tests[running_test];
@@ -349,7 +349,7 @@ static void finish_test(struct child_test **child_tests, int running_test, int c
 		if (perf_use_color_default) {
 			int running = 0;
 
-			for (int y = running_test; y < child_test_num; y++) {
+			for (size_t y = running_test; y < child_test_num; y++) {
 				if (child_tests[y] == NULL)
 					continue;
 				if (check_if_command_finished(&child_tests[y]->process) == 0)
@@ -478,13 +478,14 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
 	int err = 0;
 
 	for (struct test_suite **t = suites; *t; t++) {
-		int i, len = strlen(test_description(*t, -1));
+		int i;
+		int len = (int)strlen(test_description(*t, -1));
 
 		if (width < len)
 			width = len;
 
 		test_suite__for_each_test_case(*t, i) {
-			len = strlen(test_description(*t, i));
+			len = (int)strlen(test_description(*t, i));
 			if (width < len)
 				width = len;
 			num_tests += runs_per_test;
diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index cf6edbe697b2..0c31d5ff77e2 100644
--- a/tools/perf/tests/code-reading.c
+++ b/tools/perf/tests/code-reading.c
@@ -188,7 +188,7 @@ static int objdump_version(void)
 	char *line = NULL;
 	const char *fmt;
 	FILE *f;
-	int ret;
+	ssize_t ret;
 
 	int version_tmp, version_num = 0;
 	char *version = 0, *token;
@@ -295,7 +295,7 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
 	if (len) {
 		pr_debug("objdump read too few bytes: %zd\n", len);
 		if (!ret)
-			ret = len;
+			ret = (int)len;
 	}
 
 	pclose(f);
diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index a1fff4203b75..2954e06ae5b3 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -233,14 +233,14 @@ static int dsos__create(int cnt, int size, struct dsos *dsos)
 	return 0;
 }
 
-static int set_fd_limit(int n)
+static int set_fd_limit(long n)
 {
 	struct rlimit rlim;
 
 	if (getrlimit(RLIMIT_NOFILE, &rlim))
 		return -1;
 
-	pr_debug("file limit %ld, new %d\n", (long) rlim.rlim_cur, n);
+	pr_debug("file limit %ld, new %ld\n", (long) rlim.rlim_cur, n);
 
 	rlim.rlim_cur = n;
 	return setrlimit(RLIMIT_NOFILE, &rlim);
@@ -249,8 +249,8 @@ static int set_fd_limit(int n)
 static int test__dso_data_cache(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
 {
 	struct machine machine;
-	long nr_end, nr = open_files_cnt();
-	int dso_cnt, limit, i, fd;
+	long dso_cnt, limit, nr_end, nr = open_files_cnt();
+	int i, fd;
 
 	/* Rest the internal dso open counter limit. */
 	reset_fd_limit();
@@ -264,7 +264,7 @@ static int test__dso_data_cache(struct test_suite *test __maybe_unused, int subt
 	/* and this is now our dso open FDs limit */
 	dso_cnt = limit / 2;
 	TEST_ASSERT_VAL("failed to create dsos\n",
-			!dsos__create(dso_cnt, TEST_FILE_SIZE, &machine.dsos));
+			!dsos__create((int)dso_cnt, TEST_FILE_SIZE, &machine.dsos));
 
 	for (i = 0; i < (dso_cnt - 1); i++) {
 		struct dso *dso = machine.dsos.dsos[i];
diff --git a/tools/perf/tests/mmap-thread-lookup.c b/tools/perf/tests/mmap-thread-lookup.c
index 446a3615d720..b328ad6973a3 100644
--- a/tools/perf/tests/mmap-thread-lookup.c
+++ b/tools/perf/tests/mmap-thread-lookup.c
@@ -45,7 +45,7 @@ static int thread_init(struct thread_data *td)
 	}
 
 	td->map = map;
-	td->tid = syscall(SYS_gettid);
+	td->tid = (pid_t)syscall(SYS_gettid);
 
 	pr_debug("tid = %d, map = %p\n", td->tid, map);
 	return 0;
diff --git a/tools/perf/tests/openat-syscall-tp-fields.c b/tools/perf/tests/openat-syscall-tp-fields.c
index 0ef4ba7c1571..68f1498d0a47 100644
--- a/tools/perf/tests/openat-syscall-tp-fields.c
+++ b/tools/perf/tests/openat-syscall-tp-fields.c
@@ -119,7 +119,7 @@ static int test__syscall_openat_tp_fields(struct test_suite *test __maybe_unused
 					goto out_delete_evlist;
 				}
 
-				tp_flags = evsel__intval(evsel, &sample, "flags");
+				tp_flags = (int)evsel__intval(evsel, &sample, "flags");
 				perf_sample__exit(&sample);
 				if (flags != tp_flags) {
 					pr_debug("%s: Expected flags=%#x, got %#x\n",
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index db004d26fcb0..2762794827ce 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -609,7 +609,7 @@ static int __test_uncore_pmu_event_aliases(struct perf_pmu_test_pmu *test_pmu)
 	pmu_add_sys_aliases(pmu);
 
 	/* Count how many aliases we generated */
-	alias_count = perf_pmu__num_events(pmu);
+	alias_count = (int)perf_pmu__num_events(pmu);
 
 	/* Count how many aliases we expect from the known table */
 	for (table = &test_pmu->aliases[0]; *table; table++)
diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
index a67c756f90b8..b7d7735f8a72 100644
--- a/tools/perf/tests/sigtrap.c
+++ b/tools/perf/tests/sigtrap.c
@@ -154,13 +154,13 @@ sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __may
 {
 	if (!__atomic_fetch_add(&ctx.signal_count, 1, __ATOMIC_RELAXED))
 		ctx.first_siginfo = *info;
-	__atomic_fetch_sub(&ctx.tids_want_signal, syscall(SYS_gettid), __ATOMIC_RELAXED);
+	__atomic_fetch_sub(&ctx.tids_want_signal, (pid_t)syscall(SYS_gettid), __ATOMIC_RELAXED);
 }
 
 static void *test_thread(void *arg)
 {
 	pthread_barrier_t *barrier = (pthread_barrier_t *)arg;
-	pid_t tid = syscall(SYS_gettid);
+	pid_t tid = (pid_t)syscall(SYS_gettid);
 	int i;
 
 	pthread_barrier_wait(barrier);
diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-tracking.c
index 8df3f9d9ffd2..1f0f8321ae40 100644
--- a/tools/perf/tests/switch-tracking.c
+++ b/tools/perf/tests/switch-tracking.c
@@ -140,8 +140,8 @@ static int process_sample_event(struct evlist *evlist,
 
 	evsel = evlist__id2evsel(evlist, sample.id);
 	if (evsel == switch_tracking->switch_evsel) {
-		next_tid = evsel__intval(evsel, &sample, "next_pid");
-		prev_tid = evsel__intval(evsel, &sample, "prev_pid");
+		next_tid = (int)evsel__intval(evsel, &sample, "next_pid");
+		prev_tid = (int)evsel__intval(evsel, &sample, "prev_pid");
 		cpu = sample.cpu;
 		pr_debug3("sched_switch: cpu: %d prev_tid %d next_tid %d\n",
 			  cpu, prev_tid, next_tid);
@@ -262,9 +262,10 @@ static int compar(const void *a, const void *b)
 {
 	const struct event_node *nodea = a;
 	const struct event_node *nodeb = b;
-	s64 cmp = nodea->event_time - nodeb->event_time;
 
-	return cmp;
+	if (nodea->event_time == nodeb->event_time)
+		return 0;
+	return nodea->event_time < nodeb->event_time ? -1 : 1;
 }
 
 static int process_events(struct evlist *evlist,
@@ -398,7 +399,7 @@ static int test__switch_tracking(struct test_suite *test __maybe_unused, int sub
 
 	switch_evsel = evlist__add_sched_switch(evlist, true);
 	if (IS_ERR(switch_evsel)) {
-		err = PTR_ERR(switch_evsel);
+		err = (int)PTR_ERR(switch_evsel);
 		pr_debug("Failed to create event %s\n", sched_switch);
 		goto out_err;
 	}
diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
index 74cdbd2ce9d0..fbdb463e965d 100644
--- a/tools/perf/tests/vmlinux-kallsyms.c
+++ b/tools/perf/tests/vmlinux-kallsyms.c
@@ -82,7 +82,7 @@ static bool is_ignored_symbol(const char *name, char type)
 			return true;
 
 	for (p = ignored_suffixes; *p; p++) {
-		int l = strlen(name) - strlen(*p);
+		ssize_t l = strlen(name) - strlen(*p);
 
 		if (l >= 0 && !strcmp(name + l, *p))
 			return true;
@@ -313,7 +313,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
 				 * _really_ have a problem.
 				 */
 				s64 skew = mem_end - UM(pair->end);
-				if (llabs(skew) >= page_size)
+				if (llabs(skew) >= (s64)page_size)
 					pr_debug("WARN: %#" PRIx64 ": diff end addr for %s v: %#" PRIx64 " k: %#" PRIx64 "\n",
 						 mem_start, sym->name, mem_end,
 						 UM(pair->end));
diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
index 6c178985e37f..d5dd17eb5c05 100644
--- a/tools/perf/tests/wp.c
+++ b/tools/perf/tests/wp.c
@@ -31,10 +31,10 @@ volatile u8 data2[3];
 #ifndef __s390x__
 static int wp_read(int fd, long long *count, int size)
 {
-	int ret = read(fd, count, size);
+	ssize_t ret = read(fd, count, size);
 
 	if (ret != size) {
-		pr_debug("failed to read: %d\n", ret);
+		pr_debug("failed to read: %zd\n", ret);
 		return -1;
 	}
 	return 0;
-- 
2.49.0.504.g3bcea36a83-goog

On Tue, Apr 01, 2025 at 11:23:07AM -0700, Ian Rogers wrote:

[...]

> @@ -478,13 +478,14 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>  	int err = 0;
>  
>  	for (struct test_suite **t = suites; *t; t++) {
> -		int i, len = strlen(test_description(*t, -1));
> +		int i;
> +		int len = (int)strlen(test_description(*t, -1));

Thanks for huge polish.

Just a concern from me.  Throughout this patch, the methodology is not
consistent.  Some changes update variable types which is fine for me.

But the case above it simply cast size_t to int.  Should we update the
variable type as 'size_t' at here?

>  		if (width < len)
>  			width = len;
>  
>  		test_suite__for_each_test_case(*t, i) {
> -			len = strlen(test_description(*t, i));
> +			len = (int)strlen(test_description(*t, i));
>  			if (width < len)
>  				width = len;
>  			num_tests += runs_per_test;
> diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> index cf6edbe697b2..0c31d5ff77e2 100644
> --- a/tools/perf/tests/code-reading.c
> +++ b/tools/perf/tests/code-reading.c
> @@ -188,7 +188,7 @@ static int objdump_version(void)
>  	char *line = NULL;
>  	const char *fmt;
>  	FILE *f;
> -	int ret;
> +	ssize_t ret;
>  
>  	int version_tmp, version_num = 0;
>  	char *version = 0, *token;
> @@ -295,7 +295,7 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
>  	if (len) {
>  		pr_debug("objdump read too few bytes: %zd\n", len);
>  		if (!ret)
> -			ret = len;
> +			ret = (int)len;

A proper change is to update the function type to:

  size_t read_via_objdump(...)

[...]

> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index db004d26fcb0..2762794827ce 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -609,7 +609,7 @@ static int __test_uncore_pmu_event_aliases(struct perf_pmu_test_pmu *test_pmu)
>  	pmu_add_sys_aliases(pmu);
>  
>  	/* Count how many aliases we generated */
> -	alias_count = perf_pmu__num_events(pmu);
> +	alias_count = (int)perf_pmu__num_events(pmu);

Could we change the variable 'alias_count' to size_t type?

Or in another way, we need to update perf_pmu__num_events() with
returning int type.

My understanding is rather than using cast, we should polish code for
using consistent type for both variables and return values.

>  	/* Count how many aliases we expect from the known table */
>  	for (table = &test_pmu->aliases[0]; *table; table++)
> diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> index a67c756f90b8..b7d7735f8a72 100644
> --- a/tools/perf/tests/sigtrap.c
> +++ b/tools/perf/tests/sigtrap.c
> @@ -154,13 +154,13 @@ sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __may
>  {
>  	if (!__atomic_fetch_add(&ctx.signal_count, 1, __ATOMIC_RELAXED))
>  		ctx.first_siginfo = *info;
> -	__atomic_fetch_sub(&ctx.tids_want_signal, syscall(SYS_gettid), __ATOMIC_RELAXED);
> +	__atomic_fetch_sub(&ctx.tids_want_signal, (pid_t)syscall(SYS_gettid), __ATOMIC_RELAXED);
>  }
>  
>  static void *test_thread(void *arg)
>  {
>  	pthread_barrier_t *barrier = (pthread_barrier_t *)arg;
> -	pid_t tid = syscall(SYS_gettid);
> +	pid_t tid = (pid_t)syscall(SYS_gettid);
>  	int i;
>  
>  	pthread_barrier_wait(barrier);
> diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-tracking.c
> index 8df3f9d9ffd2..1f0f8321ae40 100644
> --- a/tools/perf/tests/switch-tracking.c
> +++ b/tools/perf/tests/switch-tracking.c
> @@ -140,8 +140,8 @@ static int process_sample_event(struct evlist *evlist,
>  
>  	evsel = evlist__id2evsel(evlist, sample.id);
>  	if (evsel == switch_tracking->switch_evsel) {
> -		next_tid = evsel__intval(evsel, &sample, "next_pid");
> -		prev_tid = evsel__intval(evsel, &sample, "prev_pid");
> +		next_tid = (int)evsel__intval(evsel, &sample, "next_pid");
> +		prev_tid = (int)evsel__intval(evsel, &sample, "prev_pid");

Change 'prev_tid' and 'next_tid' to pid_t type?

Thanks,
Leo

>  		cpu = sample.cpu;
>  		pr_debug3("sched_switch: cpu: %d prev_tid %d next_tid %d\n",
>  			  cpu, prev_tid, next_tid);
> @@ -262,9 +262,10 @@ static int compar(const void *a, const void *b)
>  {
>  	const struct event_node *nodea = a;
>  	const struct event_node *nodeb = b;
> -	s64 cmp = nodea->event_time - nodeb->event_time;
>  
> -	return cmp;
> +	if (nodea->event_time == nodeb->event_time)
> +		return 0;
> +	return nodea->event_time < nodeb->event_time ? -1 : 1;
>  }
>  
>  static int process_events(struct evlist *evlist,
> @@ -398,7 +399,7 @@ static int test__switch_tracking(struct test_suite *test __maybe_unused, int sub
>  
>  	switch_evsel = evlist__add_sched_switch(evlist, true);
>  	if (IS_ERR(switch_evsel)) {
> -		err = PTR_ERR(switch_evsel);
> +		err = (int)PTR_ERR(switch_evsel);
>  		pr_debug("Failed to create event %s\n", sched_switch);
>  		goto out_err;
>  	}
> diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> index 74cdbd2ce9d0..fbdb463e965d 100644
> --- a/tools/perf/tests/vmlinux-kallsyms.c
> +++ b/tools/perf/tests/vmlinux-kallsyms.c
> @@ -82,7 +82,7 @@ static bool is_ignored_symbol(const char *name, char type)
>  			return true;
>  
>  	for (p = ignored_suffixes; *p; p++) {
> -		int l = strlen(name) - strlen(*p);
> +		ssize_t l = strlen(name) - strlen(*p);
>  
>  		if (l >= 0 && !strcmp(name + l, *p))
>  			return true;
> @@ -313,7 +313,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
>  				 * _really_ have a problem.
>  				 */
>  				s64 skew = mem_end - UM(pair->end);
> -				if (llabs(skew) >= page_size)
> +				if (llabs(skew) >= (s64)page_size)
>  					pr_debug("WARN: %#" PRIx64 ": diff end addr for %s v: %#" PRIx64 " k: %#" PRIx64 "\n",
>  						 mem_start, sym->name, mem_end,
>  						 UM(pair->end));
> diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
> index 6c178985e37f..d5dd17eb5c05 100644
> --- a/tools/perf/tests/wp.c
> +++ b/tools/perf/tests/wp.c
> @@ -31,10 +31,10 @@ volatile u8 data2[3];
>  #ifndef __s390x__
>  static int wp_read(int fd, long long *count, int size)
>  {
> -	int ret = read(fd, count, size);
> +	ssize_t ret = read(fd, count, size);
>  
>  	if (ret != size) {
> -		pr_debug("failed to read: %d\n", ret);
> +		pr_debug("failed to read: %zd\n", ret);
>  		return -1;
>  	}
>  	return 0;
> -- 
> 2.49.0.504.g3bcea36a83-goog
> 
>

On Wed, Apr 2, 2025 at 7:35 AM Leo Yan <leo.yan@arm.com> wrote:
>
> On Tue, Apr 01, 2025 at 11:23:07AM -0700, Ian Rogers wrote:
>
> [...]
>
> > @@ -478,13 +478,14 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
> >       int err = 0;
> >
> >       for (struct test_suite **t = suites; *t; t++) {
> > -             int i, len = strlen(test_description(*t, -1));
> > +             int i;
> > +             int len = (int)strlen(test_description(*t, -1));
>
> Thanks for huge polish.
>
> Just a concern from me.  Throughout this patch, the methodology is not
> consistent.  Some changes update variable types which is fine for me.
>
> But the case above it simply cast size_t to int.  Should we update the
> variable type as 'size_t' at here?

Thanks Leo, I played around with it, but don't mind if someone wants
to do it a different way. I was trying to make the changes minimal.
The problem typically with size_t is we then use the value, for
example, as a printf size modifier and need to introduce lots of casts
back to being an int. When this isn't too great I've done it, but in
this case I think keeping the int, the lack of casts but a cast here
to capture that we expect test descriptions to fit in the size of an
int is the least worst option.

> >               if (width < len)
> >                       width = len;
> >
> >               test_suite__for_each_test_case(*t, i) {
> > -                     len = strlen(test_description(*t, i));
> > +                     len = (int)strlen(test_description(*t, i));
> >                       if (width < len)
> >                               width = len;
> >                       num_tests += runs_per_test;
> > diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
> > index cf6edbe697b2..0c31d5ff77e2 100644
> > --- a/tools/perf/tests/code-reading.c
> > +++ b/tools/perf/tests/code-reading.c
> > @@ -188,7 +188,7 @@ static int objdump_version(void)
> >       char *line = NULL;
> >       const char *fmt;
> >       FILE *f;
> > -     int ret;
> > +     ssize_t ret;
> >
> >       int version_tmp, version_num = 0;
> >       char *version = 0, *token;
> > @@ -295,7 +295,7 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
> >       if (len) {
> >               pr_debug("objdump read too few bytes: %zd\n", len);
> >               if (!ret)
> > -                     ret = len;
> > +                     ret = (int)len;
>
> A proper change is to update the function type to:
>
>   size_t read_via_objdump(...)
>
> [...]

Normally the int is negative to error, 0 and more for a successful
positive read of so many bytes. We could switch the return type to
ssize_t but I was trying to avoid changing everything.

> > diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> > index db004d26fcb0..2762794827ce 100644
> > --- a/tools/perf/tests/pmu-events.c
> > +++ b/tools/perf/tests/pmu-events.c
> > @@ -609,7 +609,7 @@ static int __test_uncore_pmu_event_aliases(struct perf_pmu_test_pmu *test_pmu)
> >       pmu_add_sys_aliases(pmu);
> >
> >       /* Count how many aliases we generated */
> > -     alias_count = perf_pmu__num_events(pmu);
> > +     alias_count = (int)perf_pmu__num_events(pmu);
>
> Could we change the variable 'alias_count' to size_t type?
>
> Or in another way, we need to update perf_pmu__num_events() with
> returning int type.
>
> My understanding is rather than using cast, we should polish code for
> using consistent type for both variables and return values.

We can, I was trying to keep the size of the change down. Later the
code will compare against matched_count which is an int, so using a
size_t means a signed vs unsigned compare, so a multi-line change vs 1
cast.

> >       /* Count how many aliases we expect from the known table */
> >       for (table = &test_pmu->aliases[0]; *table; table++)
> > diff --git a/tools/perf/tests/sigtrap.c b/tools/perf/tests/sigtrap.c
> > index a67c756f90b8..b7d7735f8a72 100644
> > --- a/tools/perf/tests/sigtrap.c
> > +++ b/tools/perf/tests/sigtrap.c
> > @@ -154,13 +154,13 @@ sigtrap_handler(int signum __maybe_unused, siginfo_t *info, void *ucontext __may
> >  {
> >       if (!__atomic_fetch_add(&ctx.signal_count, 1, __ATOMIC_RELAXED))
> >               ctx.first_siginfo = *info;
> > -     __atomic_fetch_sub(&ctx.tids_want_signal, syscall(SYS_gettid), __ATOMIC_RELAXED);
> > +     __atomic_fetch_sub(&ctx.tids_want_signal, (pid_t)syscall(SYS_gettid), __ATOMIC_RELAXED);
> >  }
> >
> >  static void *test_thread(void *arg)
> >  {
> >       pthread_barrier_t *barrier = (pthread_barrier_t *)arg;
> > -     pid_t tid = syscall(SYS_gettid);
> > +     pid_t tid = (pid_t)syscall(SYS_gettid);
> >       int i;
> >
> >       pthread_barrier_wait(barrier);
> > diff --git a/tools/perf/tests/switch-tracking.c b/tools/perf/tests/switch-tracking.c
> > index 8df3f9d9ffd2..1f0f8321ae40 100644
> > --- a/tools/perf/tests/switch-tracking.c
> > +++ b/tools/perf/tests/switch-tracking.c
> > @@ -140,8 +140,8 @@ static int process_sample_event(struct evlist *evlist,
> >
> >       evsel = evlist__id2evsel(evlist, sample.id);
> >       if (evsel == switch_tracking->switch_evsel) {
> > -             next_tid = evsel__intval(evsel, &sample, "next_pid");
> > -             prev_tid = evsel__intval(evsel, &sample, "prev_pid");
> > +             next_tid = (int)evsel__intval(evsel, &sample, "next_pid");
> > +             prev_tid = (int)evsel__intval(evsel, &sample, "prev_pid");
>
> Change 'prev_tid' and 'next_tid' to pid_t type?

I agree, but I was trying to do what was minimal and use the types as
they already were. Perhaps we should have multiple of the
evsel__<type>val helpers. Again, I wanted to do what was minimal in
order for the warning to be silenced.

Thanks,
Ian

> Thanks,
> Leo
>
> >               cpu = sample.cpu;
> >               pr_debug3("sched_switch: cpu: %d prev_tid %d next_tid %d\n",
> >                         cpu, prev_tid, next_tid);
> > @@ -262,9 +262,10 @@ static int compar(const void *a, const void *b)
> >  {
> >       const struct event_node *nodea = a;
> >       const struct event_node *nodeb = b;
> > -     s64 cmp = nodea->event_time - nodeb->event_time;
> >
> > -     return cmp;
> > +     if (nodea->event_time == nodeb->event_time)
> > +             return 0;
> > +     return nodea->event_time < nodeb->event_time ? -1 : 1;
> >  }
> >
> >  static int process_events(struct evlist *evlist,
> > @@ -398,7 +399,7 @@ static int test__switch_tracking(struct test_suite *test __maybe_unused, int sub
> >
> >       switch_evsel = evlist__add_sched_switch(evlist, true);
> >       if (IS_ERR(switch_evsel)) {
> > -             err = PTR_ERR(switch_evsel);
> > +             err = (int)PTR_ERR(switch_evsel);
> >               pr_debug("Failed to create event %s\n", sched_switch);
> >               goto out_err;
> >       }
> > diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> > index 74cdbd2ce9d0..fbdb463e965d 100644
> > --- a/tools/perf/tests/vmlinux-kallsyms.c
> > +++ b/tools/perf/tests/vmlinux-kallsyms.c
> > @@ -82,7 +82,7 @@ static bool is_ignored_symbol(const char *name, char type)
> >                       return true;
> >
> >       for (p = ignored_suffixes; *p; p++) {
> > -             int l = strlen(name) - strlen(*p);
> > +             ssize_t l = strlen(name) - strlen(*p);
> >
> >               if (l >= 0 && !strcmp(name + l, *p))
> >                       return true;
> > @@ -313,7 +313,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> >                                * _really_ have a problem.
> >                                */
> >                               s64 skew = mem_end - UM(pair->end);
> > -                             if (llabs(skew) >= page_size)
> > +                             if (llabs(skew) >= (s64)page_size)
> >                                       pr_debug("WARN: %#" PRIx64 ": diff end addr for %s v: %#" PRIx64 " k: %#" PRIx64 "\n",
> >                                                mem_start, sym->name, mem_end,
> >                                                UM(pair->end));
> > diff --git a/tools/perf/tests/wp.c b/tools/perf/tests/wp.c
> > index 6c178985e37f..d5dd17eb5c05 100644
> > --- a/tools/perf/tests/wp.c
> > +++ b/tools/perf/tests/wp.c
> > @@ -31,10 +31,10 @@ volatile u8 data2[3];
> >  #ifndef __s390x__
> >  static int wp_read(int fd, long long *count, int size)
> >  {
> > -     int ret = read(fd, count, size);
> > +     ssize_t ret = read(fd, count, size);
> >
> >       if (ret != size) {
> > -             pr_debug("failed to read: %d\n", ret);
> > +             pr_debug("failed to read: %zd\n", ret);
> >               return -1;
> >       }
> >       return 0;
> > --
> > 2.49.0.504.g3bcea36a83-goog
> >
> >

Hi Ian,

On Wed, Apr 02, 2025 at 08:42:58AM -0700, Ian Rogers wrote:

[...]

> On Wed, Apr 2, 2025 at 7:35 AM Leo Yan <leo.yan@arm.com> wrote:
> >
> > On Tue, Apr 01, 2025 at 11:23:07AM -0700, Ian Rogers wrote:
> >
> > [...]
> >
> > > @@ -478,13 +478,14 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
> > >       int err = 0;
> > >
> > >       for (struct test_suite **t = suites; *t; t++) {
> > > -             int i, len = strlen(test_description(*t, -1));
> > > +             int i;
> > > +             int len = (int)strlen(test_description(*t, -1));
> >
> > Thanks for huge polish.
> >
> > Just a concern from me.  Throughout this patch, the methodology is not
> > consistent.  Some changes update variable types which is fine for me.
> >
> > But the case above it simply cast size_t to int.  Should we update the
> > variable type as 'size_t' at here?
> 
> Thanks Leo, I played around with it, but don't mind if someone wants
> to do it a different way. I was trying to make the changes minimal.
> The problem typically with size_t is we then use the value, for
> example, as a printf size modifier and need to introduce lots of casts
> back to being an int.

This conclusion is not quite right, see:
https://stackoverflow.com/questions/2524611/how-can-one-print-a-size-t-variable-portably-using-the-printf-family

> When this isn't too great I've done it, but in
> this case I think keeping the int, the lack of casts but a cast here
> to capture that we expect test descriptions to fit in the size of an
> int is the least worst option.

I would say in another way.  After we enabled a compiler warning
option, this will give us a chance to improve code by dismissing
the warnings (and avoid potential bugs).

If we use casts just to silence warnings, we also lose the opportunity
to improve code quality.  The changes in this series that fix type
mismatches are good, but I think the use of casts is not particularly
helpful - we're simply switching from implicit compiler casts to
explicit ones.

Thanks,
Leo

On Wed, 2 Apr 2025 17:38:07 +0100
Leo Yan <leo.yan@arm.com> wrote:

> Hi Ian,
...
> If we use casts just to silence warnings, we also lose the opportunity
> to improve code quality.  The changes in this series that fix type
> mismatches are good, but I think the use of casts is not particularly
> helpful - we're simply switching from implicit compiler casts to
> explicit ones.

It is certainly my experience (a lot of years) that casts between
integers of different sizes just make code harder to read and possibly
even more buggy.

If the compiler really knew the valid range of the value (rather than
just the type) then perhaps a warning that significant bits were being
discarded might be more reasonable.
But even then you don't want anything for code like:
	hi_32 = val_64 >> 32;
	lo_32 = val_64;

    David

On Wed, Apr 2, 2025 at 3:01 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Wed, 2 Apr 2025 17:38:07 +0100
> Leo Yan <leo.yan@arm.com> wrote:
>
> > Hi Ian,
> ...
> > If we use casts just to silence warnings, we also lose the opportunity
> > to improve code quality.  The changes in this series that fix type
> > mismatches are good, but I think the use of casts is not particularly
> > helpful - we're simply switching from implicit compiler casts to
> > explicit ones.
>
> It is certainly my experience (a lot of years) that casts between
> integers of different sizes just make code harder to read and possibly
> even more buggy.
>
> If the compiler really knew the valid range of the value (rather than
> just the type) then perhaps a warning that significant bits were being
> discarded might be more reasonable.
> But even then you don't want anything for code like:
>         hi_32 = val_64 >> 32;
>         lo_32 = val_64;

There is an instance of this in:
https://lore.kernel.org/lkml/20250401182347.3422199-3-irogers@google.com/

The particular problem that Leo Yan [1] found in the code base is if a
compare function like:

  int cmp(long *a, long *b)
  {
      return *a - *b;
  }

which are typically passed to routines like list_sort, qsort or
bsearch. The subtract is potentially 64-bit and the implicit cast to
32-bit loses the sign information. Generally the problem is more
opaque than this as here you can quite easily see the types of "a" and
"b". If we say we don't warn for implicit truncating assignments then:

  int cmp(long *a, long *b)
  {
      int ret = *a - *b;
      return ret;
  }

becomes valid, but it is identical and as broken as the code before.
I'm happy for a better alternative than clang's `-Wshorten-64-to-32`
but haven't found it and I agree it's unfortunate for the churn it
kicks up.

As an aside, I wrote a Java check for this as I found a similar bug in
Android. That check found instances where the subtract and truncate
were happening in a cache that would sort elements deleting the
oldest. Unfortunately the truncation meant it could also be deleting
the newest elements. In Java lossy conversions require a cast so I
could target the warning on casts within compare routines. C of course
has given us a good way to shoot ourselves in the foot with implicit
conversion and -Wshorten-64-to-32 seems to be a good way of avoiding
that particular foot cannon. Making ordering routines use a less-than
comparison avoids the problem in C++.

Thanks,
Ian

[1] https://lore.kernel.org/lkml/20250331172759.115604-1-leo.yan@arm.com/
>     David

On Wed, Apr 2, 2025 at 9:38 AM Leo Yan <leo.yan@arm.com> wrote:
>
> Hi Ian,
>
> On Wed, Apr 02, 2025 at 08:42:58AM -0700, Ian Rogers wrote:
>
> [...]
>
> > On Wed, Apr 2, 2025 at 7:35 AM Leo Yan <leo.yan@arm.com> wrote:
> > >
> > > On Tue, Apr 01, 2025 at 11:23:07AM -0700, Ian Rogers wrote:
> > >
> > > [...]
> > >
> > > > @@ -478,13 +478,14 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
> > > >       int err = 0;
> > > >
> > > >       for (struct test_suite **t = suites; *t; t++) {
> > > > -             int i, len = strlen(test_description(*t, -1));
> > > > +             int i;
> > > > +             int len = (int)strlen(test_description(*t, -1));
> > >
> > > Thanks for huge polish.
> > >
> > > Just a concern from me.  Throughout this patch, the methodology is not
> > > consistent.  Some changes update variable types which is fine for me.
> > >
> > > But the case above it simply cast size_t to int.  Should we update the
> > > variable type as 'size_t' at here?
> >
> > Thanks Leo, I played around with it, but don't mind if someone wants
> > to do it a different way. I was trying to make the changes minimal.
> > The problem typically with size_t is we then use the value, for
> > example, as a printf size modifier and need to introduce lots of casts
> > back to being an int.
>
> This conclusion is not quite right, see:
> https://stackoverflow.com/questions/2524611/how-can-one-print-a-size-t-variable-portably-using-the-printf-family
>
> > When this isn't too great I've done it, but in
> > this case I think keeping the int, the lack of casts but a cast here
> > to capture that we expect test descriptions to fit in the size of an
> > int is the least worst option.
>
> I would say in another way.  After we enabled a compiler warning
> option, this will give us a chance to improve code by dismissing
> the warnings (and avoid potential bugs).
>
> If we use casts just to silence warnings, we also lose the opportunity
> to improve code quality.  The changes in this series that fix type
> mismatches are good, but I think the use of casts is not particularly
> helpful - we're simply switching from implicit compiler casts to
> explicit ones.

Right, but I think changing function parameters, return types would
turn into an epic refactor and the patch series is already unwieldy.
With the casts we can see the behavior is deliberate but that's not to
say it couldn't be better. With the warnings gone it allows
-Wshorten-64-to-32 to find the truly errant 64 to 32 bit implicit
casts. Anyway, I don't have time to do such a larger refactor so
somebody else is going to need to pick that up. I did refactor the
cases where I thought it mattered more, but as you say that does lead
to a feeling of inconsistency in the series.

Thanks,
Ian

> Thanks,
> Leo

On Wed, Apr 02, 2025 at 09:53:57AM -0700, Ian Rogers wrote:
> On Wed, Apr 2, 2025 at 9:38 AM Leo Yan <leo.yan@arm.com> wrote:
> >
> > Hi Ian,
> >
> > On Wed, Apr 02, 2025 at 08:42:58AM -0700, Ian Rogers wrote:
> >
> > [...]
> >
> > > On Wed, Apr 2, 2025 at 7:35 AM Leo Yan <leo.yan@arm.com> wrote:
> > > >
> > > > On Tue, Apr 01, 2025 at 11:23:07AM -0700, Ian Rogers wrote:
> > > >
> > > > [...]
> > > >
> > > > > @@ -478,13 +478,14 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
> > > > >       int err = 0;
> > > > >
> > > > >       for (struct test_suite **t = suites; *t; t++) {
> > > > > -             int i, len = strlen(test_description(*t, -1));
> > > > > +             int i;
> > > > > +             int len = (int)strlen(test_description(*t, -1));
> > > >
> > > > Thanks for huge polish.
> > > >
> > > > Just a concern from me.  Throughout this patch, the methodology is not
> > > > consistent.  Some changes update variable types which is fine for me.
> > > >
> > > > But the case above it simply cast size_t to int.  Should we update the
> > > > variable type as 'size_t' at here?
> > >
> > > Thanks Leo, I played around with it, but don't mind if someone wants
> > > to do it a different way. I was trying to make the changes minimal.
> > > The problem typically with size_t is we then use the value, for
> > > example, as a printf size modifier and need to introduce lots of casts
> > > back to being an int.
> >
> > This conclusion is not quite right, see:
> > https://stackoverflow.com/questions/2524611/how-can-one-print-a-size-t-variable-portably-using-the-printf-family
> >
> > > When this isn't too great I've done it, but in
> > > this case I think keeping the int, the lack of casts but a cast here
> > > to capture that we expect test descriptions to fit in the size of an
> > > int is the least worst option.
> >
> > I would say in another way.  After we enabled a compiler warning
> > option, this will give us a chance to improve code by dismissing
> > the warnings (and avoid potential bugs).
> >
> > If we use casts just to silence warnings, we also lose the opportunity
> > to improve code quality.  The changes in this series that fix type
> > mismatches are good, but I think the use of casts is not particularly
> > helpful - we're simply switching from implicit compiler casts to
> > explicit ones.
> 
> Right, but I think changing function parameters, return types would
> turn into an epic refactor and the patch series is already unwieldy.

Yep, it's important to reduce the number and the size of patches from
the reviewer's pespective. :)


> With the casts we can see the behavior is deliberate but that's not to
> say it couldn't be better. With the warnings gone it allows
> -Wshorten-64-to-32 to find the truly errant 64 to 32 bit implicit

Do we want to enable this warning?  Is it only available on clang?


> casts. Anyway, I don't have time to do such a larger refactor so
> somebody else is going to need to pick that up. I did refactor the
> cases where I thought it mattered more, but as you say that does lead
> to a feeling of inconsistency in the series.

I'm curious there are any actual errorenous cases other than the return
type of comparisons from Leo.

Thnaks,
Namhyung

On Wed, Apr 2, 2025 at 9:53 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Apr 02, 2025 at 09:53:57AM -0700, Ian Rogers wrote:
> > On Wed, Apr 2, 2025 at 9:38 AM Leo Yan <leo.yan@arm.com> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Wed, Apr 02, 2025 at 08:42:58AM -0700, Ian Rogers wrote:
> > >
> > > [...]
> > >
> > > > On Wed, Apr 2, 2025 at 7:35 AM Leo Yan <leo.yan@arm.com> wrote:
> > > > >
> > > > > On Tue, Apr 01, 2025 at 11:23:07AM -0700, Ian Rogers wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > @@ -478,13 +478,14 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
> > > > > >       int err = 0;
> > > > > >
> > > > > >       for (struct test_suite **t = suites; *t; t++) {
> > > > > > -             int i, len = strlen(test_description(*t, -1));
> > > > > > +             int i;
> > > > > > +             int len = (int)strlen(test_description(*t, -1));
> > > > >
> > > > > Thanks for huge polish.
> > > > >
> > > > > Just a concern from me.  Throughout this patch, the methodology is not
> > > > > consistent.  Some changes update variable types which is fine for me.
> > > > >
> > > > > But the case above it simply cast size_t to int.  Should we update the
> > > > > variable type as 'size_t' at here?
> > > >
> > > > Thanks Leo, I played around with it, but don't mind if someone wants
> > > > to do it a different way. I was trying to make the changes minimal.
> > > > The problem typically with size_t is we then use the value, for
> > > > example, as a printf size modifier and need to introduce lots of casts
> > > > back to being an int.
> > >
> > > This conclusion is not quite right, see:
> > > https://stackoverflow.com/questions/2524611/how-can-one-print-a-size-t-variable-portably-using-the-printf-family
> > >
> > > > When this isn't too great I've done it, but in
> > > > this case I think keeping the int, the lack of casts but a cast here
> > > > to capture that we expect test descriptions to fit in the size of an
> > > > int is the least worst option.
> > >
> > > I would say in another way.  After we enabled a compiler warning
> > > option, this will give us a chance to improve code by dismissing
> > > the warnings (and avoid potential bugs).
> > >
> > > If we use casts just to silence warnings, we also lose the opportunity
> > > to improve code quality.  The changes in this series that fix type
> > > mismatches are good, but I think the use of casts is not particularly
> > > helpful - we're simply switching from implicit compiler casts to
> > > explicit ones.
> >
> > Right, but I think changing function parameters, return types would
> > turn into an epic refactor and the patch series is already unwieldy.
>
> Yep, it's important to reduce the number and the size of patches from
> the reviewer's pespective. :)
>
>
> > With the casts we can see the behavior is deliberate but that's not to
> > say it couldn't be better. With the warnings gone it allows
> > -Wshorten-64-to-32 to find the truly errant 64 to 32 bit implicit
>
> Do we want to enable this warning?  Is it only available on clang?

It is clang only. libbpf fails with the warning enabled, so enabling
it would break all the BPF stuff which isn't good. That said we could
step toward enabling it, which is what the series does.

> > casts. Anyway, I don't have time to do such a larger refactor so
> > somebody else is going to need to pick that up. I did refactor the
> > cases where I thought it mattered more, but as you say that does lead
> > to a feeling of inconsistency in the series.
>
> I'm curious there are any actual errorenous cases other than the return
> type of comparisons from Leo.

There were other cases of Leo's issue here:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/ui/hist.c?h=perf-tools-next#n290
```
ret = b->callchain->max_depth - a->callchain->max_depth;
```
you can see that file has field_cmp to handle 64 bit comparisons
returning an int, presumably as this problem had been seen before, and
possibly here once the left_count and right_count are made size_ts to
match the hashmap size operation.
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/metricgroup.c?h=perf-tools-next#n1162

xyarray was a source of differences as the max values are size_t,
which is more than they need to be.
The libperf mmap code was truncating the mask field which didn't look good.
The pipe benchmark, and similar, were truncating the read amount,
which should never be an issue as we shouldn't shove 4GB down a pipe,
but it just isn't good.
There are lots of cases like:
```
                case ARM_SPE_COUNTER:
                       if (idx == SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT)
-                               decoder->record.latency = payload;
+                               decoder->record.latency = (u32)payload;
```
and
```
@@ -4830,7 +4836,7 @@ static struct syscall_entry
*syscall__sort_stats(struct hashmap *syscall_stats)

               entry[i].stats = ss;
               entry[i].msecs = (u64)st->n * (avg_stats(st) / NSEC_PER_MSEC);
-               entry[i].syscall = pos->key;
+               entry[i].syscall = (int)pos->key;
```
where I think the cast is informative that the top half of the payload
is being discarded, or the key deliberately truncated. In general I
think having the casts makes the behavior of the code more explicit
and less surprising, which is a good thing.

Thanks,
Ian

> Thnaks,
> Namhyung
>

Hi Namhyung, Ian,

On Thu, Apr 03, 2025 at 08:20:02AM -0700, Ian Rogers wrote:

[...]

> > > Anyway, I don't have time to do such a larger refactor so
> > > somebody else is going to need to pick that up. I did refactor the
> > > cases where I thought it mattered more, but as you say that does lead
> > > to a feeling of inconsistency in the series.
> >
> > I'm curious there are any actual errorenous cases other than the return
> > type of comparisons from Leo.

I am just wandering if we could give priority for errorenous cases.

Could you send fixes for these cases firstly and leave out refactoring
for later merging?

[...]

> There are lots of cases like:
> ```
>                 case ARM_SPE_COUNTER:
>                        if (idx == SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT)
> -                               decoder->record.latency = payload;
> +                               decoder->record.latency = (u32)payload;

This would be fine.  Since Arm SPE implements a counter with a maximum
of 16 bits, there will never be an overflow when storing the data in a
32-bit unsigned integer.

Thanks,
Leo

On Mon, Apr 28, 2025 at 4:10 AM Leo Yan <leo.yan@arm.com> wrote:
>
> Hi Namhyung, Ian,
>
> On Thu, Apr 03, 2025 at 08:20:02AM -0700, Ian Rogers wrote:
>
> [...]
>
> > > > Anyway, I don't have time to do such a larger refactor so
> > > > somebody else is going to need to pick that up. I did refactor the
> > > > cases where I thought it mattered more, but as you say that does lead
> > > > to a feeling of inconsistency in the series.
> > >
> > > I'm curious there are any actual errorenous cases other than the return
> > > type of comparisons from Leo.
>
> I am just wandering if we could give priority for errorenous cases.
>
> Could you send fixes for these cases firstly and leave out refactoring
> for later merging?
>
> [...]

Hi Leo,

So I sent out cleaning up the kernel headers separately:
https://lore.kernel.org/lkml/20250403165702.396388-1-irogers@google.com/
Arnd commented that it would be nicer to do larger changes, but my
concern was breaking printf modifiers, etc. It looks like those
patches have lost momentum, not sure what's going to happen about them
being merged.

On the fixes vs refactoring. I'm not sure refactoring is the right
term for the other things in the series. It is basically trying to
make implicit casts explicit, without doing some boil the ocean
exercise. I went through the issues in the email this is a reply to:
https://lore.kernel.org/lkml/CAP-5=fXwAA0PNYAOLNLdHY8g+AyEB0UxmzgX5w-gGj_DZqUxtg@mail.gmail.com/
Do you want to send the bits you think need prioritizing to the list?

> > There are lots of cases like:
> > ```
> >                 case ARM_SPE_COUNTER:
> >                        if (idx == SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT)
> > -                               decoder->record.latency = payload;
> > +                               decoder->record.latency = (u32)payload;
>
> This would be fine.  Since Arm SPE implements a counter with a maximum
> of 16 bits, there will never be an overflow when storing the data in a
> 32-bit unsigned integer.

The payload was > sizeof(u32), should record.latency be a u16? My
point wasn't so much to complain about adding a u32 cast in the SPE
code, but that having the cast makes it clear that truncation is
expected. Adding the casts isn't fixing a bug, but it is making it
clearer that the code will truncate the payload value (imo).

Thanks,
Ian

> Thanks,
> Leo

Hi Ian,

On Mon, Apr 28, 2025 at 09:29:37AM -0700, Ian Rogers wrote:

[...]

> Hi Leo,
> 
> So I sent out cleaning up the kernel headers separately:
> https://lore.kernel.org/lkml/20250403165702.396388-1-irogers@google.com/
> Arnd commented that it would be nicer to do larger changes, but my
> concern was breaking printf modifiers, etc. It looks like those
> patches have lost momentum, not sure what's going to happen about them
> being merged.
> 
> On the fixes vs refactoring. I'm not sure refactoring is the right
> term for the other things in the series. It is basically trying to
> make implicit casts explicit, without doing some boil the ocean
> exercise.

Yeah, I agreed that some "refactoring" is actually fixing bugs.

> I went through the issues in the email this is a reply to:
> https://lore.kernel.org/lkml/CAP-5=fXwAA0PNYAOLNLdHY8g+AyEB0UxmzgX5w-gGj_DZqUxtg@mail.gmail.com/
> Do you want to send the bits you think need prioritizing to the list?

For me, at least two things can be fixed first:

- One is the test case switch-tracking.c is broken on Arm64, your
  change for the compar() function would be fine for me.

- You mentioned the issue in ui/hist.c:
  ret = b->callchain->max_depth - a->callchain->max_depth;

These two issues are similiar and can fixed in the same format.

> > > There are lots of cases like:
> > > ```
> > >                 case ARM_SPE_COUNTER:
> > >                        if (idx == SPE_CNT_PKT_HDR_INDEX_TOTAL_LAT)
> > > -                               decoder->record.latency = payload;
> > > +                               decoder->record.latency = (u32)payload;
> >
> > This would be fine.  Since Arm SPE implements a counter with a maximum
> > of 16 bits, there will never be an overflow when storing the data in a
> > 32-bit unsigned integer.
> 
> The payload was > sizeof(u32), should record.latency be a u16?

Yes, we can change to u16 type for latency.

> My point wasn't so much to complain about adding a u32 cast in the SPE
> code, but that having the cast makes it clear that truncation is
> expected. Adding the casts isn't fixing a bug, but it is making it
> clearer that the code will truncate the payload value (imo).

As Arm SPE has defined the format, I would prefer to refine the code
based on the format:

  #define SPE_CNT_PKT_COUNT(v)    ((v) & GENMASK_ULL(15, 0))

  decoder->record.latency = SPE_CNT_PKT_COUNT(payload);

I can send a minor patch series for this, if not conflict with your
side.

Thanks,
Leo