linux-sparse.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Warn about "unsigned value that used to be signed against zero"
@ 2025-09-21  6:12 Vincent Mailhol
  2025-09-21 15:16 ` Linus Torvalds
  2025-09-22 12:02 ` Chris Li
  0 siblings, 2 replies; 14+ messages in thread
From: Vincent Mailhol @ 2025-09-21  6:12 UTC (permalink / raw)
  To: linux-sparse, Chris Li
  Cc: Luc Van Oostenryck, Linus Torvalds, Steven Rostedt,
	Vincent Mailhol

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  2025-09-21  6:12 [PATCH] Warn about "unsigned value that used to be signed against zero" Vincent Mailhol
@ 2025-09-21 15:16 ` Linus Torvalds
  2025-09-22 12:10   ` Chris Li
  2025-09-22 12:02 ` Chris Li
  1 sibling, 1 reply; 14+ messages in thread
From: Linus Torvalds @ 2025-09-21 15:16 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: linux-sparse, Chris Li, Luc Van Oostenryck, Steven Rostedt

On Sat, 20 Sept 2025 at 23:13, Vincent Mailhol <mailhol@kernel.org> wrote:
>
> 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.

Ack. I had forgotten about this patch, but I used it for a little bit
in my private testing, and it seemed to work fairly well.

It had some false positives too, but the ones I looked at generally
made _sense_ to me.

They weren't always trivial to figure out, though (nor were the
non-false positives, for that matter). Because sometimes the signed
source was fairly far away from the unsigned use that it complained
about.

That's why I had added that

        info(def->pos, "signed value source");

part of the patch: it still didn't always make it entirely obvious,
but it helped a lot when the value came in from a mix of macros and
inline functions, and it was hard to see what the source of the issue
was.

Even with that information, it wasn't necessarily easy, but it was eas_ier_.

But I only used that patch for a couple of days and only looked at a
fairly small handful of cases overall.

I felt it was *enormously* much better than the insane and completely
"-Wtype-limits" warning was, because that one warns for code that
cannot sanely be improved (ie the "fix" for that warning is typically
to remove a sane test that might be required in a macro in other
contexts or to just disallow some sane types).

Put another way: it looked fine in my testing, and it matched my "gut
feel" for how things should work.

But I didn't use it enough to really know for sure and then I forgot
about it all.

If Vincent has been using it for months successfully, I think that's a
good sign that it wasn't _just_ my gut feeling.

             Linus

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  2025-09-21  6:12 [PATCH] Warn about "unsigned value that used to be signed against zero" Vincent Mailhol
  2025-09-21 15:16 ` Linus Torvalds
@ 2025-09-22 12:02 ` Chris Li
  2025-09-22 13:00   ` Vincent Mailhol
  1 sibling, 1 reply; 14+ messages in thread
From: Chris Li @ 2025-09-22 12:02 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: linux-sparse, Luc Van Oostenryck, Linus Torvalds, Steven Rostedt

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  2025-09-21 15:16 ` Linus Torvalds
@ 2025-09-22 12:10   ` Chris Li
  2025-09-22 16:16     ` Linus Torvalds
  0 siblings, 1 reply; 14+ messages in thread
From: Chris Li @ 2025-09-22 12:10 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Vincent Mailhol, linux-sparse, Luc Van Oostenryck, Steven Rostedt

On Sun, Sep 21, 2025 at 8:16 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, 20 Sept 2025 at 23:13, Vincent Mailhol <mailhol@kernel.org> wrote:
> >
> > 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.
>
> Ack. I had forgotten about this patch, but I used it for a little bit
> in my private testing, and it seemed to work fairly well.
>
> It had some false positives too, but the ones I looked at generally
> made _sense_ to me.

I think the false positive is kind of unavoidable there. Detecting
such a case can be equivalent to the turing halting problem, which is
impossible to have a perfect rule or algrothium.

>
> They weren't always trivial to figure out, though (nor were the
> non-false positives, for that matter). Because sometimes the signed
> source was fairly far away from the unsigned use that it complained
> about.
>
> That's why I had added that
>
>         info(def->pos, "signed value source");

Do you want such a line in the final patch as well? Seems worth it.

> part of the patch: it still didn't always make it entirely obvious,
> but it helped a lot when the value came in from a mix of macros and
> inline functions, and it was hard to see what the source of the issue
> was.
>
> Even with that information, it wasn't necessarily easy, but it was eas_ier_.
>
> But I only used that patch for a couple of days and only looked at a
> fairly small handful of cases overall.
>
> I felt it was *enormously* much better than the insane and completely
> "-Wtype-limits" warning was, because that one warns for code that
> cannot sanely be improved (ie the "fix" for that warning is typically
> to remove a sane test that might be required in a macro in other
> contexts or to just disallow some sane types).
>
> Put another way: it looked fine in my testing, and it matched my "gut
> feel" for how things should work.
>
> But I didn't use it enough to really know for sure and then I forgot
> about it all.
>
> If Vincent has been using it for months successfully, I think that's a
> good sign that it wasn't _just_ my gut feeling.

Yes, I applied in my local git repo smoothly. Just waiting for the
test case now.

Chris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  2025-09-22 12:02 ` Chris Li
@ 2025-09-22 13:00   ` Vincent Mailhol
  2025-09-22 14:58     ` Chris Li
  0 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2025-09-22 13:00 UTC (permalink / raw)
  To: Chris Li; +Cc: linux-sparse, Luc Van Oostenryck, Linus Torvalds, Steven Rostedt

