[PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[David Miller]

The grub printf code tries to treat 'long' and 'int' the same,
that doesn't work on (most) 64-bit platforms.

2009-04-11  David S. Miller  <davem@davemloft.net>

	* kern/misc.c (grub_ltoa): New function.
	(grub_vsprintf): Use it to format 'long' integers.
---
 kern/misc.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/kern/misc.c b/kern/misc.c
index 23eaa14..85a5d95 100644
--- a/kern/misc.c
+++ b/kern/misc.c
@@ -590,6 +590,31 @@ grub_itoa (char *str, int c, unsigned n)
   return p;
 }
 
+static char *
+grub_ltoa (char *str, int c, unsigned long n)
+{
+  unsigned long base = (c == 'x') ? 16 : 10;
+  char *p;
+
+  if ((long) n < 0 && c == 'd')
+    {
+      n = (unsigned) (-((long) n));
+      *str++ = '-';
+    }
+
+  p = str;
+  do
+    {
+      unsigned long d = n % base;
+      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
+    }
+  while (n /= base);
+  *p = 0;
+
+  grub_reverse (str);
+  return p;
+}
+
 /* Divide N by D, return the quotient, and store the remainder in *R.  */
 grub_uint64_t
 grub_divmod64 (grub_uint64_t n, grub_uint32_t d, grub_uint32_t *r)
@@ -790,12 +815,14 @@ grub_vsprintf (char *str, const char *fmt, va_list args)
 		  ll = va_arg (args, long long);
 		  grub_lltoa (tmp, c, ll);
 		}
+	      else if (longfmt)
+		{
+		  long l = va_arg (args, long);
+		  grub_ltoa (tmp, c, l);
+		}
 	      else
 		{
-		  if (longfmt)
-		    n = va_arg (args, long);
-		  else
-		    n = va_arg (args, int);
+		  n = va_arg (args, int);
 		  grub_itoa (tmp, c, n);
 		}
 	      if (! rightfill && grub_strlen (tmp) < format1)
-- 
1.6.2.2

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[Yoshinori K. Okuji]

On Saturday 11 April 2009 17:08:29 David Miller wrote:
> The grub printf code tries to treat 'long' and 'int' the same,
> that doesn't work on (most) 64-bit platforms.

Right. Good finding.

Regards,
Okuji

>
> 2009-04-11  David S. Miller  <davem@davemloft.net>
>
> 	* kern/misc.c (grub_ltoa): New function.
> 	(grub_vsprintf): Use it to format 'long' integers.
> ---
>  kern/misc.c |   35 +++++++++++++++++++++++++++++++----
>  1 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/kern/misc.c b/kern/misc.c
> index 23eaa14..85a5d95 100644
> --- a/kern/misc.c
> +++ b/kern/misc.c
> @@ -590,6 +590,31 @@ grub_itoa (char *str, int c, unsigned n)
>    return p;
>  }
>
> +static char *
> +grub_ltoa (char *str, int c, unsigned long n)
> +{
> +  unsigned long base = (c == 'x') ? 16 : 10;
> +  char *p;
> +
> +  if ((long) n < 0 && c == 'd')
> +    {
> +      n = (unsigned) (-((long) n));
> +      *str++ = '-';
> +    }
> +
> +  p = str;
> +  do
> +    {
> +      unsigned long d = n % base;
> +      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
> +    }
> +  while (n /= base);
> +  *p = 0;
> +
> +  grub_reverse (str);
> +  return p;
> +}
> +
>  /* Divide N by D, return the quotient, and store the remainder in *R.  */
>  grub_uint64_t
>  grub_divmod64 (grub_uint64_t n, grub_uint32_t d, grub_uint32_t *r)
> @@ -790,12 +815,14 @@ grub_vsprintf (char *str, const char *fmt, va_list
> args) ll = va_arg (args, long long);
>  		  grub_lltoa (tmp, c, ll);
>  		}
> +	      else if (longfmt)
> +		{
> +		  long l = va_arg (args, long);
> +		  grub_ltoa (tmp, c, l);
> +		}
>  	      else
>  		{
> -		  if (longfmt)
> -		    n = va_arg (args, long);
> -		  else
> -		    n = va_arg (args, int);
> +		  n = va_arg (args, int);
>  		  grub_itoa (tmp, c, n);
>  		}
>  	      if (! rightfill && grub_strlen (tmp) < format1)

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[David Miller]

