From: Vincent Mailhol <mailhol@kernel.org>
To: linux-sparse@vger.kernel.org, Chris Li <sparse@chrisli.org>
Cc: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Steven Rostedt <rostedt@goodmis.org>,
Vincent Mailhol <mailhol@kernel.org>
Subject: [PATCH] Warn about "unsigned value that used to be signed against zero"
Date: Sun, 21 Sep 2025 15:12:30 +0900 [thread overview]
Message-ID: <20250921061337.3047616-1-mailhol@kernel.org> (raw)
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.
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.
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
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.
Changelog:
Linus's patch -> v2:
- Add a patch description
- Change warning message from
unsigned value that used to be signed checked for negative?
to
unsigned value that used to be signed checked against zero?
because the check catches both unsigned < 0 and unsigned >= 0
tautologies.
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git/commit/?id=46ee49478660
Link: https://lore.kernel.org/all/CAHk-=wjQCbRA1UEag-1-9yn08KNNqerTj++SCbbW80At=rg5RQ@mail.gmail.com/
---
simplify.c | 41 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/simplify.c b/simplify.c
index 3c4ace3c..68c5f9c7 100644
--- a/simplify.c
+++ b/simplify.c
@@ -1167,6 +1167,43 @@ static int simplify_seteq_setne(struct instruction *insn, long long value)
return 0;
}
+static struct instruction *used_to_be_signed(struct instruction *insn)
+{
+ pseudo_t pseudo = insn->src1;
+ struct instruction *def;
+ struct symbol *sym;
+
+ if (pseudo->type != PSEUDO_REG)
+ return NULL;
+ def = pseudo->def;
+ if (!def)
+ return NULL;
+
+ // Did the value come from a sign-extension?
+ // If so, the source was clearly signed
+ if (def->opcode == OP_SEXT)
+ return def;
+
+ // Or was the op that generated the value signed?
+ sym = def->type;
+ if (sym && !(sym->ctype.modifiers & MOD_UNSIGNED))
+ return def;
+
+ return NULL;
+}
+
+static int simplify_unsigned_zero_compare(struct instruction *insn, int result)
+{
+ struct instruction *def = used_to_be_signed(insn);
+
+ if (def) {
+ warning(insn->pos, "unsigned value that used to be signed checked against zero?");
+ info(def->pos, "signed value source");
+ }
+
+ return replace_with_pseudo(insn, value_pseudo(result));
+}
+
static int simplify_compare_constant(struct instruction *insn, long long value)
{
unsigned size = insn->itype->bit_size;
@@ -1228,7 +1265,7 @@ static int simplify_compare_constant(struct instruction *insn, long long value)
case OP_SET_B:
if (!value) // (x < 0) --> 0
- return replace_with_pseudo(insn, value_pseudo(0));
+ return simplify_unsigned_zero_compare(insn, 0);
if (value == 1) // (x < 1) --> (x == 0)
return replace_binop_value(insn, OP_SET_EQ, 0);
else if (value == bits) // (x < ~0) --> (x != ~0)
@@ -1238,7 +1275,7 @@ static int simplify_compare_constant(struct instruction *insn, long long value)
break;
case OP_SET_AE:
if (!value) // (x >= 0) --> 1
- return replace_with_pseudo(insn, value_pseudo(1));
+ return simplify_unsigned_zero_compare(insn, 1);
if (value == 1) // (x >= 1) --> (x != 0)
return replace_binop_value(insn, OP_SET_NE, 0);
else if (value == bits) // (x >= ~0) --> (x == ~0)
--
2.49.1
next reply other threads:[~2025-09-21 6:13 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-21 6:12 Vincent Mailhol [this message]
2025-09-21 15:16 ` [PATCH] Warn about "unsigned value that used to be signed against zero" Linus Torvalds
2025-09-22 12:10 ` Chris Li
2025-09-22 16:16 ` Linus Torvalds
2025-09-24 7:07 ` Chris Li
2025-09-24 8:54 ` Vincent Mailhol
2025-09-24 17:44 ` Chris Li
2025-09-22 12:02 ` Chris Li
2025-09-22 13:00 ` Vincent Mailhol
2025-09-22 14:58 ` Chris Li
2025-09-22 15:53 ` [PATCH] vadidation: add used-to-be-signed unit tests Vincent Mailhol
2025-09-24 7:03 ` Chris Li
2025-09-24 9:27 ` Vincent Mailhol
2025-09-24 17:47 ` Chris Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250921061337.3047616-1-mailhol@kernel.org \
--to=mailhol@kernel.org \
--cc=linux-sparse@vger.kernel.org \
--cc=luc.vanoostenryck@gmail.com \
--cc=rostedt@goodmis.org \
--cc=sparse@chrisli.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).