On 22/09/2025 at 21:02, Chris Li wrote:

(...)

> 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.

No problem!

Would something like that be OK?

diff --git a/validation/used-to-be-signed.c b/validation/used-to-be-signed.c
new file mode 100644
index 00000000..75c0ca38
--- /dev/null
+++ b/validation/used-to-be-signed.c
@@ -0,0 +1,25 @@
+void error(void);
+int check(void);
+
+static void foo(unsigned int val)
+{
+	unsigned int ret;
+
+	/* Positive test case */
+	ret = check();
+	if (ret < 0)
+		error();
+
+	/* Negative test case */
+	if (val < 0 || val > 42)
+		error();
+}
+
+/*
+ * check-name: used-to-be-signed
+ *
+ * check-error-start
+used-to-be-signed.c:10:19: warning: unsigned value that used to be signed
checked against zero?
+used-to-be-signed.c:9:20: signed value source
+ * check-error-end
+ */

Output:

  $ ./test-suite used-to-be-signed.c
    TEST    used-to-be-signed (used-to-be-signed.c)
  OK: out of 1 tests, 1 passed, 0 failed


Also, do you want the test in a separate patch or should I squash with previous
patch?


Yours sincerely,
Vincent Mailhol

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  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
  0 siblings, 1 reply; 14+ messages in thread
From: Chris Li @ 2025-09-22 14:58 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: linux-sparse, Luc Van Oostenryck, Linus Torvalds, Steven Rostedt

On Mon, Sep 22, 2025 at 6:00 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>
> On 22/09/2025 at 21:02, Chris Li wrote:
>
> (...)
>
> > 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.
>
> No problem!
>
> Would something like that be OK?

I was thinking "used-to-be-signed-positive.c" and
"used-to-be-signed-negative.c".
Merging them into one file is fine as well.

If you have other positive and negative test cases, maybe it makes
sense to have positive and negative as separate files so make things
obvious. Just one each is fine in one file as well, your call.

> diff --git a/validation/used-to-be-signed.c b/validation/used-to-be-signed.c
> new file mode 100644
> index 00000000..75c0ca38
> --- /dev/null
> +++ b/validation/used-to-be-signed.c
> @@ -0,0 +1,25 @@
> +void error(void);
> +int check(void);
> +
> +static void foo(unsigned int val)
> +{
> +       unsigned int ret;
> +
> +       /* Positive test case */
> +       ret = check();
> +       if (ret < 0)
> +               error();
> +
> +       /* Negative test case */
> +       if (val < 0 || val > 42)
> +               error();
> +}
> +
> +/*
> + * check-name: used-to-be-signed
> + *
> + * check-error-start
> +used-to-be-signed.c:10:19: warning: unsigned value that used to be signed
> checked against zero?
> +used-to-be-signed.c:9:20: signed value source
> + * check-error-end
> + */
>
> Output:
>
>   $ ./test-suite used-to-be-signed.c
>     TEST    used-to-be-signed (used-to-be-signed.c)
>   OK: out of 1 tests, 1 passed, 0 failed
>
>
> Also, do you want the test in a separate patch or should I squash with previous
> patch?

Ideally the test is before patch so it can show the before and after
effect of the protagonist patch, making more test cases pass.