From: "Yoshinori K. Okuji" <okuji@enbug.org>
Date: Sat, 11 Apr 2009 19:19:49 +0900

> On Saturday 11 April 2009 17:08:29 David Miller wrote:
>> The grub printf code tries to treat 'long' and 'int' the same,
>> that doesn't work on (most) 64-bit platforms.
> 
> Right. Good finding.

Committed.  Thanks for reviewing.

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[phcoder]

I do not agree with having separate grub_itoa/grub_ltoa/grub_lltoa in 
kernel where the size is critical. We should have only lltoa and use it 
in all contexts. I know because of divmod64 it's slower than ltoa or 
itoa but it's used only for output so speed isn't so important
David Miller wrote:
> From: "Yoshinori K. Okuji" <okuji@enbug.org>
> Date: Sat, 11 Apr 2009 19:19:49 +0900
> 
>> On Saturday 11 April 2009 17:08:29 David Miller wrote:
>>> The grub printf code tries to treat 'long' and 'int' the same,
>>> that doesn't work on (most) 64-bit platforms.
>> Right. Good finding.
> 
> Committed.  Thanks for reviewing.
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[Pavel Roskin]

On Sat, 2009-04-11 at 21:10 +0200, phcoder wrote:
> I do not agree with having separate grub_itoa/grub_ltoa/grub_lltoa in 
> kernel where the size is critical.

Nice catch!

>  We should have only lltoa and use it 
> in all contexts. I know because of divmod64 it's slower than ltoa or 
> itoa but it's used only for output so speed isn't so important

I agree.

-- 
Regards,
Pavel Roskin

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[David Miller]

From: phcoder <phcoder@gmail.com>
Date: Sat, 11 Apr 2009 21:10:26 +0200

> I do not agree with having separate grub_itoa/grub_ltoa/grub_lltoa in
> kernel where the size is critical. We should have only lltoa and use
> it in all contexts.

I look forward to your patch implementing this improvement :-)

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

David Miller wrote:
> From: phcoder <phcoder@gmail.com>
> Date: Sat, 11 Apr 2009 21:10:26 +0200
> 
>> I do not agree with having separate grub_itoa/grub_ltoa/grub_lltoa in
>> kernel where the size is critical. We should have only lltoa and use
>> it in all contexts.
> 
> I look forward to your patch implementing this improvement :-)
> 


-- 

Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: lltoa.diff --]
[-- Type: text/x-diff, Size: 2047 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 237c6f4..d701519 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2009-04-12  Vladimir Serbinenko	 <phcoder@gmail.com>
+
+	use grub_lltoa instead of grub_itoa and grub_ltoa for all purposes
+	
+	* kern/misc.c (grub_itoa): Removed function
+	(grub_ltoa): likewise
+	(grub_vsprintf): use grub_lltoa
+
 2009-04-12  David S. Miller  <davem@davemloft.net>
 
 	* kern/misc.c (grub_ltoa): Fix cast when handling negative
diff --git a/kern/misc.c b/kern/misc.c
index 5a12535..70abb33 100644
--- a/kern/misc.c
+++ b/kern/misc.c
@@ -565,56 +565,6 @@ grub_reverse (char *str)
     }
 }
 
