Quoting full email for Matthew and Zhengbin to have context. On Sat 21-09-19 14:10:52, Linus Torvalds wrote: > On Fri, Sep 20, 2019 at 4:00 AM Jan Kara <jack@xxxxxxx> wrote: > > > > could you please pull from > > Pulled and then unpulled. > > This is a prime example of a "cleanup" that should never ever be done, > and a compiler warning that is a disgrace and shouldn't happen. > > This code: > > WARN_ON_ONCE(len < 0 || len >= FANOTIFY_EVENT_ALIGN); > > is obvious and makes sense. It clearly and unambiguously checks that > 'len' is in the specified range. > > In contrast, this code: > > WARN_ON_ONCE(len >= FANOTIFY_EVENT_ALIGN); > > quite naturally will make a human wonder "what about negative values". > > Yes, it turns out that "len" is unsigned. That isn't actually > immediately obvious to a human, since the declaration of 'len' is 20+ > lines earlier (and even then the type doesn't say "unsigned", although > a lot of people do recognize "size_t" as such). > > In fact, maybe some day the type will change, and the careful range > checking means that the code continues to work correctly. Yeah, I was also a bit undecided about this patch because the check with "len < 0" seems more obvious. But then decided to take it because we have a very similar WARN_ON_ONCE() at the beginning of the function (copy_fid_to_user()) making sure "len" is large enough. But seeing your arguments I'll just drop the patch. Thanks for review! > The fact that "len" is unsigned _is_ obvious to the compiler, which > just means that now that compiler can ignore the "< 0" thing and > optimize it away. Great. > > But that doesn't make the compiler warning valid, and it doesn't make > the patch any better. > > When it comes to actual code quality, the version that checks against > zero is the better version. > > Please stop using -Wtype-limits with compilers that are too stupid to > understand that range checking with the type range is sane. > > Compilers that think that warning for the above kind of thing is ok > are inexcusable garbage. > > And compiler writers who think that the warning is a good thing can't > see the forest for the trees. They are too hung up on a detail to see > the big picture. > > Why/how was this found in the first place? We don't enable type-limit > checking by default. The report has come from a CI system run at Huawei. Not sure what exactly they run there. > We may have to add an explicit > > ccflags-y += $(call cc-disable-warning, type-limits) > > if these kinds of patches continue to happen, which would be sad. > There are _valid_ type limits. > > But this kind of range-based check is not a valid thing to warn about, > and we shouldn't make the kernel source code worse just because the > compiler is doing garbage things. > > Linus Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
