On Tue, May 28, 2024 at 2:03 PM Rasmus Villemoes
<linux@xxxxxxxxxxxxxxxxxx> wrote:
>
> On 28/05/2024 12.37, Mateusz Guzik wrote:
> > I misplaced a closing ) in a patch using unlikely() and broke the
> > comparison, see [1] for context.
> >
> > The fix was:
> > - if (unlikely(abs(count + amount)) >= batch) {
> > + if (unlikely(abs(count + amount) >= batch)) {
> >
> > Neither kernel build with W=1 nor C=1 (smatch) report the problem.
> >
> > Given that this compiles fine and for many cases with the bogus usage
> > it also happens to produce the correct result, I have to suspect there
> > are spots in the kernel with the problem.
> >
> > Either way, sounds like something smatch should be testing for.
>
> scripts/checkpatch.pl:# likely/unlikely comparisons similar to
> "(likely(foo) > 0)"
>
> So checkpatch should already have such a check, but running it on those
> two files doesn't catch the instances; playing around a bit more
> suggests that it only works when the argument to (un)likely has no
> whitespace - so essentially, consists of a single identifier as in the
> comment.
>
Oh heh, I did not think to run checkpatch after all the above. I
confirm it did spot the problem in this particular patch.
Even so I would argue catching this in smatch is warranted -- my
expectation is that once the patch is validated with the above, at
worst some cosmetic or otherwise non-semantics changes can be reported
by checkpatch. Would be pretty weird to ask people to run several
tools for linting as they iterate on a patch.
--
Mateusz Guzik <mjguzik gmail.com>
