diff options
| author | Vincent Mailhol <mailhol@kernel.org> | 2025-09-21 15:12:30 +0900 |
|---|---|---|
| committer | Chris Li <sparse@chrisli.org> | 2025-09-23 23:43:00 -0700 |
| commit | 366ad4b2fa3e8361001a08b3719881b6d7c87e90 (patch) | |
| tree | 7a5ff32b6ef13f119bf83e72614d4a1e7f83cb0a | |
| parent | c47766ce0620001dfaf201674c990cde2e10423e (diff) | |
| download | sparse-dev-366ad4b2fa3e8361001a08b3719881b6d7c87e90.tar.gz | |
Warn about "unsigned value that used to be signed against zero"
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>
Signed-off-by: Chris Li <sparse@chrisli.org>
| -rw-r--r-- | simplify.c | 41 |
1 files changed, 39 insertions, 2 deletions
@@ -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) |