Chris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] vadidation: add used-to-be-signed unit tests
  2025-09-22 14:58     ` Chris Li
@ 2025-09-22 15:53       ` Vincent Mailhol
  2025-09-24  7:03         ` Chris Li
  0 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2025-09-22 15:53 UTC (permalink / raw)
  To: Chris Li, linux-sparse
  Cc: Luc Van Oostenryck, Linus Torvalds, Steven Rostedt,
	Vincent Mailhol

Add unit tests for the new used-to-be-signed check as introduced in [1]:

Results before applying [1]:

  $ ./test-suite used-to-be-signed.c
    TEST    used-to-be-signed (used-to-be-signed.c)
  error: actual error text does not match expected error text.
  error: see used-to-be-signed.c.error.* for further investigation.
  --- used-to-be-signed.c.error.expected	2025-09-23 00:50:07.079654644 +0900
  +++ used-to-be-signed.c.error.got	2025-09-23 00:50:07.073654719 +0900
  @@ -1,8 +0,0 @@
  -used-to-be-signed.c:8:19: warning: unsigned value that used to be signed checked against zero?
  -used-to-be-signed.c:6:33: signed value source
  -used-to-be-signed.c:11:17: warning: unsigned value that used to be signed checked against zero?
  -used-to-be-signed.c:6:33: signed value source
  -used-to-be-signed.c:14:20: warning: unsigned value that used to be signed checked against zero?
  -used-to-be-signed.c:6:33: signed value source
  -used-to-be-signed.c:17:18: warning: unsigned value that used to be signed checked against zero?
  -used-to-be-signed.c:6:33: signed value source
  error: FAIL: test 'used-to-be-signed.c' failed
  KO: out of 1 tests, 0 passed, 1 failed

...and after:

  $ ./test-suite used-to-be-signed.c
    TEST    used-to-be-signed (used-to-be-signed.c)
  OK: out of 1 tests, 1 passed, 0 failed

[1] Warn about "unsigned value that used to be signed against zero"
Link: https://lore.kernel.org/linux-sparse/20250921061337.3047616-1-mailhol@kernel.org/

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Hi Chris,

Thanks for your guidance. I added more tests.

Because you asked me for my preference, I went for the single file.
But if you finally have a preference to split, tell me and I will
update.
---
 validation/used-to-be-signed.c | 49 ++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 validation/used-to-be-signed.c

diff --git a/validation/used-to-be-signed.c b/validation/used-to-be-signed.c
new file mode 100644
index 00000000..75eab7f3
--- /dev/null
+++ b/validation/used-to-be-signed.c
@@ -0,0 +1,49 @@
+void error(void);
+int check(void);
+
+static void positive_tests(unsigned int val)
+{
+	unsigned int ret = check();
+
+	if (ret < 0)
+		error();
+
+	if (0 > ret)
+		error();
+
+	if (ret >= 0)
+		/* Do stuff */;
+
+	if (0 <= ret)
+		/* Do stuff */;
+}
+
+static void negative_tests(unsigned int val)
+{
+	if (val < 0 || val > 42)
+		error();
+
+	if (0 > val || 42 < val)
+		error();
+
+	if (val >= 0 && val < 42)
+		/* Do stuff */;
+
+	if (0 <= val && 42 > val)
+		/* Do stuff */;
+}
+
+/*
+ * check-name: used-to-be-signed
+ *
+ * check-error-start
+used-to-be-signed.c:8:19: warning: unsigned value that used to be signed checked against zero?
+used-to-be-signed.c:6:33: signed value source
+used-to-be-signed.c:11:17: warning: unsigned value that used to be signed checked against zero?
+used-to-be-signed.c:6:33: signed value source
+used-to-be-signed.c:14:20: warning: unsigned value that used to be signed checked against zero?
+used-to-be-signed.c:6:33: signed value source
+used-to-be-signed.c:17:18: warning: unsigned value that used to be signed checked against zero?
+used-to-be-signed.c:6:33: signed value source
+ * check-error-end
+ */
-- 
2.49.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  2025-09-22 12:10   ` Chris Li
@ 2025-09-22 16:16     ` Linus Torvalds
  2025-09-24  7:07       ` Chris Li
  0 siblings, 1 reply; 14+ messages in thread
From: Linus Torvalds @ 2025-09-22 16:16 UTC (permalink / raw)
  To: Chris Li
  Cc: Vincent Mailhol, linux-sparse, Luc Van Oostenryck, Steven Rostedt

