Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220821000737.328590235@goodmis.org> <20220821000844.510643400@goodmis.org>
In-Reply-To: <20220821000844.510643400@goodmis.org>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun, 21 Aug 2022 11:35:29 -0700
Message-ID: <CAHk-=wjsxu782N0P+oMu35N7rJAOeh3buQFWJaZHZTNmVSB=3Q@mail.gmail.com>
Subject: Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings
 triggered by is_signed_type()
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Christoph Hellwig <hch@lst.de>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
        Jens Axboe <axboe@kernel.dk>,
        Bart Van Assche <bvanassche@acm.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

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.

                  Linus