Messages in this thread | | | Date | Mon, 22 Aug 2022 11:20:03 -0700 | Subject | Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type() | From | 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.
| |