Re: [PATCH] Warn about "unsigned value that used to be signed against zero"

[Chris Li <sparse@chrisli.org>]

On Sat, Sep 20, 2025 at 11:13 PM Vincent Mailhol <mailhol@kernel.org> wrote:
>
> Consider this first pattern:
>
>   void error(void);
>   int check(void);
>
>   void foo (void)
>   {
>         unsigned int ret;
>
>         ret = check();
>         if (ret < 0)
>                 error();
>   }
>
> Here, the comparison against zero is a tautology: ret, which is
> unsigned, can never be negative. Thus the compiler will remove the
> error branch causing a bug.
>
> This pattern is caught by clang and gcc's -Wtype-limits. *However*,
> that diagnostic has many lost bullets. It will also complain on some
> legitimate things such as in this second pattern:
>
>   void error(void);
>
>   void bar (unsigned int val)
>   {
>         if (val < 0 || val > 42)
>                 error();
>   }
>
> Here, the author just want to do a range check. Yes, the
>
>   val < 0
>
> comparison is a tautology, but that time, it does not result in faulty
> code when optimised out by the compiler.
>
> There is thus a need for a check that will catch the first pattern but
> that will let the second one go through. The difference between the
> two patterns is that in the first one the value returned by the
> check() function used to be signed whereas in the second one val was
> always unsigned to begin with.

Sounds like a bit heuristic but if it helps to reduce the noise level
that seems worth it.

>
> Add a check in sparse to warn if a value which used to be signed gets
> assigned to an unsigned and then gets compared against zero, either
> val < 0 or val >= 0.
>
> As pointed out by Linus in his original message, a few false positives
> remain, especially when many inline functions and macros get involved,
> but the level of noise is nothing in comparison to the -Wtype-limits.

Can you please add a few validation checks for  the positive and
negative case? You  can add it under the validation directory. With
validation I can quickly catch the behavior change in the future.

>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/all/CAHk-=wjQCbRA1UEag-1-9yn08KNNqerTj++SCbbW80At=rg5RQ@mail.gmail.com/
> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
> ---
> Hi Chris,
>
> I saw your email in which you announced your comeback. First of all,
> I wish you a warm welcome back!
>
> Second, I would like to inaugurate your comeback with this patch. It

Thank you. I did apply the patch in my local git branch without any conflict.

Waiting for your test case.

> was posted by Linus in the middle of the huge thread about the Rust
> kernel policy, so I guess it did not catch the attention it
> deserved. I have been using this locally for the last half year and it
> works fine.
>
> So, aside from a minor change as listed in below Changelog, this is
> basically a resend.
>
> As for the tags, I tagged Linus as Suggested-by and myself as the
> author. Not sure if this is the most appropriate tag, but adding
> Linus's Signed-off tag seems wrong, so this is the best tag I could
> think of. Let me know if there is any more appropriate tag.

Looks good otherwise.

Chris