Re: [PATCH] kbuild: treat char as always signed

From: Nick Desaulniers <ndesaulniers@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Segher Boessenkool <segher@kernel.crashing.org>,
	"Jason A. Donenfeld" <Jason@zx2c4.com>,
	linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org,
	linux-arch@vger.kernel.org, linux-toolchains@vger.kernel.org,
	Masahiro Yamada <masahiroy@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH] kbuild: treat char as always signed
Date: Wed, 19 Oct 2022 11:10:34 -0700	[thread overview]
Message-ID: <CAKwvOdmDz2VfU1JJkAEnPLTcx4PHH48KfZQfW6gvO6we_QbrRQ@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=whggBoH78ojE0wttyHKwuf48hrSS_X7s3D3Qd_516ayzQ@mail.gmail.com>

On Wed, Oct 19, 2022 at 10:26 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Wed, Oct 19, 2022 at 10:14 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > The pointer-sign thing doesn't actually help (ie it won't find places
> > where you actually compare a char), and it causes untold damage in
> > doing completely insane things.
>
> Side note: several years ago I tried to make up some sane rules to
> have 'sparse' actually be able to warn when a 'char' was used in a
> context where the sign mattered.

Do you have examples? Maybe we could turn this into a compiler feature
request.  Having prior art on the problem would be a boon.

>
> I failed miserably.
>
> You actually can see some signs (heh) of that in the sparse sources,
> in that the type system actually has a bit for explicitly signed types
> ("MOD_EXPLICITLY_SIGNED"), but it ends up being almost entirely
> unused.
>
> That bit does still have one particular use: the "bitfield is
> dubiously signed" thing where sparse will complain about bitfields
> that are implicitly (but not explicitly) signed. Because people really
> expect 'int a:1' to have values 0/1, not 0/-1.

Clang's -Wbitfield-constant-conversion can catch that.
commit 5c5c2baad2b5 ("ASoC: mchp-spdiftx: Fix clang
-Wbitfield-constant-conversion")
commit eab9100d9898 ("ASoC: mchp-spdiftx: Fix clang
-Wbitfield-constant-conversion")
commit 37209783c73a ("thunderbolt: Make priority unsigned in struct tb_path")

>
> But the original intent was to find code where people used a 'char'
> that wasn't explicitly signed, and that then had architecture-defined
> behavior.
>
> I just could not come up with any even remotely sane warning
> heuristics that didn't have a metric buttload of false positives.
>
> I still have this feeling that it *should* be possible to warn about
> the situation where you end up doing an implicit type widening (ie the
> normal C "arithmetic is always done in at least 'int'") that then does
> not get narrowed down again without the upper bits ever mattering.
>
> But it needs somebody smarter than me, I'm afraid.
>
> And the fact that I don't think any other compiler has that warning
> either makes me just wonder if my feeling that it should be possible
> is just wrong.
>
>                    Linus

-- 
Thanks,
~Nick Desaulniers