In twos complement the most negative number can not be negated.

Fixes: b1c21e7d99cd ("tools/nolibc/stdlib: add i64toa() and u64toa()")
Fixes: 66c397c4d2e1 ("tools/nolibc/stdlib: replace the ltoa() function with more efficient ones")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/include/nolibc/stdlib.h | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/tools/include/nolibc/stdlib.h b/tools/include/nolibc/stdlib.h
index 86ad378ab1ea220559d5ab1adc4bb9972977ba9e..5e4b97810d49ac1b1bd79d6f779f6a748f188a39 100644
--- a/tools/include/nolibc/stdlib.h
+++ b/tools/include/nolibc/stdlib.h
@@ -271,16 +271,12 @@ int utoa_r(unsigned long in, char *buffer)
 static __attribute__((unused))
 int itoa_r(long in, char *buffer)
 {
-	char *ptr = buffer;
-	int len = 0;
-
 	if (in < 0) {
-		in = -in;
-		*(ptr++) = '-';
-		len++;
+		*(buffer++) = '-';
+		return 1 + utoa_r(-(unsigned long)in, buffer);
 	}
-	len += utoa_r(in, ptr);
-	return len;
+
+	return utoa_r(in, buffer);
 }
 
 /* for historical compatibility, same as above but returns the pointer to the
@@ -407,16 +403,12 @@ int u64toa_r(uint64_t in, char *buffer)
 static __attribute__((unused))
 int i64toa_r(int64_t in, char *buffer)
 {
-	char *ptr = buffer;
-	int len = 0;
-
 	if (in < 0) {
-		in = -in;
-		*(ptr++) = '-';
-		len++;
+		*(buffer++) = '-';
+		return 1 + u64toa_r(-(unsigned long long)in, buffer);
 	}
-	len += u64toa_r(in, ptr);
-	return len;
+
+	return u64toa_r(in, buffer);
 }
 
 /* converts int64_t <in> to a string using the static itoa_buffer and returns

-- 
2.49.0

On Wed, Apr 16, 2025 at 08:40:19PM +0200, Thomas Weißschuh wrote:
> In twos complement the most negative number can not be negated.

well, if we're picky, that's not really an int overflow since it's only
used as an unsigned in the end, so -2^(N-1) == 2^(N-1) in twos complement.

> @@ -271,16 +271,12 @@ int utoa_r(unsigned long in, char *buffer)
>  static __attribute__((unused))
>  int itoa_r(long in, char *buffer)
>  {
> -	char *ptr = buffer;
> -	int len = 0;
> -
>  	if (in < 0) {
> -		in = -in;
> -		*(ptr++) = '-';
> -		len++;
> +		*(buffer++) = '-';
> +		return 1 + utoa_r(-(unsigned long)in, buffer);
>  	}
> -	len += utoa_r(in, ptr);
> -	return len;
> +
> +	return utoa_r(in, buffer);
>  }

At -Os it's OK but at -O2 it inflates it a little bit and at -O3 it
significantly inflates the function (175 -> 320 bytes) due to the two
calls that get inlined. Have you tried to check if ubsan is happy
with just this?

-		in = -in;
+		in = -(unsigned long)in;

Otherwise this variant doesn't inflate it for me and keeps the spirit
of the original one (i.e. single call):

  int itoa_r3(long in, char *buffer)
  {
        unsigned long uin = in;
        int len = 0;

        if ((long)uin < 0) {
                uin = -uin;
                *(buffer++) = '-';
                len++;
        }
        len += utoa_r(uin, buffer);
        return len;
  }

I'm also seeing a way to make it even smaller than the original by
changing utoa_r() so that it takes the original buffer in a 3rd
argument and emits the difference at the end as the length. This
allows to always perform a tail call, though I'm not sure we really
need it. 
 
Thanks,
Willy

On 2025-04-19 11:40:08+0200, Willy Tarreau wrote:
> On Wed, Apr 16, 2025 at 08:40:19PM +0200, Thomas Weißschuh wrote:
> > In twos complement the most negative number can not be negated.
> 
> well, if we're picky, that's not really an int overflow since it's only
> used as an unsigned in the end, so -2^(N-1) == 2^(N-1) in twos complement.

It is used as unsigned, but at the point of the cast it's still signed.
From ccpreference:

The unary minus invokes undefined behavior due to signed integer
overflow when applied to INT_MIN, LONG_MIN, or LLONG_MIN, on typical
(2's complement) platforms.

https://en.cppreference.com/w/c/language/operator_arithmetic

> 
> > @@ -271,16 +271,12 @@ int utoa_r(unsigned long in, char *buffer)
> >  static __attribute__((unused))
> >  int itoa_r(long in, char *buffer)
> >  {
> > -	char *ptr = buffer;
> > -	int len = 0;
> > -
> >  	if (in < 0) {
> > -		in = -in;
> > -		*(ptr++) = '-';
> > -		len++;
> > +		*(buffer++) = '-';
> > +		return 1 + utoa_r(-(unsigned long)in, buffer);
> >  	}
> > -	len += utoa_r(in, ptr);
> > -	return len;
> > +
> > +	return utoa_r(in, buffer);
> >  }
> 
> At -Os it's OK but at -O2 it inflates it a little bit and at -O3 it
> significantly inflates the function (175 -> 320 bytes) due to the two
> calls that get inlined. Have you tried to check if ubsan is happy
> with just this?
> 
> -		in = -in;
> +		in = -(unsigned long)in;

That seems to work. Let's use it.

Thanks!

> Otherwise this variant doesn't inflate it for me and keeps the spirit
> of the original one (i.e. single call):
> 
>   int itoa_r3(long in, char *buffer)
>   {
>         unsigned long uin = in;
>         int len = 0;
> 
>         if ((long)uin < 0) {
>                 uin = -uin;
>                 *(buffer++) = '-';
>                 len++;
>         }
>         len += utoa_r(uin, buffer);
>         return len;
>   }
> 
> I'm also seeing a way to make it even smaller than the original by
> changing utoa_r() so that it takes the original buffer in a 3rd
> argument and emits the difference at the end as the length. This
> allows to always perform a tail call, though I'm not sure we really
> need it. 

Thomas