-static char *
-grub_itoa (char *str, int c, unsigned n)
-{
-  unsigned base = (c == 'x') ? 16 : 10;
-  char *p;
-  
-  if ((int) n < 0 && c == 'd')
-    {
-      n = (unsigned) (-((int) n));
-      *str++ = '-';
-    }
-
-  p = str;
-  do
-    {
-      unsigned d = n % base;
-      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
-    }
-  while (n /= base);
-  *p = 0;
-
-  grub_reverse (str);
-  return p;
-}
-
-static char *
-grub_ltoa (char *str, int c, unsigned long n)
-{
-  unsigned long base = (c == 'x') ? 16 : 10;
-  char *p;
-
-  if ((long) n < 0 && c == 'd')
-    {
-      n = (unsigned long) (-((long) n));
-      *str++ = '-';
-    }
-
-  p = str;
-  do
-    {
-      unsigned long d = n % base;
-      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
-    }
-  while (n /= base);
-  *p = 0;
-
-  grub_reverse (str);
-  return p;
-}
-
 /* Divide N by D, return the quotient, and store the remainder in *R.  */
 grub_uint64_t
 grub_divmod64 (grub_uint64_t n, grub_uint32_t d, grub_uint32_t *r)
@@ -818,12 +768,12 @@ grub_vsprintf (char *str, const char *fmt, va_list args)
 	      else if (longfmt)
 		{
 		  long l = va_arg (args, long);
-		  grub_ltoa (tmp, c, l);
+		  grub_lltoa (tmp, c, l);
 		}
 	      else
 		{
 		  n = va_arg (args, int);
-		  grub_itoa (tmp, c, n);
+		  grub_lltoa (tmp, c, n);
 		}
 	      if (! rightfill && grub_strlen (tmp) < format1)
 		write_fill (zerofill, format1 - grub_strlen (tmp));

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[Isaac Dupree]

> -                 grub_ltoa (tmp, c, l);
> +                 grub_lltoa (tmp, c, l);
>                 }
>               else
>                 {
>                   n = va_arg (args, int);
> -                 grub_itoa (tmp, c, n);
> +                 grub_lltoa (tmp, c, n);

Do you think they deserve a comment saying why lltoa and that e.g. grub_ltoa 
would work equally well there except for space reasons?  (In case we ever 
decided to change that code back, it would be easier to figure out.)

-Isaac

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[Pavel Roskin]

On Sun, 2009-04-12 at 10:19 -0400, Isaac Dupree wrote:
> > -                 grub_ltoa (tmp, c, l);
> > +                 grub_lltoa (tmp, c, l);
> >                 }
> >               else
> >                 {
> >                   n = va_arg (args, int);
> > -                 grub_itoa (tmp, c, n);
> > +                 grub_lltoa (tmp, c, n);
> 
> Do you think they deserve a comment saying why lltoa and that e.g. grub_ltoa 
> would work equally well there except for space reasons?  (In case we ever 
> decided to change that code back, it would be easier to figure out.)

If somebody wants to change the code back, that person would need to
justify adding two functions to kern/misc.c, which is part of the GRUB
core and should be kept minimal.  I don't think it will be easy to
justify.

-- 
Regards,
Pavel Roskin

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[Pavel Roskin]

On Sun, 2009-04-12 at 14:46 +0200, phcoder wrote:
> -                 grub_itoa (tmp, c, n);
> +                 grub_lltoa (tmp, c, n);

Wrong.  For "x" and "u" specifiers, the value is unsigned int, but n is
signed int.  If we extend signed into to unsigned long long, we get a
sign extension that would be incorrect.

Suppose the argument is 0xffffffff.  n would be -1.  grub_lltoa() would
get (unsigned long long)-1, that is 0xffffffffffffffff.

If the type is going to be converted, we need to treat signed and
unsigned values separately.

-- 
Regards,
Pavel Roskin

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[Pavel Roskin]

On Sun, 2009-04-12 at 17:33 -0400, Pavel Roskin wrote:

> If the type is going to be converted, we need to treat signed and
> unsigned values separately.

This is compile tested only, to show what I mean.  The size of
kernel.img is reduced from 31220 to 31124.  The code looks long, but I'm
sure an optimizing compiler can simplify it well.  Variable names are
changed for readability.

Index: kern/misc.c
===================================================================
--- kern/misc.c	(revision 2087)
+++ kern/misc.c	(working copy)
@@ -565,56 +565,6 @@
     }
 }
 
