On 28/05/2024 14.21, Mateusz Guzik wrote:
> On Tue, May 28, 2024 at 2:03 PM Rasmus Villemoes
> <linux@xxxxxxxxxxxxxxxxxx> wrote:
>>
>> 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.
Ah, ok, so it's not whitespace in the argument itself that's the
problem, but it does seem that the argument must consist of just one "atom":
void bar(void) {
// caught
if (likely(foo(x + y)) > 0)
bugger();
// not caught
if (likely(foo(x + y) + z) > 0)
bugger();
// not caught
if (likely(x + y) > 0)
bugger();
}
The first happens to match your case, but without the abs() it wouldn't
have. Perhaps Joe can figure out if there's a simple way to make the
check match the two latter cases (and the actual in-tree cases) as well.
> Even so I would argue catching this in smatch is warranted
It's probably doable, at least wrt. the > >= < <= operators. Something
like a generic check for when one of the operands to such an operator is
itself a result of an operator that has "boolean" output (i.e. unary !
and binary > >= < <= != ==), and then, if it doesn't already, lift the
the argument to __builtin_expect(), which will always be of the !!()
kind. ISTR that there is already some handling of at least the first
part, but maybe that was coccinelle rather than smatch.
Actually, perhaps coccinelle would be a simpler place to implement a
check like this.
Rasmus
