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

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

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