-static char *
-grub_itoa (char *str, int c, unsigned n)
-{
-  unsigned base = (c == 'x') ? 16 : 10;
-  char *p;
-  
-  if ((int) n < 0 && c == 'd')
-    {
-      n = (unsigned) (-((int) n));
-      *str++ = '-';
-    }
-
-  p = str;
-  do
-    {
-      unsigned d = n % base;
-      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
-    }
-  while (n /= base);
-  *p = 0;
-
-  grub_reverse (str);
-  return p;
-}
-
-static char *
-grub_ltoa (char *str, int c, unsigned long n)
-{
-  unsigned long base = (c == 'x') ? 16 : 10;
-  char *p;
-
-  if ((long) n < 0 && c == 'd')
-    {
-      n = (unsigned long) (-((long) n));
-      *str++ = '-';
-    }
-
-  p = str;
-  do
-    {
-      unsigned long d = n % base;
-      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
-    }
-  while (n /= base);
-  *p = 0;
-
-  grub_reverse (str);
-  return p;
-}
-
 /* Divide N by D, return the quotient, and store the remainder in *R.  */
 grub_uint64_t
 grub_divmod64 (grub_uint64_t n, grub_uint32_t d, grub_uint32_t *r)
@@ -807,23 +757,36 @@
 	      /* fall through */
 	    case 'x':
 	    case 'u':
+	      if (longlongfmt)
+		{
+		  unsigned long long ull = va_arg (args, unsigned long long);
+		  grub_lltoa (tmp, c, ull);
+		}
+	      else if (longfmt)
+		{
+		  unsigned long ul = va_arg (args, unsigned long);
+		  grub_lltoa (tmp, c, ul);
+		}
+	      else
+		{
+		  unsigned int ui = va_arg (args, unsigned int);
+		  grub_lltoa (tmp, c, ui);
+		}
 	    case 'd':
 	      if (longlongfmt)
 		{
-		  long long ll;
-
-		  ll = va_arg (args, long long);
-		  grub_lltoa (tmp, c, ll);
+		  long long sll = va_arg (args, long long);
+		  grub_lltoa (tmp, c, sll);
 		}
 	      else if (longfmt)
 		{
-		  long l = va_arg (args, long);
-		  grub_ltoa (tmp, c, l);
+		  long sl = va_arg (args, long);
+		  grub_lltoa (tmp, c, sl);
 		}
 	      else
 		{
-		  n = va_arg (args, int);
-		  grub_itoa (tmp, c, n);
+		  int si = va_arg (args, int);
+		  grub_lltoa (tmp, c, si);
 		}
 	      if (! rightfill && grub_strlen (tmp) < format1)
 		write_fill (zerofill, format1 - grub_strlen (tmp));


-- 
Regards,
Pavel Roskin

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[phcoder]

