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/bench/breakpoint.c           |  8 +++++---
 tools/perf/bench/epoll-wait.c           |  3 ++-
 tools/perf/bench/evlist-open-close.c    |  8 ++++----
 tools/perf/bench/find-bit-bench.c       |  5 +++--
 tools/perf/bench/futex.h                |  4 ++--
 tools/perf/bench/inject-buildid.c       |  8 ++++----
 tools/perf/bench/mem-functions.c        |  2 +-
 tools/perf/bench/pmu-scan.c             | 12 ++++++------
 tools/perf/bench/sched-messaging.c      |  4 ++--
 tools/perf/bench/sched-pipe.c           | 15 +++++++++------
 tools/perf/bench/sched-seccomp-notify.c |  2 +-
 tools/perf/bench/synthesize.c           |  6 +++---
 tools/perf/builtin-bench.c              |  2 +-
 13 files changed, 43 insertions(+), 36 deletions(-)

diff --git a/tools/perf/bench/breakpoint.c b/tools/perf/bench/breakpoint.c
index dfd18f5db97d..72802b334eeb 100644
--- a/tools/perf/bench/breakpoint.c
+++ b/tools/perf/bench/breakpoint.c
@@ -57,7 +57,7 @@ static int breakpoint_setup(void *addr)
 	attr.bp_addr = (unsigned long)addr;
 	attr.bp_type = HW_BREAKPOINT_RW;
 	attr.bp_len = HW_BREAKPOINT_LEN_1;
-	fd = syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
+	fd = (int)syscall(SYS_perf_event_open, &attr, 0, -1, -1, 0);
 
 	if (fd < 0)
 		fd = -errno;
