Re: [PATCH v2 04/47] perf bench: Silence -Wshorten-64-to-32 warnings

[David Laight <david.laight.linux@gmail.com>]

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
>