[-- Attachment #1: Type: text/plain, Size: 3898 bytes --]

I already understood what you meant in first mail. Sorry for not paying 
attention to this detail. Here is my proposition. IT decreases the size 
from 31224 to 31068 bytes. I tested it with following input
   grub_printf ("Hello World: %d %ld %lld %x %lx %llx %u %lu %llu\n",
                0xffffffff, 0xffffffff, 0xffffffffffffffffLL,
                0xffffffff, 0xffffffff, 0xffffffffffffffffLL,
                0xffffffff, 0xffffffff, 0xffffffffffffffffLL);
   grub_printf ("Hello World: %d %ld %lld %x %lx %llx %u %lu %llu\n",
                0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL,
                0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL,
                0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL);
Output was:
Hello World: -1 -1 -1 ffffffff ffffffff ffffffffffffffff 4294967295 
4294967295 18446744073709551615
Hello World: 268435455 268435455 1152921504606846975 fffffff fffffff 
fffffffffffffff 268435455 268435455 1152921504606846975

Pavel Roskin wrote:
> On Sun, 2009-04-12 at 17:33 -0400, Pavel Roskin wrote:
> 
>> If the type is going to be converted, we need to treat signed and
>> unsigned values separately.
> 
> This is compile tested only, to show what I mean.  The size of
> kernel.img is reduced from 31220 to 31124.  The code looks long, but I'm
> sure an optimizing compiler can simplify it well.  Variable names are
> changed for readability.
> 
> Index: kern/misc.c
> ===================================================================
> --- kern/misc.c	(revision 2087)
> +++ kern/misc.c	(working copy)
> @@ -565,56 +565,6 @@
>      }
>  }
>  
> -static char *
> -grub_itoa (char *str, int c, unsigned n)
> -{
> -  unsigned base = (c == 'x') ? 16 : 10;
> -  char *p;
> -  
> -  if ((int) n < 0 && c == 'd')
> -    {
> -      n = (unsigned) (-((int) n));
> -      *str++ = '-';
> -    }
> -
> -  p = str;
> -  do
> -    {
> -      unsigned d = n % base;
> -      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
> -    }
> -  while (n /= base);
> -  *p = 0;
> -
> -  grub_reverse (str);
> -  return p;
> -}
> -
> -static char *
> -grub_ltoa (char *str, int c, unsigned long n)
> -{
> -  unsigned long base = (c == 'x') ? 16 : 10;
> -  char *p;
> -
> -  if ((long) n < 0 && c == 'd')
> -    {
> -      n = (unsigned long) (-((long) n));
> -      *str++ = '-';
> -    }
> -
> -  p = str;
> -  do
> -    {
> -      unsigned long d = n % base;
> -      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
> -    }
> -  while (n /= base);
> -  *p = 0;
> -
> -  grub_reverse (str);
> -  return p;
> -}
> -
>  /* Divide N by D, return the quotient, and store the remainder in *R.  */
>  grub_uint64_t
>  grub_divmod64 (grub_uint64_t n, grub_uint32_t d, grub_uint32_t *r)
> @@ -807,23 +757,36 @@
>  	      /* fall through */
>  	    case 'x':
>  	    case 'u':
> +	      if (longlongfmt)
> +		{
> +		  unsigned long long ull = va_arg (args, unsigned long long);
> +		  grub_lltoa (tmp, c, ull);
> +		}
> +	      else if (longfmt)
> +		{
> +		  unsigned long ul = va_arg (args, unsigned long);
> +		  grub_lltoa (tmp, c, ul);
> +		}
> +	      else
> +		{
> +		  unsigned int ui = va_arg (args, unsigned int);
> +		  grub_lltoa (tmp, c, ui);
> +		}
>  	    case 'd':
>  	      if (longlongfmt)
>  		{
> -		  long long ll;
> -
> -		  ll = va_arg (args, long long);
> -		  grub_lltoa (tmp, c, ll);
> +		  long long sll = va_arg (args, long long);
> +		  grub_lltoa (tmp, c, sll);
>  		}
>  	      else if (longfmt)
>  		{
> -		  long l = va_arg (args, long);
> -		  grub_ltoa (tmp, c, l);
> +		  long sl = va_arg (args, long);
> +		  grub_lltoa (tmp, c, sl);
>  		}
>  	      else
>  		{
> -		  n = va_arg (args, int);
> -		  grub_itoa (tmp, c, n);
> +		  int si = va_arg (args, int);
> +		  grub_lltoa (tmp, c, si);
>  		}
>  	      if (! rightfill && grub_strlen (tmp) < format1)
>  		write_fill (zerofill, format1 - grub_strlen (tmp));
> 
> 