On Mon, 22 Sept 2025 at 05:10, Chris Li <sparse@chrisli.org> wrote:
>
> On Sun, Sep 21, 2025 at 8:16 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > That's why I had added that
> >
> >         info(def->pos, "signed value source");
>
> Do you want such a line in the final patch as well? Seems worth it.

Oh, absolutely. Some of the cases it reported were really hard to
understand without it.

If it ends up being noisy, there could possibly be some heuristic like
"if the source is very close to the use that triggers it, don't bother
talking about it", but that would be a later tweak.

           Linus

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] vadidation: add used-to-be-signed unit tests
  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
  0 siblings, 1 reply; 14+ messages in thread
From: Chris Li @ 2025-09-24  7:03 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: linux-sparse, Luc Van Oostenryck, Linus Torvalds, Steven Rostedt

On Mon, Sep 22, 2025 at 8:54 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>
> Add unit tests for the new used-to-be-signed check as introduced in [1]:

Applied and pushed on sparse-dev repo. Can you please take a look if I
am doing it correctly on the sparse-dev?
Linus has one more debug print line, can you add it for me and submit
an incremental patch? It should be just a one liner. I will squash it
with your change. I can ping you on the other email as well.

I intend to use sparse-dev as the unstable sparse developer repo. It
will always be based on sparse repo but the commit in sparse-dev can
be rewinded. Patches will sit in the sparse-dev for about a week then
move into sparse repo. The sparse repo is a stable repo, it will not
rewind.

BTW, the recommended base to submit the sparse patches is the stable
sparse repo unless you depend on some bleeding edge feature only on
sparse-dev repo. Pull request please base on the sparse repo not the
unstable sparse-dev repo.

Thanks

Chris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  2025-09-22 16:16     ` Linus Torvalds
@ 2025-09-24  7:07       ` Chris Li
  2025-09-24  8:54         ` Vincent Mailhol
  0 siblings, 1 reply; 14+ messages in thread
From: Chris Li @ 2025-09-24  7:07 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Vincent Mailhol, linux-sparse, Luc Van Oostenryck, Steven Rostedt

On Mon, Sep 22, 2025 at 9:17 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, 22 Sept 2025 at 05:10, Chris Li <sparse@chrisli.org> wrote:
> >
> > On Sun, Sep 21, 2025 at 8:16 AM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > That's why I had added that
> > >
> > >         info(def->pos, "signed value source");
> >
> > Do you want such a line in the final patch as well? Seems worth it.
>
> Oh, absolutely. Some of the cases it reported were really hard to
> understand without it.
>
> If it ends up being noisy, there could possibly be some heuristic like
> "if the source is very close to the use that triggers it, don't bother
> talking about it", but that would be a later tweak.

Vincent, can you add the above info() line suggested by Linus as
incremental or full patches?

I will try out the patch folding on sparse-dev.

Chris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  2025-09-24  7:07       ` Chris Li
@ 2025-09-24  8:54         ` Vincent Mailhol
  2025-09-24 17:44           ` Chris Li
  0 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2025-09-24  8:54 UTC (permalink / raw)
  To: Chris Li, Linus Torvalds; +Cc: linux-sparse, Luc Van Oostenryck, Steven Rostedt

On 24/09/2025 at 16:07, Chris Li wrote:
> On Mon, Sep 22, 2025 at 9:17 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> On Mon, 22 Sept 2025 at 05:10, Chris Li <sparse@chrisli.org> wrote:
>>>
>>> On Sun, Sep 21, 2025 at 8:16 AM Linus Torvalds
>>> <torvalds@linux-foundation.org> wrote:
>>>>
>>>> That's why I had added that
>>>>
>>>>         info(def->pos, "signed value source");
>>>
>>> Do you want such a line in the final patch as well? Seems worth it.
>>
>> Oh, absolutely. Some of the cases it reported were really hard to
>> understand without it.
>>
>> If it ends up being noisy, there could possibly be some heuristic like
>> "if the source is very close to the use that triggers it, don't bother
>> talking about it", but that would be a later tweak.
> 
> Vincent, can you add the above info() line suggested by Linus as
> incremental or full patches?

I think that there is a small confusion here. That line is already in the patch
inside simplify_unsigned_zero_compare(). I think that Linus was just explaining
why he added it in the original patch.


