Messages in this thread | | | Date | Sun, 21 Aug 2022 15:55:33 -0400 | From | Steven Rostedt <> | Subject | Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed_type() |
| |
On Sun, 21 Aug 2022 11:35:29 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sat, Aug 20, 2022 at 5:08 PM Steven Rostedt <rostedt@goodmis.org> 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
| |