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

[Steven Rostedt]

On Sun, 21 Aug 2022 11:35:29 -0700
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Sat, Aug 20, 2022 at 5:08 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> >
> > Since there is no known way of checking signedness of a bitwise type
> > without triggering sparse warnings, disable signedness checking when
> > verifying code with sparse.  
> 
> What, what, what?
> 
> The last I saw of this discussion, the fix was just to make sparse not
> warn about these cases. Why did this bogus fix make it into a pull
> request that I will now ignore?

Sorry, I was triaging my internal patchwork and saw the "Suggested-by"
Christoph and was thinking this was what we decided on.

> 
> 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.
> 
> So that "no known way" was always bogus, the real question was whether
> there was a way to make sparse not need the "ignore bitwise" hack.
> 
> 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.


I'll remove this patch and send another pull request.

Thanks,

-- Steve