@@ -111,7 +111,8 @@ static void *breakpoint_thread(void *arg)
 // then starts nparallel threads which create and join bench_repeat batches of nthreads threads.
 int bench_breakpoint_thread(int argc, const char **argv)
 {
-	unsigned int i, result_usec;
+	unsigned int i;
+	long result_usec;
 	int repeat = bench_repeat;
 	struct breakpoint *breakpoints;
 	pthread_t *parallel;
@@ -197,7 +198,8 @@ static const char * const enable_usage[] = {
 // and then disables and enables the breakpoint bench_repeat times.
 int bench_breakpoint_enable(int argc, const char **argv)
 {
-	unsigned int i, nthreads, result_usec, done = 0;
+	unsigned int i, nthreads, done = 0;
+	long result_usec;
 	char watched;
 	int fd;
 	pthread_t *threads;
diff --git a/tools/perf/bench/epoll-wait.c b/tools/perf/bench/epoll-wait.c
index 20fe4f72b4af..5ea3e0b2daf2 100644
--- a/tools/perf/bench/epoll-wait.c
+++ b/tools/perf/bench/epoll-wait.c
@@ -182,7 +182,8 @@ static void shuffle(void *array, size_t n, size_t size)
 
 static void *workerfn(void *arg)
 {
-	int fd, ret, r;
+	int fd, ret;
+	ssize_t r;
 	struct worker *w = (struct worker *) arg;
 	unsigned long ops = w->ops;
 	struct epoll_event ev;
diff --git a/tools/perf/bench/evlist-open-close.c b/tools/perf/bench/evlist-open-close.c
index 79cedcf94a39..6b75e2dc2fbf 100644
--- a/tools/perf/bench/evlist-open-close.c
+++ b/tools/perf/bench/evlist-open-close.c
@@ -173,16 +173,16 @@ static char *bench__repeat_event_string(const char *evstr, int n)
 {
 	char sbuf[STRERR_BUFSIZE];
 	struct strbuf buf;
-	int i, str_size = strlen(evstr),
-	    final_size = str_size * n + n,
-	    err = strbuf_init(&buf, final_size);
+	size_t str_size = strlen(evstr);
+	size_t final_size = str_size * n + n;
+	int err = strbuf_init(&buf, final_size);
 
 	if (err) {
 		pr_err("strbuf_init: %s\n", str_error_r(err, sbuf, sizeof(sbuf)));
 		goto out_error;
 	}
 
-	for (i = 0; i < n; i++) {
+	for (int i = 0; i < n; i++) {
 		err = strbuf_add(&buf, evstr, str_size);
 		if (err) {
 			pr_err("strbuf_add: %s\n", str_error_r(err, sbuf, sizeof(sbuf)));
diff --git a/tools/perf/bench/find-bit-bench.c b/tools/perf/bench/find-bit-bench.c
index 7e25b0e413f6..59b00e89f71b 100644
--- a/tools/perf/bench/find-bit-bench.c
+++ b/tools/perf/bench/find-bit-bench.c
@@ -31,7 +31,7 @@ static const char *const bench_usage[] = {
 static unsigned int accumulator;
 static unsigned int use_of_val;
 
-static noinline void workload(int val)
+static noinline void workload(unsigned long val)
 {
 	use_of_val += val;
 	accumulator++;
@@ -59,8 +59,9 @@ static int do_for_each_set_bit(unsigned int num_bits)
 	u64 runtime_us;
 	struct stats fb_time_stats, tb_time_stats;
 	double time_average, time_stddev;
-	unsigned int bit, i, j;
+	unsigned int i, j;
 	unsigned int set_bits, skip;
+	unsigned long bit;
 
 	init_stats(&fb_time_stats);
 	init_stats(&tb_time_stats);
diff --git a/tools/perf/bench/futex.h b/tools/perf/bench/futex.h
index ebdc2b032afc..6d9076e091eb 100644
--- a/tools/perf/bench/futex.h
+++ b/tools/perf/bench/futex.h
@@ -49,14 +49,14 @@ static inline int
 futex_syscall(volatile u_int32_t *uaddr, int op, u_int32_t val, struct timespec *timeout,
 	      volatile u_int32_t *uaddr2, int val3, int opflags)
 {
-	return syscall(SYS_futex, uaddr, op | opflags, val, timeout, uaddr2, val3);
+	return (int)syscall(SYS_futex, uaddr, op | opflags, val, timeout, uaddr2, val3);
 }
 
 static inline int
 futex_syscall_nr_requeue(volatile u_int32_t *uaddr, int op, u_int32_t val, int nr_requeue,
 			 volatile u_int32_t *uaddr2, int val3, int opflags)
 {
-	return syscall(SYS_futex, uaddr, op | opflags, val, nr_requeue, uaddr2, val3);
+	return (int)syscall(SYS_futex, uaddr, op | opflags, val, nr_requeue, uaddr2, val3);
 }
 
 /**
diff --git a/tools/perf/bench/inject-buildid.c b/tools/perf/bench/inject-buildid.c
index f55c07e4be94..b78de817e721 100644
--- a/tools/perf/bench/inject-buildid.c
+++ b/tools/perf/bench/inject-buildid.c
@@ -175,7 +175,7 @@ static ssize_t synthesize_mmap(struct bench_data *data, struct bench_dso *dso, u
 	union perf_event event;
 	size_t len = offsetof(struct perf_record_mmap2, filename);
 	u64 *id_hdr_ptr = (void *)&event;
-	int ts_idx;
+	size_t ts_idx;
 
 	len += roundup(strlen(dso->name) + 1, 8) + bench_id_hdr_size;
 
@@ -250,14 +250,14 @@ static void *data_reader(void *arg)
 	struct bench_data *data = arg;
 	char buf[8192];
 	int flag;
-	int n;
 
 	flag = fcntl(data->output_pipe[0], F_GETFL);
 	fcntl(data->output_pipe[0], F_SETFL, flag | O_NONBLOCK);
 
 	/* read out data from child */
 	while (true) {
-		n = read(data->output_pipe[0], buf, sizeof(buf));
+		ssize_t n = read(data->output_pipe[0], buf, sizeof(buf));
+
 		if (n > 0)
 			continue;
 		if (n == 0)
@@ -451,7 +451,7 @@ static void do_inject_loop(struct bench_data *data, bool build_id_all)
 static int do_inject_loops(struct bench_data *data)
 {
 
-	srand(time(NULL));
+	srand((int)time(NULL));
 	symbol__init(NULL);
 
 	bench_sample_type  = PERF_SAMPLE_IDENTIFIER | PERF_SAMPLE_IP;
diff --git a/tools/perf/bench/mem-functions.c b/tools/perf/bench/mem-functions.c
index 19d45c377ac1..672b76eb8f99 100644
--- a/tools/perf/bench/mem-functions.c
+++ b/tools/perf/bench/mem-functions.c
@@ -82,7 +82,7 @@ static int init_cycles(void)
 
 static u64 get_cycles(void)
 {
-	int ret;
+	ssize_t ret;
 	u64 clk;
 
 	ret = read(cycles_fd, &clk, sizeof(u64));
diff --git a/tools/perf/bench/pmu-scan.c b/tools/perf/bench/pmu-scan.c
index 9e4d36486f62..4a885d4bd279 100644
--- a/tools/perf/bench/pmu-scan.c
+++ b/tools/perf/bench/pmu-scan.c
@@ -19,8 +19,8 @@ static unsigned int iterations = 100;
 
 struct pmu_scan_result {
 	char *name;
-	int nr_aliases;
-	int nr_formats;
+	size_t nr_aliases;
+	size_t nr_formats;
 	int nr_caps;
 	bool is_core;
 };
@@ -63,7 +63,7 @@ static int save_result(void)
 		list_for_each(list, &pmu->format)
 			r->nr_formats++;
 
-		pr_debug("pmu[%d] name=%s, nr_caps=%d, nr_aliases=%d, nr_formats=%d\n",
+		pr_debug("pmu[%d] name=%s, nr_caps=%d, nr_aliases=%zu, nr_formats=%zu\n",
 			nr_pmus, r->name, r->nr_caps, r->nr_aliases, r->nr_formats);
 		nr_pmus++;
 	}
@@ -77,7 +77,7 @@ static int check_result(bool core_only)
 	struct pmu_scan_result *r;
 	struct perf_pmu *pmu;
 	struct list_head *list;
-	int nr;
+	size_t nr;
 
 	for (int i = 0; i < nr_pmus; i++) {
 		r = &results[i];
@@ -98,7 +98,7 @@ static int check_result(bool core_only)
 
 		nr = perf_pmu__num_events(pmu);
 		if (nr != r->nr_aliases) {
-			pr_err("Unmatched number of event aliases in %s: expect %d vs got %d\n",
+			pr_err("Unmatched number of event aliases in %s: expect %zu vs got %zu\n",
 				pmu->name, r->nr_aliases, nr);
 			return -1;
 		}
@@ -107,7 +107,7 @@ static int check_result(bool core_only)
 		list_for_each(list, &pmu->format)
 			nr++;
 		if (nr != r->nr_formats) {
-			pr_err("Unmatched number of event formats in %s: expect %d vs got %d\n",
+			pr_err("Unmatched number of event formats in %s: expect %zu vs got %zu\n",
 				pmu->name, r->nr_formats, nr);
 			return -1;
 		}
diff --git a/tools/perf/bench/sched-messaging.c b/tools/perf/bench/sched-messaging.c
index 93dcd9dba3d0..a2eb47944f44 100644
--- a/tools/perf/bench/sched-messaging.c
+++ b/tools/perf/bench/sched-messaging.c
@@ -102,7 +102,7 @@ static void *sender(struct sender_context *ctx)
 	/* Now pump to every receiver. */
 	for (i = 0; i < nr_loops; i++) {
 		for (j = 0; j < ctx->num_fds; j++) {
-			int ret, done = 0;
+			ssize_t ret, done = 0;
 
 again:
 			ret = write(ctx->out_fds[j], data + done,
@@ -133,7 +133,7 @@ static void *receiver(struct receiver_context* ctx)
 	/* Receive them all */
 	for (i = 0; i < ctx->num_packets; i++) {
 		char data[DATASIZE];
-		int ret, done = 0;
+		ssize_t ret, done = 0;
 
 again:
 		ret = read(ctx->in_fds[0], data + done, DATASIZE - done);
diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
index 70139036d68f..b847213fd616 100644
--- a/tools/perf/bench/sched-pipe.c
+++ b/tools/perf/bench/sched-pipe.c
@@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
 static int enter_cgroup(int nr)
 {
 	char buf[32];
-	int fd, len, ret;
+	int fd;
+	ssize_t ret, len;
 	int saved_errno;
 	struct cgroup *cgrp;
 	pid_t pid;
@@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
 	cgrp = cgrps[nr];
 
 	if (threaded)
-		pid = syscall(__NR_gettid);
+		pid = (pid_t)syscall(__NR_gettid);
 	else
 		pid = getpid();
 
@@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
 
 static inline int read_pipe(struct thread_data *td)
 {
-	int ret, m;
+	ssize_t ret;
+	int m;
 retry:
 	if (nonblocking) {
 		ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
 		if (ret < 0)
-			return ret;
+			return (int)ret;
 	}
 	ret = read(td->pipe_read, &m, sizeof(int));
 	if (nonblocking && ret < 0 && errno == EWOULDBLOCK)
 		goto retry;
-	return ret;
+	return (int)ret;
 }
 
 static void *worker_thread(void *__tdata)
 {
 	struct thread_data *td = __tdata;
-	int i, ret, m = 0;
+	int i, m = 0;
+	ssize_t ret;
 
 	ret = enter_cgroup(td->nr);
 	if (ret < 0) {
diff --git a/tools/perf/bench/sched-seccomp-notify.c b/tools/perf/bench/sched-seccomp-notify.c
index 269c1f4a6852..4f0c68b366e3 100644
--- a/tools/perf/bench/sched-seccomp-notify.c
+++ b/tools/perf/bench/sched-seccomp-notify.c
@@ -43,7 +43,7 @@ static const char * const bench_seccomp_usage[] = {
 
 static int seccomp(unsigned int op, unsigned int flags, void *args)
 {
-	return syscall(__NR_seccomp, op, flags, args);
+	return (int)syscall(__NR_seccomp, op, flags, args);
 }
 
 static int user_notif_syscall(int nr, unsigned int flags)
diff --git a/tools/perf/bench/synthesize.c b/tools/perf/bench/synthesize.c
index 9b333276cbdb..484ba825fcf4 100644
--- a/tools/perf/bench/synthesize.c
+++ b/tools/perf/bench/synthesize.c
@@ -120,7 +120,7 @@ static int run_single_threaded(void)
 	session = perf_session__new(NULL, NULL);
 	if (IS_ERR(session)) {
 		pr_err("Session creation failed.\n");
-		return PTR_ERR(session);
+		return (int)PTR_ERR(session);
 	}
 	threads = thread_map__new_by_pid(getpid());
 	if (!threads) {
@@ -163,7 +163,7 @@ static int do_run_multi_threaded(struct target *target,
 	for (i = 0; i < multi_iterations; i++) {
 		session = perf_session__new(NULL, NULL);
 		if (IS_ERR(session))
-			return PTR_ERR(session);
+			return (int)PTR_ERR(session);
 
 		atomic_set(&event_count, 0);
 		gettimeofday(&start, NULL);
@@ -210,7 +210,7 @@ static int run_multi_threaded(void)
 	int err;
 
 	if (max_threads == UINT_MAX)
-		max_threads = sysconf(_SC_NPROCESSORS_ONLN);
+		max_threads = (int)sysconf(_SC_NPROCESSORS_ONLN);
 
 	puts(
 "Computing performance of multi threaded perf event synthesis by\n"
diff --git a/tools/perf/builtin-bench.c b/tools/perf/builtin-bench.c
index 2c1a9f3d847a..2f6a34908273 100644
--- a/tools/perf/builtin-bench.c
+++ b/tools/perf/builtin-bench.c
@@ -212,7 +212,7 @@ static int bench_str2int(const char *str)
 static int run_bench(const char *coll_name, const char *bench_name, bench_fn_t fn,
 		     int argc, const char **argv)
 {
-	int size;
+	size_t size;
 	char *name;
 	int ret;
 
-- 
2.49.0.906.g1f30a19c02-goog

Hi Ian,

considering so many eyes looking at this, I am probably wrong.

So, this is only a "gauge reply" to see if it's worth I really read
through all the commits ;-)

Ian Rogers <irogers@google.com> writes:

[SNIP]

> diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
> index 70139036d68f..b847213fd616 100644
> --- a/tools/perf/bench/sched-pipe.c
> +++ b/tools/perf/bench/sched-pipe.c
> @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
>  static int enter_cgroup(int nr)
>  {
>  	char buf[32];
> -	int fd, len, ret;
> +	int fd;
> +	ssize_t ret, len;
>  	int saved_errno;
>  	struct cgroup *cgrp;
>  	pid_t pid;
> @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
>  	cgrp = cgrps[nr];
>  
>  	if (threaded)
> -		pid = syscall(__NR_gettid);
> +		pid = (pid_t)syscall(__NR_gettid);
>  	else
>  		pid = getpid();
>  
> @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
>  
>  static inline int read_pipe(struct thread_data *td)
>  {
> -	int ret, m;
> +	ssize_t ret;
> +	int m;
>  retry:
>  	if (nonblocking) {
>  		ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);

The epoll_wait(), I know of, returns an int and not ssize_t.

That shouldn't show up, because it doesn't cause real problems...


Best regards,

Dirk

On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@gouders.net> wrote:
>
> Hi Ian,
>
> considering so many eyes looking at this, I am probably wrong.
>
> So, this is only a "gauge reply" to see if it's worth I really read
> through all the commits ;-)
>
> Ian Rogers <irogers@google.com> writes:
>
> [SNIP]
>
> > diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
> > index 70139036d68f..b847213fd616 100644
> > --- a/tools/perf/bench/sched-pipe.c
> > +++ b/tools/perf/bench/sched-pipe.c
> > @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
> >  static int enter_cgroup(int nr)
> >  {
> >       char buf[32];
> > -     int fd, len, ret;
> > +     int fd;
> > +     ssize_t ret, len;
> >       int saved_errno;
> >       struct cgroup *cgrp;
> >       pid_t pid;
> > @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
> >       cgrp = cgrps[nr];
> >
> >       if (threaded)
> > -             pid = syscall(__NR_gettid);
> > +             pid = (pid_t)syscall(__NR_gettid);
> >       else
> >               pid = getpid();
> >
> > @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
> >
> >  static inline int read_pipe(struct thread_data *td)
> >  {
> > -     int ret, m;
> > +     ssize_t ret;
> > +     int m;
> >  retry:
> >       if (nonblocking) {
> >               ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
>
> The epoll_wait(), I know of, returns an int and not ssize_t.
>
> That shouldn't show up, because it doesn't cause real problems...

So the function is read_pipe so it should probably return a ssize_t. I
stopped short of that but made ret a ssize_t to silence the truncation
warning on the read call. Assigning smaller to bigger is of course not
an issue for epoll_wait.

Thanks,
Ian

> Best regards,
>
> Dirk
>
>

Ian Rogers <irogers@google.com> writes:

> On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@gouders.net> wrote:
>>
>> Hi Ian,
>>
>> considering so many eyes looking at this, I am probably wrong.
>>
>> So, this is only a "gauge reply" to see if it's worth I really read
>> through all the commits ;-)
>>
>> Ian Rogers <irogers@google.com> writes:
>>
>> [SNIP]
>>
>> > diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
>> > index 70139036d68f..b847213fd616 100644
>> > --- a/tools/perf/bench/sched-pipe.c
>> > +++ b/tools/perf/bench/sched-pipe.c
>> > @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
>> >  static int enter_cgroup(int nr)
>> >  {
>> >       char buf[32];
>> > -     int fd, len, ret;
>> > +     int fd;
>> > +     ssize_t ret, len;
>> >       int saved_errno;
>> >       struct cgroup *cgrp;
>> >       pid_t pid;
>> > @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
>> >       cgrp = cgrps[nr];
>> >
>> >       if (threaded)
>> > -             pid = syscall(__NR_gettid);
>> > +             pid = (pid_t)syscall(__NR_gettid);
>> >       else
>> >               pid = getpid();
>> >
>> > @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
>> >
>> >  static inline int read_pipe(struct thread_data *td)
>> >  {
>> > -     int ret, m;
>> > +     ssize_t ret;
>> > +     int m;
>> >  retry:
>> >       if (nonblocking) {
>> >               ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
>>
>> The epoll_wait(), I know of, returns an int and not ssize_t.
>>
>> That shouldn't show up, because it doesn't cause real problems...
>
> So the function is read_pipe so it should probably return a ssize_t. I
> stopped short of that but made ret a ssize_t to silence the truncation
> warning on the read call. Assigning smaller to bigger is of course not
> an issue for epoll_wait.

Oh yes, I missed that ret is also used for the result of read().

Some lines down there is also a combination of

ret = enter_cgroup() (which is int)

and

ret = write()


Just confusing but yes, because ret is also used for read() and write()
in those cases it should be ssize_t.

I'm sorry for the noise.

Best regards,

Dirk

On Wed, Apr 30, 2025 at 3:19 PM Dirk Gouders <dirk@gouders.net> wrote:
>
> Ian Rogers <irogers@google.com> writes:
>
> > On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@gouders.net> wrote:
> >>
> >> Hi Ian,
> >>
> >> considering so many eyes looking at this, I am probably wrong.
> >>
> >> So, this is only a "gauge reply" to see if it's worth I really read
> >> through all the commits ;-)
> >>
> >> Ian Rogers <irogers@google.com> writes:
> >>
> >> [SNIP]
> >>
> >> > diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
> >> > index 70139036d68f..b847213fd616 100644
> >> > --- a/tools/perf/bench/sched-pipe.c
> >> > +++ b/tools/perf/bench/sched-pipe.c
> >> > @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
> >> >  static int enter_cgroup(int nr)
> >> >  {
> >> >       char buf[32];
> >> > -     int fd, len, ret;
> >> > +     int fd;
> >> > +     ssize_t ret, len;
> >> >       int saved_errno;
> >> >       struct cgroup *cgrp;
> >> >       pid_t pid;
> >> > @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
> >> >       cgrp = cgrps[nr];
> >> >
> >> >       if (threaded)
> >> > -             pid = syscall(__NR_gettid);
> >> > +             pid = (pid_t)syscall(__NR_gettid);
> >> >       else
> >> >               pid = getpid();
> >> >
> >> > @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
> >> >
> >> >  static inline int read_pipe(struct thread_data *td)
> >> >  {
> >> > -     int ret, m;
> >> > +     ssize_t ret;
> >> > +     int m;
> >> >  retry:
> >> >       if (nonblocking) {
> >> >               ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
> >>
> >> The epoll_wait(), I know of, returns an int and not ssize_t.
> >>
> >> That shouldn't show up, because it doesn't cause real problems...
> >
> > So the function is read_pipe so it should probably return a ssize_t. I
> > stopped short of that but made ret a ssize_t to silence the truncation
> > warning on the read call. Assigning smaller to bigger is of course not
> > an issue for epoll_wait.
>
> Oh yes, I missed that ret is also used for the result of read().
>
> Some lines down there is also a combination of
>
> ret = enter_cgroup() (which is int)
>
> and
>
> ret = write()
>
>
> Just confusing but yes, because ret is also used for read() and write()
> in those cases it should be ssize_t.
>
> I'm sorry for the noise.

No worries, I'm appreciative of the eyes. I suspect we'll only pick up
the first patches in this series to fix what is a bug on ARM. I think
I'm responsible for too much noise here ;-)

Thanks,
Ian

> Best regards,
>
> Dirk

Ian Rogers <irogers@google.com> writes:

> On Wed, Apr 30, 2025 at 3:19 PM Dirk Gouders <dirk@gouders.net> wrote:
>>
>> Ian Rogers <irogers@google.com> writes:
>>
>> > On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@gouders.net> wrote:
>> >>
>> >> Hi Ian,
>> >>
>> >> considering so many eyes looking at this, I am probably wrong.
>> >>
>> >> So, this is only a "gauge reply" to see if it's worth I really read
>> >> through all the commits ;-)
>> >>
>> >> Ian Rogers <irogers@google.com> writes:
>> >>
>> >> [SNIP]
>> >>
>> >> > diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
>> >> > index 70139036d68f..b847213fd616 100644
>> >> > --- a/tools/perf/bench/sched-pipe.c
>> >> > +++ b/tools/perf/bench/sched-pipe.c
>> >> > @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
>> >> >  static int enter_cgroup(int nr)
>> >> >  {
>> >> >       char buf[32];
>> >> > -     int fd, len, ret;
>> >> > +     int fd;
>> >> > +     ssize_t ret, len;
>> >> >       int saved_errno;
>> >> >       struct cgroup *cgrp;
>> >> >       pid_t pid;
>> >> > @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
>> >> >       cgrp = cgrps[nr];
>> >> >
>> >> >       if (threaded)
>> >> > -             pid = syscall(__NR_gettid);
>> >> > +             pid = (pid_t)syscall(__NR_gettid);
>> >> >       else
>> >> >               pid = getpid();
>> >> >
>> >> > @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
>> >> >
>> >> >  static inline int read_pipe(struct thread_data *td)
>> >> >  {
>> >> > -     int ret, m;
>> >> > +     ssize_t ret;
>> >> > +     int m;
>> >> >  retry:
>> >> >       if (nonblocking) {
>> >> >               ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
>> >>
>> >> The epoll_wait(), I know of, returns an int and not ssize_t.
>> >>
>> >> That shouldn't show up, because it doesn't cause real problems...
>> >
>> > So the function is read_pipe so it should probably return a ssize_t. I
>> > stopped short of that but made ret a ssize_t to silence the truncation
>> > warning on the read call. Assigning smaller to bigger is of course not
>> > an issue for epoll_wait.
>>
>> Oh yes, I missed that ret is also used for the result of read().
>>
>> Some lines down there is also a combination of
>>
>> ret = enter_cgroup() (which is int)
>>
>> and
>>
>> ret = write()
>>
>>
>> Just confusing but yes, because ret is also used for read() and write()
>> in those cases it should be ssize_t.
>>
>> I'm sorry for the noise.
>
> No worries, I'm appreciative of the eyes. I suspect we'll only pick up
> the first patches in this series to fix what is a bug on ARM. I think
> I'm responsible for too much noise here ;-)

A final thought (in case this patch will also be picked):

Why not, in case of read_pipe() and worker_thread() just cast
read() and write() to int?  Both get counts of sizeof(int) and
it would clearly show: we know the result fits into an int.

In case of read_pipe() that would mean to just change one line in
contrast to:

@@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
 
 static inline int read_pipe(struct thread_data *td)
 {
-	int ret, m;
+	ssize_t ret;
+	int m;
 retry:
 	if (nonblocking) {
 		ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
 		if (ret < 0)
-			return ret;
+			return (int)ret;
 	}
 	ret = read(td->pipe_read, &m, sizeof(int));
 	if (nonblocking && ret < 0 && errno == EWOULDBLOCK)
 		goto retry;
-	return ret;
+	return (int)ret;


Best regards,

Dirk

On Thu, 01 May 2025 01:11:16 +0200
Dirk Gouders <dirk@gouders.net> wrote:

> Ian Rogers <irogers@google.com> writes:
> 
> > On Wed, Apr 30, 2025 at 3:19 PM Dirk Gouders <dirk@gouders.net> wrote:  
> >>
> >> Ian Rogers <irogers@google.com> writes:
> >>  
> >> > On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@gouders.net> wrote:  
> >> >>
> >> >> Hi Ian,
> >> >>
> >> >> considering so many eyes looking at this, I am probably wrong.
> >> >>
> >> >> So, this is only a "gauge reply" to see if it's worth I really read
> >> >> through all the commits ;-)
> >> >>
> >> >> Ian Rogers <irogers@google.com> writes:
> >> >>
> >> >> [SNIP]
> >> >>  
> >> >> > diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
> >> >> > index 70139036d68f..b847213fd616 100644
> >> >> > --- a/tools/perf/bench/sched-pipe.c
> >> >> > +++ b/tools/perf/bench/sched-pipe.c
> >> >> > @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
> >> >> >  static int enter_cgroup(int nr)
> >> >> >  {
> >> >> >       char buf[32];
> >> >> > -     int fd, len, ret;
> >> >> > +     int fd;
> >> >> > +     ssize_t ret, len;
> >> >> >       int saved_errno;
> >> >> >       struct cgroup *cgrp;
> >> >> >       pid_t pid;
> >> >> > @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
> >> >> >       cgrp = cgrps[nr];
> >> >> >
> >> >> >       if (threaded)
> >> >> > -             pid = syscall(__NR_gettid);
> >> >> > +             pid = (pid_t)syscall(__NR_gettid);
> >> >> >       else
> >> >> >               pid = getpid();
> >> >> >
> >> >> > @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
> >> >> >
> >> >> >  static inline int read_pipe(struct thread_data *td)
> >> >> >  {
> >> >> > -     int ret, m;
> >> >> > +     ssize_t ret;
> >> >> > +     int m;
> >> >> >  retry:
> >> >> >       if (nonblocking) {
> >> >> >               ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);  
> >> >>
> >> >> The epoll_wait(), I know of, returns an int and not ssize_t.
> >> >>
> >> >> That shouldn't show up, because it doesn't cause real problems...  
> >> >
> >> > So the function is read_pipe so it should probably return a ssize_t. I
> >> > stopped short of that but made ret a ssize_t to silence the truncation
> >> > warning on the read call. Assigning smaller to bigger is of course not
> >> > an issue for epoll_wait.  
> >>
> >> Oh yes, I missed that ret is also used for the result of read().
> >>
> >> Some lines down there is also a combination of
> >>
> >> ret = enter_cgroup() (which is int)
> >>
> >> and
> >>
> >> ret = write()
> >>
> >>
> >> Just confusing but yes, because ret is also used for read() and write()
> >> in those cases it should be ssize_t.
> >>
> >> I'm sorry for the noise.  
> >
> > No worries, I'm appreciative of the eyes. I suspect we'll only pick up
> > the first patches in this series to fix what is a bug on ARM. I think
> > I'm responsible for too much noise here ;-)  
> 
> A final thought (in case this patch will also be picked):
> 
> Why not, in case of read_pipe() and worker_thread() just cast
> read() and write() to int?  Both get counts of sizeof(int) and
> it would clearly show: we know the result fits into an int.

This is an obvious case of the entire insanity of these changes.

	David

> 
> In case of read_pipe() that would mean to just change one line in
> contrast to:
> 
> @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
>  
>  static inline int read_pipe(struct thread_data *td)
>  {
> -	int ret, m;
> +	ssize_t ret;
> +	int m;
>  retry:
>  	if (nonblocking) {
>  		ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
>  		if (ret < 0)
> -			return ret;
> +			return (int)ret;
>  	}
>  	ret = read(td->pipe_read, &m, sizeof(int));
>  	if (nonblocking && ret < 0 && errno == EWOULDBLOCK)
>  		goto retry;
> -	return ret;
> +	return (int)ret;
> 
> 
> Best regards,
> 
> Dirk
> 

David Laight <david.laight.linux@gmail.com> writes:

> On Thu, 01 May 2025 01:11:16 +0200
> Dirk Gouders <dirk@gouders.net> wrote:
>
>> Ian Rogers <irogers@google.com> writes:
>> 
>> > On Wed, Apr 30, 2025 at 3:19 PM Dirk Gouders <dirk@gouders.net> wrote:  
>> >>
>> >> Ian Rogers <irogers@google.com> writes:
>> >>  
>> >> > On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@gouders.net> wrote:  
>> >> >>
>> >> >> Hi Ian,
>> >> >>
>> >> >> considering so many eyes looking at this, I am probably wrong.
>> >> >>
>> >> >> So, this is only a "gauge reply" to see if it's worth I really read
>> >> >> through all the commits ;-)
>> >> >>
>> >> >> Ian Rogers <irogers@google.com> writes:
>> >> >>
>> >> >> [SNIP]
>> >> >>  
>> >> >> > diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
>> >> >> > index 70139036d68f..b847213fd616 100644
>> >> >> > --- a/tools/perf/bench/sched-pipe.c
>> >> >> > +++ b/tools/perf/bench/sched-pipe.c
>> >> >> > @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
>> >> >> >  static int enter_cgroup(int nr)
>> >> >> >  {
>> >> >> >       char buf[32];
>> >> >> > -     int fd, len, ret;
>> >> >> > +     int fd;
>> >> >> > +     ssize_t ret, len;
>> >> >> >       int saved_errno;
>> >> >> >       struct cgroup *cgrp;
>> >> >> >       pid_t pid;
>> >> >> > @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
>> >> >> >       cgrp = cgrps[nr];
>> >> >> >
>> >> >> >       if (threaded)
>> >> >> > -             pid = syscall(__NR_gettid);
>> >> >> > +             pid = (pid_t)syscall(__NR_gettid);
>> >> >> >       else
>> >> >> >               pid = getpid();
>> >> >> >
>> >> >> > @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
>> >> >> >
>> >> >> >  static inline int read_pipe(struct thread_data *td)
>> >> >> >  {
>> >> >> > -     int ret, m;
>> >> >> > +     ssize_t ret;
>> >> >> > +     int m;
>> >> >> >  retry:
>> >> >> >       if (nonblocking) {
>> >> >> >               ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);  
>> >> >>
>> >> >> The epoll_wait(), I know of, returns an int and not ssize_t.
>> >> >>
>> >> >> That shouldn't show up, because it doesn't cause real problems...  
>> >> >
>> >> > So the function is read_pipe so it should probably return a ssize_t. I
>> >> > stopped short of that but made ret a ssize_t to silence the truncation
>> >> > warning on the read call. Assigning smaller to bigger is of course not
>> >> > an issue for epoll_wait.  
>> >>
>> >> Oh yes, I missed that ret is also used for the result of read().
>> >>
>> >> Some lines down there is also a combination of
>> >>
>> >> ret = enter_cgroup() (which is int)
>> >>
>> >> and
>> >>
>> >> ret = write()
>> >>
>> >>
>> >> Just confusing but yes, because ret is also used for read() and write()
>> >> in those cases it should be ssize_t.
>> >>
>> >> I'm sorry for the noise.  
>> >
>> > No worries, I'm appreciative of the eyes. I suspect we'll only pick up
>> > the first patches in this series to fix what is a bug on ARM. I think
>> > I'm responsible for too much noise here ;-)  
>> 
>> A final thought (in case this patch will also be picked):
>> 
>> Why not, in case of read_pipe() and worker_thread() just cast
>> read() and write() to int?  Both get counts of sizeof(int) and
>> it would clearly show: we know the result fits into an int.
>
> This is an obvious case of the entire insanity of these changes.

You mean, because there is still the -1 case where the sign-lost can
happen?

I guess your reply is in combination with your replies to another thread
to this subject.  As far as I understood, Ian also has problems with
full understanding and I wonder if it helps to talk about a real
example.  As far as I understood you say that code like this
(from tools/perf/bench/sched-pipe.c) is simply wrong:

static inline int read_pipe(struct thread_data *td)
{
	int ret, m;
retry:
	if (nonblocking) {
		ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
		if (ret < 0)
			return ret;
	}
	ret = read(td->pipe_read, &m, sizeof(int));
	if (nonblocking && ret < 0 && errno == EWOULDBLOCK)
		goto retry;
	return ret;
}

And from your reply I understand that casting the read() explicitely to
int is insane.  And now, I wonder what you would suggest -- honestly, I
am expecting to learn something, here.

Best regards,

Dirk

On Fri, 02 May 2025 16:12:17 +0200
Dirk Gouders <dirk@gouders.net> wrote:

> David Laight <david.laight.linux@gmail.com> writes:
> 
> > On Thu, 01 May 2025 01:11:16 +0200
> > Dirk Gouders <dirk@gouders.net> wrote:
> >  
> >> Ian Rogers <irogers@google.com> writes:
> >>   
> >> > On Wed, Apr 30, 2025 at 3:19 PM Dirk Gouders <dirk@gouders.net> wrote:    
> >> >>
> >> >> Ian Rogers <irogers@google.com> writes:
> >> >>    
> >> >> > On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@gouders.net> wrote:    
> >> >> >>
> >> >> >> Hi Ian,
> >> >> >>
> >> >> >> considering so many eyes looking at this, I am probably wrong.
> >> >> >>
> >> >> >> So, this is only a "gauge reply" to see if it's worth I really read
> >> >> >> through all the commits ;-)
> >> >> >>
> >> >> >> Ian Rogers <irogers@google.com> writes:
> >> >> >>
> >> >> >> [SNIP]
> >> >> >>    
> >> >> >> > diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
> >> >> >> > index 70139036d68f..b847213fd616 100644
> >> >> >> > --- a/tools/perf/bench/sched-pipe.c
> >> >> >> > +++ b/tools/perf/bench/sched-pipe.c
> >> >> >> > @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
> >> >> >> >  static int enter_cgroup(int nr)
> >> >> >> >  {
> >> >> >> >       char buf[32];
> >> >> >> > -     int fd, len, ret;
> >> >> >> > +     int fd;
> >> >> >> > +     ssize_t ret, len;
> >> >> >> >       int saved_errno;
> >> >> >> >       struct cgroup *cgrp;
> >> >> >> >       pid_t pid;
> >> >> >> > @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
> >> >> >> >       cgrp = cgrps[nr];
> >> >> >> >
> >> >> >> >       if (threaded)
> >> >> >> > -             pid = syscall(__NR_gettid);
> >> >> >> > +             pid = (pid_t)syscall(__NR_gettid);
> >> >> >> >       else
> >> >> >> >               pid = getpid();
> >> >> >> >
> >> >> >> > @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
> >> >> >> >
> >> >> >> >  static inline int read_pipe(struct thread_data *td)
> >> >> >> >  {
> >> >> >> > -     int ret, m;
> >> >> >> > +     ssize_t ret;
> >> >> >> > +     int m;
> >> >> >> >  retry:
> >> >> >> >       if (nonblocking) {
> >> >> >> >               ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);    
> >> >> >>
> >> >> >> The epoll_wait(), I know of, returns an int and not ssize_t.
> >> >> >>
> >> >> >> That shouldn't show up, because it doesn't cause real problems...    
> >> >> >
> >> >> > So the function is read_pipe so it should probably return a ssize_t. I
> >> >> > stopped short of that but made ret a ssize_t to silence the truncation
> >> >> > warning on the read call. Assigning smaller to bigger is of course not
> >> >> > an issue for epoll_wait.    
> >> >>
> >> >> Oh yes, I missed that ret is also used for the result of read().
> >> >>
> >> >> Some lines down there is also a combination of
> >> >>
> >> >> ret = enter_cgroup() (which is int)
> >> >>
> >> >> and
> >> >>
> >> >> ret = write()
> >> >>
> >> >>
> >> >> Just confusing but yes, because ret is also used for read() and write()
> >> >> in those cases it should be ssize_t.
> >> >>
> >> >> I'm sorry for the noise.    
> >> >
> >> > No worries, I'm appreciative of the eyes. I suspect we'll only pick up
> >> > the first patches in this series to fix what is a bug on ARM. I think
> >> > I'm responsible for too much noise here ;-)    
> >> 
> >> A final thought (in case this patch will also be picked):
> >> 
> >> Why not, in case of read_pipe() and worker_thread() just cast
> >> read() and write() to int?  Both get counts of sizeof(int) and
> >> it would clearly show: we know the result fits into an int.  
> >
> > This is an obvious case of the entire insanity of these changes.  
> 
> You mean, because there is still the -1 case where the sign-lost can
> happen?
> 
> I guess your reply is in combination with your replies to another thread
> to this subject.  As far as I understood, Ian also has problems with
> full understanding and I wonder if it helps to talk about a real
> example.  As far as I understood you say that code like this
> (from tools/perf/bench/sched-pipe.c) is simply wrong:
> 
> static inline int read_pipe(struct thread_data *td)
> {
> 	int ret, m;
> retry:
> 	if (nonblocking) {
> 		ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
> 		if (ret < 0)
> 			return ret;
> 	}
> 	ret = read(td->pipe_read, &m, sizeof(int));
> 	if (nonblocking && ret < 0 && errno == EWOULDBLOCK)
> 		goto retry;
> 	return ret;
> }
> 
> And from your reply I understand that casting the read() explicitely to
> int is insane.  And now, I wonder what you would suggest -- honestly, I
> am expecting to learn something, here.

If you look through pretty much all 'posix' userspace code the return
value from 'read' is assigned to an 'int' variable.
If the compiler is going to complain that the return value doesn't fit
into a 32bit int, it better have a pretty good idea the return value
might exceed 2^^32.
That requires knowledge of what 'read' does and analysis of the domain
(not just type) of the length passed to read.

Now if you add an (int) cast, you won't get an error (on 32bit) if
the value is a pointer - and that is an error you always want.

I'm pretty sure that it is also true the linux limits read (and write)
to INT_MAX - so, for linux, the return value from read() always fits
int 'int'.

The underlying problem is that if you start adding unnecessary casts for
integer type conversions you end up with so many casts that it is far too
easy for a 'broken' one to slip into the code.

If you scan the kernel for min_t() there are plenty of very dubious ones.
They've been added to 'fix' a compile time warning, but there are plenty
that cast to u8, u16 or long (where there are u64 lurking).
One of the u16 ones I found was a real bug and found/fixed separately
from my scans of all the min_t().

	David 

> 
> Best regards,
> 
> Dirk

David Laight <david.laight.linux@gmail.com> writes:

> On Fri, 02 May 2025 16:12:17 +0200
> Dirk Gouders <dirk@gouders.net> wrote:
>
>> David Laight <david.laight.linux@gmail.com> writes:
>> 
>> > On Thu, 01 May 2025 01:11:16 +0200
>> > Dirk Gouders <dirk@gouders.net> wrote:
>> >  
>> >> Ian Rogers <irogers@google.com> writes:
>> >>   
>> >> > On Wed, Apr 30, 2025 at 3:19 PM Dirk Gouders <dirk@gouders.net> wrote:    
>> >> >>
>> >> >> Ian Rogers <irogers@google.com> writes:
>> >> >>    
>> >> >> > On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@gouders.net> wrote:    
>> >> >> >>
>> >> >> >> Hi Ian,
>> >> >> >>
>> >> >> >> considering so many eyes looking at this, I am probably wrong.
>> >> >> >>
>> >> >> >> So, this is only a "gauge reply" to see if it's worth I really read
>> >> >> >> through all the commits ;-)
>> >> >> >>
>> >> >> >> Ian Rogers <irogers@google.com> writes:
>> >> >> >>
>> >> >> >> [SNIP]
>> >> >> >>    
>> >> >> >> > diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
>> >> >> >> > index 70139036d68f..b847213fd616 100644
>> >> >> >> > --- a/tools/perf/bench/sched-pipe.c
>> >> >> >> > +++ b/tools/perf/bench/sched-pipe.c
>> >> >> >> > @@ -102,7 +102,8 @@ static const char * const bench_sched_pipe_usage[] = {
>> >> >> >> >  static int enter_cgroup(int nr)
>> >> >> >> >  {
>> >> >> >> >       char buf[32];
>> >> >> >> > -     int fd, len, ret;
>> >> >> >> > +     int fd;
>> >> >> >> > +     ssize_t ret, len;
>> >> >> >> >       int saved_errno;
>> >> >> >> >       struct cgroup *cgrp;
>> >> >> >> >       pid_t pid;
>> >> >> >> > @@ -118,7 +119,7 @@ static int enter_cgroup(int nr)
>> >> >> >> >       cgrp = cgrps[nr];
>> >> >> >> >
>> >> >> >> >       if (threaded)
>> >> >> >> > -             pid = syscall(__NR_gettid);
>> >> >> >> > +             pid = (pid_t)syscall(__NR_gettid);
>> >> >> >> >       else
>> >> >> >> >               pid = getpid();
>> >> >> >> >
>> >> >> >> > @@ -172,23 +173,25 @@ static void exit_cgroup(int nr)
>> >> >> >> >
>> >> >> >> >  static inline int read_pipe(struct thread_data *td)
>> >> >> >> >  {
>> >> >> >> > -     int ret, m;
>> >> >> >> > +     ssize_t ret;
>> >> >> >> > +     int m;
>> >> >> >> >  retry:
>> >> >> >> >       if (nonblocking) {
>> >> >> >> >               ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);    
>> >> >> >>
>> >> >> >> The epoll_wait(), I know of, returns an int and not ssize_t.
>> >> >> >>
>> >> >> >> That shouldn't show up, because it doesn't cause real problems...    
>> >> >> >
>> >> >> > So the function is read_pipe so it should probably return a ssize_t. I
>> >> >> > stopped short of that but made ret a ssize_t to silence the truncation
>> >> >> > warning on the read call. Assigning smaller to bigger is of course not
>> >> >> > an issue for epoll_wait.    
>> >> >>
>> >> >> Oh yes, I missed that ret is also used for the result of read().
>> >> >>
>> >> >> Some lines down there is also a combination of
>> >> >>
>> >> >> ret = enter_cgroup() (which is int)
>> >> >>
>> >> >> and
>> >> >>
>> >> >> ret = write()
>> >> >>
>> >> >>
>> >> >> Just confusing but yes, because ret is also used for read() and write()
>> >> >> in those cases it should be ssize_t.
>> >> >>
>> >> >> I'm sorry for the noise.    
>> >> >
>> >> > No worries, I'm appreciative of the eyes. I suspect we'll only pick up
>> >> > the first patches in this series to fix what is a bug on ARM. I think
>> >> > I'm responsible for too much noise here ;-)    
>> >> 
>> >> A final thought (in case this patch will also be picked):
>> >> 
>> >> Why not, in case of read_pipe() and worker_thread() just cast
>> >> read() and write() to int?  Both get counts of sizeof(int) and
>> >> it would clearly show: we know the result fits into an int.  
>> >
>> > This is an obvious case of the entire insanity of these changes.  
>> 
>> You mean, because there is still the -1 case where the sign-lost can
>> happen?
>> 
>> I guess your reply is in combination with your replies to another thread
>> to this subject.  As far as I understood, Ian also has problems with
>> full understanding and I wonder if it helps to talk about a real
>> example.  As far as I understood you say that code like this
>> (from tools/perf/bench/sched-pipe.c) is simply wrong:
>> 
>> static inline int read_pipe(struct thread_data *td)
>> {
>> 	int ret, m;
>> retry:
>> 	if (nonblocking) {
>> 		ret = epoll_wait(td->epoll_fd, &td->epoll_ev, 1, -1);
>> 		if (ret < 0)
>> 			return ret;
>> 	}
>> 	ret = read(td->pipe_read, &m, sizeof(int));
>> 	if (nonblocking && ret < 0 && errno == EWOULDBLOCK)
>> 		goto retry;
>> 	return ret;
>> }
>> 
>> And from your reply I understand that casting the read() explicitely to
>> int is insane.  And now, I wonder what you would suggest -- honestly, I
>> am expecting to learn something, here.

First, thank you for elaborating on this.  As I expected, I indeed learn
more than one thing.

> If you look through pretty much all 'posix' userspace code the return
> value from 'read' is assigned to an 'int' variable.

I looked at some read()s in util-linux and all those that I looked at
use ssize_t.  Two reads, I found in bash use int.  In mpich, both
versions are used...  I didn't see a single cast, though ;-)

> If the compiler is going to complain that the return value doesn't fit
> into a 32bit int, it better have a pretty good idea the return value
> might exceed 2^^32.
> That requires knowledge of what 'read' does and analysis of the domain
> (not just type) of the length passed to read.
> Now if you add an (int) cast, you won't get an error (on 32bit) if
> the value is a pointer - and that is an error you always want.

You mean something like:

char *ptr = (int)read(fd, buf, sizeof(buf));

Here in my environment, I'd get an error:

error: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]

(I have no 32bit system to test it but from my memory, I'd say I know
 such error from times when 64bit wasn't available...)

> I'm pretty sure that it is also true the linux limits read (and write)
> to INT_MAX - so, for linux, the return value from read() always fits
> int 'int'.

Yes, didn't know that.  From read(2):

------------------------------------------------------------------------
NOTES
       On Linux, read() (and similar system calls) will transfer at most
       0x7ffff000 (2,147,479,552) bytes, returning the number of bytes
       actually transferred.  (This is true on both 32-bit and 64-bit systems.)
------------------------------------------------------------------------

Oh well, I myself took `ssize_t read()' always so serious that I gave my
best to always try to match that type...

> The underlying problem is that if you start adding unnecessary casts for
> integer type conversions you end up with so many casts that it is far too
> easy for a 'broken' one to slip into the code.

OK, in the other thread, you also said that, in your opinion, (just
integer?) casts should be kept to an absolute minimum and I wonder, what
would be an example for such (mandatory) cases.  Just the ones where the
compiler would complain (except for -Wshorten-64-to-32)?

> If you scan the kernel for min_t() there are plenty of very dubious ones.
> They've been added to 'fix' a compile time warning, but there are plenty
> that cast to u8, u16 or long (where there are u64 lurking).
> One of the u16 ones I found was a real bug and found/fixed separately
> from my scans of all the min_t().

Sorry for me still failing to fully understand: do your concerns then
mean you'd vote for not enabling -Wshorten-64-to-32 and live with the
perhaps rare cases of problems like the one in

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

or identify them by other means?

Best regards,

Dirk