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

[Dirk Gouders]

David Laight <david.laight.linux@xxxxxxxxx> writes:

> On Thu, 01 May 2025 01:11:16 +0200
> Dirk Gouders <dirk@xxxxxxxxxxx> wrote:
>
>> Ian Rogers <irogers@xxxxxxxxxx> writes:
>> 
>> > On Wed, Apr 30, 2025 at 3:19 PM Dirk Gouders <dirk@xxxxxxxxxxx> wrote:  
>> >>
>> >> Ian Rogers <irogers@xxxxxxxxxx> writes:
>> >>  
>> >> > On Wed, Apr 30, 2025 at 1:23 PM Dirk Gouders <dirk@xxxxxxxxxxx> 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@xxxxxxxxxx> 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