On Mon, Oct 24, 2022 at 12:30:11PM +0300, Dan Carpenter wrote: > On Mon, Oct 24, 2022 at 12:24:24PM +0300, Dan Carpenter wrote: > > On Wed, Oct 19, 2022 at 02:30:34PM -0600, Jason A. Donenfeld wrote: > > > Recently, some compile-time checking I added to the clamp_t family of > > > functions triggered a build error when a poorly written driver was > > > compiled on ARM, because the driver assumed that the naked `char` type > > > is signed, but ARM treats it as unsigned, and the C standard says it's > > > architecture-dependent. > > > > > > I doubt this particular driver is the only instance in which > > > unsuspecting authors make assumptions about `char` with no `signed` or > > > `unsigned` specifier. We were lucky enough this time that that driver > > > used `clamp_t(char, negative_value, positive_value)`, so the new > > > checking code found it, and I've sent a patch to fix it, but there are > > > likely other places lurking that won't be so easily unearthed. > > > > > > So let's just eliminate this particular variety of heisensign bugs > > > entirely. Set `-funsigned-char` globally, so that gcc makes the type > > > unsigned on all architectures. > > > > > > This will break things in some places and fix things in others, so this > > > will likely cause a bit of churn while reconciling the type misuse. > > > > > > > This is a very daring change and obviously is going to introduce bugs. > > It might be better to create a static checker rule that says "char" > > without explicit signedness can only be used for strings. > > > > arch/parisc/kernel/drivers.c:337 print_hwpath() warn: impossible condition '(path->bc[i] == -1) => (0-255 == (-1))' > > arch/parisc/kernel/drivers.c:410 setup_bus_id() warn: impossible condition '(path.bc[i] == -1) => (0-255 == (-1))' > > arch/parisc/kernel/drivers.c:486 create_parisc_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))' > > arch/parisc/kernel/drivers.c:759 hwpath_to_device() warn: impossible condition '(modpath->bc[i] == -1) => (0-255 == (-1))' > > drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: assigning (-9) to unsigned variable 'tm' > > drivers/media/dvb-frontends/stv0288.c:471 stv0288_set_frontend() warn: we never enter this loop > > drivers/misc/sgi-gru/grumain.c:711 gru_check_chiplet_assignment() warn: 'gts->ts_user_chiplet_id' is unsigned > > drivers/net/wireless/cisco/airo.c:5316 proc_wepkey_on_close() warn: assigning (-16) to unsigned variable 'key[i / 3]' > > drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9415 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'idx0' > > drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9470 rt2800_iq_search() warn: assigning (-32) to unsigned variable 'perr' > > drivers/video/fbdev/sis/init301.c:3549 SiS_GetCRT2Data301() warn: 'SiS_Pr->SiS_EModeIDTable[ModeIdIndex]->ROMMODEIDX661' is unsigned > > sound/pci/au88x0/au88x0_core.c:2029 vortex_adb_checkinout() warn: signedness bug returning '(-22)' > > sound/pci/au88x0/au88x0_core.c:2046 vortex_adb_checkinout() warn: signedness bug returning '(-12)' > > sound/pci/au88x0/au88x0_core.c:2125 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, (0), en, 0)' is unsigned > > sound/pci/au88x0/au88x0_core.c:2170 vortex_adb_allocroute() warn: 'vortex_adb_checkinout(vortex, stream->resources, en, 4)' is unsigned > > sound/pci/rme9652/hdsp.c:3953 hdsp_channel_buffer_location() warn: 'hdsp->channel_map[channel]' is unsigned > > sound/pci/rme9652/rme9652.c:1833 rme9652_channel_buffer_location() warn: 'rme9652->channel_map[channel]' is unsigned > > Here are some more: > > drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9472 rt2800_iq_search() warn: impossible condition '(gerr < -7) => (0-255 < (-7))' > drivers/net/wireless/ralink/rt2x00/rt2800lib.c:9476 rt2800_iq_search() warn: impossible condition '(perr < -31) => (0-255 < (-31))' > drivers/staging/rtl8192e/rtllib_softmac_wx.c:459 rtllib_wx_set_essid() warn: impossible condition '(extra[i] < 0) => (0-255 < 0)' > sound/pci/rme9652/hdsp.c:4153 snd_hdsp_channel_info() warn: impossible condition '(hdsp->channel_map[channel] < 0) => (0-255 < 0)' > > This might be interesting for backports if everyone starts to rely on > the fact that char is unsigned as the PPC people currently do. Give these a minute to hit Lore, but patches just submitted to various maintainers as fixes (for 6.1), since these are already broken on some architecture. https://lore.kernel.org/all/20221024163005.536097-1-Jason@xxxxxxxxx https://lore.kernel.org/all/20221024162947.536060-1-Jason@xxxxxxxxx https://lore.kernel.org/all/20221024162929.536004-1-Jason@xxxxxxxxx https://lore.kernel.org/all/20221024162901.535972-1-Jason@xxxxxxxxx https://lore.kernel.org/all/20221024162843.535921-1-Jason@xxxxxxxxx https://lore.kernel.org/all/20221024162823.535884-1-Jason@xxxxxxxxx https://lore.kernel.org/all/20221024162756.535776-1-Jason@xxxxxxxxx Jason