Yours sincerely,
Vincent Mailhol


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] vadidation: add used-to-be-signed unit tests
  2025-09-24  7:03         ` Chris Li
@ 2025-09-24  9:27           ` Vincent Mailhol
  2025-09-24 17:47             ` Chris Li
  0 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2025-09-24  9:27 UTC (permalink / raw)
  To: Chris Li; +Cc: linux-sparse, Luc Van Oostenryck, Linus Torvalds, Steven Rostedt

On 24/09/2025 at 16:03, Chris Li wrote:
> On Mon, Sep 22, 2025 at 8:54 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>>
>> Add unit tests for the new used-to-be-signed check as introduced in [1]:
> 
> Applied and pushed on sparse-dev repo. Can you please take a look if I
> am doing it correctly on the sparse-dev?

I just checked out sparse-dev. I re-run the tests and everything looks fine!

> Linus has one more debug print line, can you add it for me and submit
> an incremental patch? It should be just a one liner. I will squash it
> with your change. I can ping you on the other email as well.

I replied in the other thread. That one line is already in the final patch ;)

> I intend to use sparse-dev as the unstable sparse developer repo. It
> will always be based on sparse repo but the commit in sparse-dev can
> be rewinded. Patches will sit in the sparse-dev for about a week then
> move into sparse repo. The sparse repo is a stable repo, it will not
> rewind.
> 
> BTW, the recommended base to submit the sparse patches is the stable
> sparse repo unless you depend on some bleeding edge feature only on
> sparse-dev repo. Pull request please base on the sparse repo not the
> unstable sparse-dev repo.
Ack. At the moment, I am not planning to do more sparse development, this patch
will likely be a one shot. But I will keep this in mind if I either decide to do
another contribution.


Yours sincerely,
Vincent Mailhol


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Warn about "unsigned value that used to be signed against zero"
  2025-09-24  8:54         ` Vincent Mailhol
@ 2025-09-24 17:44           ` Chris Li
  0 siblings, 0 replies; 14+ messages in thread
From: Chris Li @ 2025-09-24 17:44 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Linus Torvalds, linux-sparse, Luc Van Oostenryck, Steven Rostedt

On Wed, Sep 24, 2025 at 1:54 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>
>
> I think that there is a small confusion here. That line is already in the patch
> inside simplify_unsigned_zero_compare(). I think that Linus was just explaining
> why he added it in the original patch.

Ah, sorry for my confusion and thanks for the clarification.

It is all good then.

Chris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] vadidation: add used-to-be-signed unit tests
  2025-09-24  9:27           ` Vincent Mailhol
@ 2025-09-24 17:47             ` Chris Li
  0 siblings, 0 replies; 14+ messages in thread
From: Chris Li @ 2025-09-24 17:47 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: linux-sparse, Luc Van Oostenryck, Linus Torvalds, Steven Rostedt

On Wed, Sep 24, 2025 at 2:28 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>
> On 24/09/2025 at 16:03, Chris Li wrote:
> > On Mon, Sep 22, 2025 at 8:54 AM Vincent Mailhol <mailhol@kernel.org> wrote:
> >>
> >> Add unit tests for the new used-to-be-signed check as introduced in [1]:
> >
> > Applied and pushed on sparse-dev repo. Can you please take a look if I
> > am doing it correctly on the sparse-dev?
>
> I just checked out sparse-dev. I re-run the tests and everything looks fine!

Thanks for the confirmation.


>
> > Linus has one more debug print line, can you add it for me and submit
> > an incremental patch? It should be just a one liner. I will squash it
> > with your change. I can ping you on the other email as well.
>
> I replied in the other thread. That one line is already in the final patch ;)

Ack.

>
> > I intend to use sparse-dev as the unstable sparse developer repo. It
> > will always be based on sparse repo but the commit in sparse-dev can
> > be rewinded. Patches will sit in the sparse-dev for about a week then
> > move into sparse repo. The sparse repo is a stable repo, it will not
> > rewind.
> >
> > BTW, the recommended base to submit the sparse patches is the stable
> > sparse repo unless you depend on some bleeding edge feature only on
> > sparse-dev repo. Pull request please base on the sparse repo not the
> > unstable sparse-dev repo.
> Ack. At the moment, I am not planning to do more sparse development, this patch
> will likely be a one shot. But I will keep this in mind if I either decide to do
> another contribution.

That is more of a note for the general sparse patch submission.
Alignment for how to use the two sparse repo. I will send a separate
email to the sparse mailing list.

Thanks for your first two patches after I am back as the maintainer.

Chris

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2025-09-24 17:47 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-21  6:12 [PATCH] Warn about "unsigned value that used to be signed against zero" Vincent Mailhol
2025-09-21 15:16 ` 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

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).