-- 

Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: lltoa.diff --]
[-- Type: text/x-diff, Size: 2907 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 237c6f4..d701519 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2009-04-13  Vladimir Serbinenko	 <phcoder@gmail.com>
+
+	use grub_lltoa instead of grub_itoa and grub_ltoa for all purposes
+	
+	* kern/misc.c (grub_itoa): Removed function
+	(grub_ltoa): likewise
+	(grub_vsprintf): use grub_lltoa
+
 2009-04-12  David S. Miller  <davem@davemloft.net>
 
 	* kern/misc.c (grub_ltoa): Fix cast when handling negative
diff --git a/kern/misc.c b/kern/misc.c
index 5a12535..4c71ca8 100644
--- a/kern/misc.c
+++ b/kern/misc.c
@@ -565,56 +565,6 @@ grub_reverse (char *str)
     }
 }
 
-static char *
-grub_itoa (char *str, int c, unsigned n)
-{
-  unsigned base = (c == 'x') ? 16 : 10;
-  char *p;
-  
-  if ((int) n < 0 && c == 'd')
-    {
-      n = (unsigned) (-((int) n));
-      *str++ = '-';
-    }
-
-  p = str;
-  do
-    {
-      unsigned d = n % base;
-      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
-    }
-  while (n /= base);
-  *p = 0;
-
-  grub_reverse (str);
-  return p;
-}
-
-static char *
-grub_ltoa (char *str, int c, unsigned long n)
-{
-  unsigned long base = (c == 'x') ? 16 : 10;
-  char *p;
-
-  if ((long) n < 0 && c == 'd')
-    {
-      n = (unsigned long) (-((long) n));
-      *str++ = '-';
-    }
-
-  p = str;
-  do
-    {
-      unsigned long d = n % base;
-      *p++ = (d > 9) ? d + 'a' - 10 : d + '0';
-    }
-  while (n /= base);
-  *p = 0;
-
-  grub_reverse (str);
-  return p;
-}
-
 /* Divide N by D, return the quotient, and store the remainder in *R.  */
 grub_uint64_t
 grub_divmod64 (grub_uint64_t n, grub_uint32_t d, grub_uint32_t *r)
@@ -746,6 +696,7 @@ grub_vsprintf (char *str, const char *fmt, va_list args)
 	  int n;
 	  int longfmt = 0;
 	  int longlongfmt = 0;
