Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type()

[Bart Van Assche]

On 8/21/22 11:35, Linus Torvalds wrote:
> If we want to just shut up the sparse warning, then afaik the simple
> one-liner fix would have been
>
> -#define is_signed_type(type)   (((type)(-1)) < (type)1)
> +#define is_signed_type(type)   (((__force type)(-1)) < (__force type)1)
>
> and at least then sparse checks the same source as is compiled,
> instead of passing a "this is not a signed type" to places.

Hi Linus,

I agree that it's better that sparse sees the same code as what is used to
build the kernel. However, I do not agree that the patch above is a solution.
Sparse reports a warning for the suggested definition above of is_signed_type()
because the new definition tries to use the less-than (<) operator to compare
two __bitwise types.

> Btw, that patch is entirely broken for *another* reason.
>
> Even if you were to say "ok, sparse just gets a different argument",
> the fact that the trace_events file re-defined that is_signed_type()
> macro means that you added that
>
> +#undef is_signed_type
>
> to make the compiler happy about how you only modified one of them.
>
> But that then means that if <linux/trace_events.h> gets included
> *before* <linux/overflow.h>, you'll just get the warning *there*
> instead.
>
> Now, that warning would only happen for a __CHECKER__ build - but
> that's the only build this patch is relevant for anyway.
>
> And maybe that ordering doesn't exist, or maybe it only exists on some
> very random config. Regardless, it's broken.
>
> Of course, the real fix should be to just not re-define that macro at
> all, and just have it in *one* place.

Agreed that is_signed_type() should only be defined once. If nobody else
beats me to this I will prepare a patch to fix this.

Thanks,

Bart.