Re: -Wswitch Clang warnings in drivers/scsi

[Nick Desaulniers]

On Thu, Oct 4, 2018 at 11:45 AM Nathan Chancellor
<natechancellor@xxxxxxxxx> wrote:
>
> On Thu, Oct 04, 2018 at 11:34:29AM -0700, Bart Van Assche wrote:
> > On Thu, 2018-10-04 at 11:30 -0700, Nathan Chancellor wrote:
> > > Hi SCSI folks,
> > >
> > > In an effort to get the kernel building warning free with Clang, we've
> > > come across an interesting occurrence in a few scsi drivers:
> > >
> > > drivers/scsi/hpsa.c:6533:7: warning: overflow converting case value to switch condition type (2148024833 to 18446744071562609153) [-Wswitch]
> > >         case CCISS_GETPCIINFO:
> > >              ^
> > > ./include/uapi/linux/cciss_ioctl.h:65:26: note: expanded from macro 'CCISS_GETPCIINFO'
> > > #define CCISS_GETPCIINFO _IOR(CCISS_IOC_MAGIC, 1, cciss_pci_info_struct)
> > >                          ^
> > > ./include/uapi/asm-generic/ioctl.h:86:28: note: expanded from macro '_IOR'
> > > #define _IOR(type,nr,size)      _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
> > >                                 ^
> > > ./include/uapi/asm-generic/ioctl.h:70:2: note: expanded from macro '_IOC'
> > >         (((dir)  << _IOC_DIRSHIFT) | \
> > >         ^
> > >
> > > I see this warning in drivers/scsi/hpsa.c and drivers/scsi/smartpqi/smartpqi_init.c
> > > on an arm64 allyesconfig build and it has also been reported in a couple of files in
> > > drivers/scsi/cxlflash.
> > >
> > > As the warning states, there is an overflow because the switch statement's value is of
> > > type int but the switch value is greater than INT_MAX. I did a brief sweep of the tree
> > > and it seems that all uses of _IOC in switch statement values either are small enough
> > > to fit into size int or the value is of size unsigned int.
> > >
> > > I am unsure of the implications of using a smaller _IOC value or converting all ioctls
> > > to expect a cmd of type unsigned int (especially since that has userspace implications)
> > > but I didn't see any negative ioctl commands. Some clarity and insight would be
> > > appreciated.
> >
> > Have you verified how gcc compiles these switch statements? Maybe gcc supports
> > switch / case statements on integral types that are larger than an int?

GCC just doesn't warn when the case expression is greater than the
maximal representable value and thus would wrap (or appears to, this
is probably undefined behavior).  Using an unsigned int here is no
functional change:
https://godbolt.org/z/1IyYV4

GCC and Clang do effectively the same thing as each other, and in the
cases of switching on an unsigned int vs signed int.

> >
> > Bart.
>
> Hi Bart,
>
> That is entirely possible, I will do some research. I did build with GCC
> to see if there was any warning and there isn't so I'll be curious to
> see what is happening at a lower level.
>
> Thanks for the comment!
> Nathan



-- 
Thanks,
~Nick Desaulniers