+	  int unsig = 0;
 
 	  if (*fmt && *fmt =='-')
 	    {
@@ -804,9 +755,11 @@ grub_vsprintf (char *str, const char *fmt, va_list args)
 	      write_str ("0x");
 	      c = 'x';
 	      longlongfmt |= (sizeof (void *) == sizeof (long long));
-	      /* fall through */
+	      /* Fall through. */
 	    case 'x':
 	    case 'u':
+	      unsig = 1;
+	      /* Fall through. */
 	    case 'd':
 	      if (longlongfmt)
 		{
@@ -815,15 +768,25 @@ grub_vsprintf (char *str, const char *fmt, va_list args)
 		  ll = va_arg (args, long long);
 		  grub_lltoa (tmp, c, ll);
 		}
+	      else if (longfmt && unsig)
+		{
+		  unsigned long l = va_arg (args, unsigned long);
+		  grub_lltoa (tmp, c, l);
+		}
 	      else if (longfmt)
 		{
 		  long l = va_arg (args, long);
-		  grub_ltoa (tmp, c, l);
+		  grub_lltoa (tmp, c, l);
+		}
+	      else if (unsig)
+		{
+		  unsigned u = va_arg (args, unsigned);
+		  grub_lltoa (tmp, c, u);
 		}
 	      else
 		{
 		  n = va_arg (args, int);
-		  grub_itoa (tmp, c, n);
+		  grub_lltoa (tmp, c, n);
 		}
 	      if (! rightfill && grub_strlen (tmp) < format1)
 		write_fill (zerofill, format1 - grub_strlen (tmp));

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[Pavel Roskin]

On Mon, 2009-04-13 at 00:19 +0200, phcoder wrote:
> I already understood what you meant in first mail. Sorry for not paying 
> attention to this detail. Here is my proposition. IT decreases the size 
> from 31224 to 31068 bytes. I tested it with following input
>    grub_printf ("Hello World: %d %ld %lld %x %lx %llx %u %lu %llu\n",
>                 0xffffffff, 0xffffffff, 0xffffffffffffffffLL,
>                 0xffffffff, 0xffffffff, 0xffffffffffffffffLL,
>                 0xffffffff, 0xffffffff, 0xffffffffffffffffLL);
>    grub_printf ("Hello World: %d %ld %lld %x %lx %llx %u %lu %llu\n",
>                 0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL,
>                 0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL,
>                 0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL);
> Output was:
> Hello World: -1 -1 -1 ffffffff ffffffff ffffffffffffffff 4294967295 
> 4294967295 18446744073709551615
> Hello World: 268435455 268435455 1152921504606846975 fffffff fffffff 
> fffffffffffffff 268435455 268435455 1152921504606846975

Thanks for the patch and for the test!  You patch doesn't look as
pedantic as mine, but apparently the compiler can optimize you code
better.  I have no objections to your patch.

-- 
Regards,
Pavel Roskin

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[phcoder]

commited
Pavel Roskin wrote:
> On Mon, 2009-04-13 at 00:19 +0200, phcoder wrote:
>> I already understood what you meant in first mail. Sorry for not paying 
>> attention to this detail. Here is my proposition. IT decreases the size 
>> from 31224 to 31068 bytes. I tested it with following input
>>    grub_printf ("Hello World: %d %ld %lld %x %lx %llx %u %lu %llu\n",
>>                 0xffffffff, 0xffffffff, 0xffffffffffffffffLL,
>>                 0xffffffff, 0xffffffff, 0xffffffffffffffffLL,
>>                 0xffffffff, 0xffffffff, 0xffffffffffffffffLL);
>>    grub_printf ("Hello World: %d %ld %lld %x %lx %llx %u %lu %llu\n",
>>                 0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL,
>>                 0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL,
>>                 0x0fffffff, 0x0fffffff, 0x0fffffffffffffffLL);
>> Output was:
>> Hello World: -1 -1 -1 ffffffff ffffffff ffffffffffffffff 4294967295 
>> 4294967295 18446744073709551615
>> Hello World: 268435455 268435455 1152921504606846975 fffffff fffffff 
>> fffffffffffffff 268435455 268435455 1152921504606846975
> 
> Thanks for the patch and for the test!  You patch doesn't look as
> pedantic as mine, but apparently the compiler can optimize you code
> better.  I have no objections to your patch.
> 


-- 

Regards
Vladimir 'phcoder' Serbinenko

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[Pavel Roskin]

On Sat, 2009-04-11 at 01:08 -0700, David Miller wrote:
> The grub printf code tries to treat 'long' and 'int' the same,
> that doesn't work on (most) 64-bit platforms.

> +      n = (unsigned) (-((long) n));

"unsigned" means "unsigned int".  You need "unsigned long" here.

It's probably irrelevant if we kill grub_ltoa(), but anyway, let's be
careful.

-- 
Regards,
Pavel Roskin

Re: [PATCH]: grub: Fix handling of long printf arguments on 64-bit.

[David Miller]

From: Pavel Roskin <proski@gnu.org>
Date: Sun, 12 Apr 2009 02:39:22 -0400

> On Sat, 2009-04-11 at 01:08 -0700, David Miller wrote:
>> The grub printf code tries to treat 'long' and 'int' the same,
>> that doesn't work on (most) 64-bit platforms.
> 
>> +      n = (unsigned) (-((long) n));
> 
> "unsigned" means "unsigned int".  You need "unsigned long" here.

Thanks, I'll fix this as follows.

	* kern/misc.c (grub_ltoa): Fix cast when handling negative
	values.  Noticed by Pavel Roskin.

--- kern/misc.c	(revision 2086)
+++ kern/misc.c	(working copy)
@@ -598,7 +598,7 @@
 
   if ((long) n < 0 && c == 'd')
     {
-      n = (unsigned) (-((long) n));
+      n = (unsigned long) (-((long) n));
       *str++ = '-';
     }