On Fri, Jun 30, 2023 at 12:28 PM Nathan Chancellor <nathan@xxxxxxxxxx> wrote: > > On Fri, Jun 30, 2023 at 09:14:42AM -0700, Jakub Kicinski wrote: > > On Fri, 30 Jun 2023 09:10:43 -0700 Nathan Chancellor wrote: > > > > Let me CC llvm@ in case someone's there is willing to make > > > > the compiler warn about this. > > > > > > Turns out clang already has a warning for this, -Wcomma: > > > > > > drivers/nvme/host/tcp.c:1017:38: error: possible misuse of comma operator here [-Werror,-Wcomma] > > > 1017 | msg.msg_flags &= ~MSG_SPLICE_PAGES, > > > | ^ > > > drivers/nvme/host/tcp.c:1017:4: note: cast expression to void to silence warning > > > 1017 | msg.msg_flags &= ~MSG_SPLICE_PAGES, > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > | (void)( ) > > > 1 error generated. > > > > > > Let me do some wider build testing to see if it is viable to turn this > > > on for the whole kernel because it seems worth it, at least in this > > > case. There are a lot of cases where a warning won't be emitted (see the > > > original upstream review for a list: https://reviews.llvm.org/D3976) but > > > something is better than nothing, right? :) > > Well, that was a pipe dream :/ In ARCH=arm multi_v7_defconfig alone, > there are 289 unique instances of the warning (although a good number > have multiple instances per line, so it is not quite as bad as it seems, > but still bad): > > $ rg -- -Wcomma arm-multi_v7_defconfig.log | sort | uniq -c | wc -l > 289 > > https://gist.github.com/nathanchance/907867e0a7adffc877fd39fd08853801 It's definitely interesting to take a look at some of these cases. Some are pretty funny IMO. > > Probably not a good sign of the signal to noise ratio, I looked through > a good handful and all the cases I saw were not interesting... Perhaps > the warning could be tuned further to become useful for the kernel but > in its current form, it is definitely a no-go :/ > > > Ah, neat. Misleading indentation is another possible angle, I reckon, > > but not sure if that's enabled/possible to enable for the entire kernel > > Yeah, I was surprised there was no warning for misleading indentation... > it is a part of -Wall for both clang and GCC, so it is on for the > kernel, it just appears not to trigger in this case. > > > either :( We test-build with W=1 in networking, FWIW, so W=1 would be > > enough for us. > > Unfortunately, even in its current form, it is way too noisy for W=1, as > the qualifier for W=1 is "do not occur too often". Probably could be > placed under W=2 but it still has the problem of wading through every > instance and it is basically a no-op because nobody tests with W=2. > > Cheers, > Nathan > -- Thanks, ~Nick Desaulniers
