Bug#584474: FTBFS: usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’

Bug#584474: FTBFS: usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’

[sean finney]

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

Source: grub2
Version: 1.98+20100602-1
Severity: serious
Tags: upstream patch
Justification: FTBFS

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

while investigated an unrelated bug, i noticed that the latest snapshot
uploaded to debian fails with the following error:

gcc-4.4 -Idisk -I/home/seanius/grub2-1.98+20100602/disk -I/home/seanius/grub2-1.98+20100602/include -I. -I./include -Wall -W  -ffreestanding  -Os -DGRUB_MACHINE_EMU=1 -DMACHINE=X86_64_EMU -Wall -W -Wshadow -Wpointer-arith -Wmissing-prototypes                -Wundef -Wstrict-prototypes -g -fno-dwarf2-cfi-asm -m64 -fno-stack-protector -mno-stack-arg-probe -Werror -DGRUB_TARGET_NO_MODULES=1 -DUSE_ASCII_FAILBACK=1  -DGRUB_FILE=\"disk/usbms.c\" -MD -c -o usbms_mod-disk_usbms.o /home/seanius/grub2-1.98+20100602/disk/usbms.c
cc1: warnings being treated as errors
/home/seanius/grub2-1.98+20100602/disk/usbms.c: In function ‘grub_usbms_transfer’:
/home/seanius/grub2-1.98+20100602/disk/usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’, but argument 5 has type ‘grub_size_t’
/home/seanius/grub2-1.98+20100602/disk/usbms.c:333: error: format ‘%02x’ expects type ‘unsigned int’, but argument 5 has type ‘grub_size_t’
make[1]: *** [usbms_mod-disk_usbms.o] Error 1

the two lines in question are both debug printf type statements.
therefore, instead of doing something complicated to get the format
string to match with the size of grub_size_t (which i'm guessing could
vary based on the platform), i have created a patch that simply casts the
parameter in question as unsigned to match the format string instead (which
only shows two digits anyway).


	sean

- -- System Information:
Debian Release: squeeze/sid
  APT prefers unstable
  APT policy: (500, 'unstable'), (500, 'testing'), (1, 'experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 2.6.34-rc5minime-00783-gbad884d (SMP w/2 CPU cores)
Locale: LANG=en_US.utf8, LC_CTYPE=en_US.utf8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iD4DBQFMCAImynjLPm522B0RAsldAJjTMs+9qU0jot0h06FdvMFG8nQCAJ0f0c0X
Fu2Hnjr1OrS/XiOazgU1bQ==
=+y8T
-----END PGP SIGNATURE-----

[-- Attachment #2: 911_usbms_ftbfs.diff --]
[-- Type: text/x-c, Size: 846 bytes --]

--- grub2-1.98+20100602.orig/disk/usbms.c
+++ grub2-1.98+20100602/disk/usbms.c
@@ -312,7 +312,7 @@ grub_usbms_transfer (struct grub_scsi *s
       grub_dprintf ("usb", "buf:\n");
       if (size <= 64)
         for (i=0; i<size; i++)
-          grub_dprintf ("usb", "0x%02x: 0x%02x\n", i, buf[i]);
+          grub_dprintf ("usb", "0x%02x: 0x%02x\n", (unsigned)i, buf[i]);
       else
           grub_dprintf ("usb", "Too much data for debug print...\n");
     }
@@ -330,7 +330,7 @@ grub_usbms_transfer (struct grub_scsi *s
       /* Debug print of sent data. */
       if (size <= 256)
         for (i=0; i<size; i++)
-          grub_dprintf ("usb", "0x%02x: 0x%02x\n", i, buf[i]);
+          grub_dprintf ("usb", "0x%02x: 0x%02x\n", (unsigned)i, buf[i]);
       else
           grub_dprintf ("usb", "Too much data for debug print...\n");
     }

Re: Bug#584474: FTBFS: usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’

[Colin Watson]

On Thu, Jun 03, 2010 at 09:27:38PM +0200, sean finney wrote:
> while investigated an unrelated bug, i noticed that the latest snapshot
> uploaded to debian fails with the following error:
> 
> gcc-4.4 -Idisk -I/home/seanius/grub2-1.98+20100602/disk -I/home/seanius/grub2-1.98+20100602/include -I. -I./include -Wall -W  -ffreestanding  -Os -DGRUB_MACHINE_EMU=1 -DMACHINE=X86_64_EMU -Wall -W -Wshadow -Wpointer-arith -Wmissing-prototypes                -Wundef -Wstrict-prototypes -g -fno-dwarf2-cfi-asm -m64 -fno-stack-protector -mno-stack-arg-probe -Werror -DGRUB_TARGET_NO_MODULES=1 -DUSE_ASCII_FAILBACK=1  -DGRUB_FILE=\"disk/usbms.c\" -MD -c -o usbms_mod-disk_usbms.o /home/seanius/grub2-1.98+20100602/disk/usbms.c
> cc1: warnings being treated as errors
> /home/seanius/grub2-1.98+20100602/disk/usbms.c: In function ‘grub_usbms_transfer’:
> /home/seanius/grub2-1.98+20100602/disk/usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’, but argument 5 has type ‘grub_size_t’
> /home/seanius/grub2-1.98+20100602/disk/usbms.c:333: error: format ‘%02x’ expects type ‘unsigned int’, but argument 5 has type ‘grub_size_t’
> make[1]: *** [usbms_mod-disk_usbms.o] Error 1
> 
> the two lines in question are both debug printf type statements.
> therefore, instead of doing something complicated to get the format
> string to match with the size of grub_size_t (which i'm guessing could
> vary based on the platform), i have created a patch that simply casts the
> parameter in question as unsigned to match the format string instead (which
> only shows two digits anyway).

As Vladimir pointed out on IRC: no, that's a minimum field width, not a
precision.  It shows *at least* two digits.

I think I'd prefer to add a grub_printf length modifier to print
grub_size_t values.  'z' is standard for size_t, so it seems reasonable
to use that, it can be done in very little code, and it saves on ugly
and inaccurate casts.  Vladimir, what do you think?

(If this is unacceptable for whatever reason, then the proper fix would
be to cast to (unsigned long long) and to use %02llx - but I prefer
having an accurate length modifier, really.)

2010-06-04  Colin Watson  <cjwatson@ubuntu.com>

	* kern/misc.c (grub_vsnprintf_real): Support 'z' length
	modifier, for grub_size_t.
	* disk/usbms.c (grub_usbms_transfer): Use it.

=== modified file 'disk/usbms.c'
--- disk/usbms.c	2010-06-01 00:10:19 +0000
+++ disk/usbms.c	2010-06-04 12:04:11 +0000
@@ -312,7 +312,7 @@ grub_usbms_transfer (struct grub_scsi *s
       grub_dprintf ("usb", "buf:\n");
       if (size <= 64)
         for (i=0; i<size; i++)
-          grub_dprintf ("usb", "0x%02x: 0x%02x\n", i, buf[i]);
+          grub_dprintf ("usb", "0x%02zx: 0x%02x\n", i, buf[i]);
       else
           grub_dprintf ("usb", "Too much data for debug print...\n");
     }
@@ -330,7 +330,7 @@ grub_usbms_transfer (struct grub_scsi *s
       /* Debug print of sent data. */
       if (size <= 256)
         for (i=0; i<size; i++)
-          grub_dprintf ("usb", "0x%02x: 0x%02x\n", i, buf[i]);
+          grub_dprintf ("usb", "0x%02zx: 0x%02x\n", i, buf[i]);
       else
           grub_dprintf ("usb", "Too much data for debug print...\n");
     }

=== modified file 'kern/misc.c'
--- kern/misc.c	2010-05-28 13:48:45 +0000
+++ kern/misc.c	2010-06-04 12:04:11 +0000
@@ -688,6 +688,7 @@ grub_vsnprintf_real (char *str, grub_siz
 	  int n;
 	  int longfmt = 0;
 	  int longlongfmt = 0;
+	  int sizetfmt = 0;
 	  int unsig = 0;
 
 	  if (*fmt && *fmt =='-')
@@ -740,6 +741,11 @@ grub_vsnprintf_real (char *str, grub_siz
 		  c = *fmt++;
 		}
 	    }
+	  else if (c == 'z')
+	    {
+	      sizetfmt = 1;
+	      c = *fmt++;
+	    }
 
 	  switch (c)
 	    {
@@ -770,6 +776,11 @@ grub_vsnprintf_real (char *str, grub_siz
 		  long l = va_arg (args, long);
 		  grub_lltoa (tmp, c, l);
 		}
+	      else if (sizetfmt)
+		{
+		  grub_size_t sz = va_arg (args, grub_size_t);
+		  grub_lltoa (tmp, c, sz);
+		}
 	      else if (unsig)
 		{
 		  unsigned u = va_arg (args, unsigned);

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Bug#584474: FTBFS: usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 06/04/2010 02:09 PM, Colin Watson wrote:
> On Thu, Jun 03, 2010 at 09:27:38PM +0200, sean finney wrote:
>   
>> while investigated an unrelated bug, i noticed that the latest snapshot
>> uploaded to debian fails with the following error:
>>
>> gcc-4.4 -Idisk -I/home/seanius/grub2-1.98+20100602/disk -I/home/seanius/grub2-1.98+20100602/include -I. -I./include -Wall -W  -ffreestanding  -Os -DGRUB_MACHINE_EMU=1 -DMACHINE=X86_64_EMU -Wall -W -Wshadow -Wpointer-arith -Wmissing-prototypes                -Wundef -Wstrict-prototypes -g -fno-dwarf2-cfi-asm -m64 -fno-stack-protector -mno-stack-arg-probe -Werror -DGRUB_TARGET_NO_MODULES=1 -DUSE_ASCII_FAILBACK=1  -DGRUB_FILE=\"disk/usbms.c\" -MD -c -o usbms_mod-disk_usbms.o /home/seanius/grub2-1.98+20100602/disk/usbms.c
>> cc1: warnings being treated as errors
>> /home/seanius/grub2-1.98+20100602/disk/usbms.c: In function ‘grub_usbms_transfer’:
>> /home/seanius/grub2-1.98+20100602/disk/usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’, but argument 5 has type ‘grub_size_t’
>> /home/seanius/grub2-1.98+20100602/disk/usbms.c:333: error: format ‘%02x’ expects type ‘unsigned int’, but argument 5 has type ‘grub_size_t’
>> make[1]: *** [usbms_mod-disk_usbms.o] Error 1
>>
>> the two lines in question are both debug printf type statements.
>> therefore, instead of doing something complicated to get the format
>> string to match with the size of grub_size_t (which i'm guessing could
>> vary based on the platform), i have created a patch that simply casts the
>> parameter in question as unsigned to match the format string instead (which
>> only shows two digits anyway).
>>     
> As Vladimir pointed out on IRC: no, that's a minimum field width, not a
> precision.  It shows *at least* two digits.
>
> I think I'd prefer to add a grub_printf length modifier to print
> grub_size_t values.  'z' is standard for size_t, so it seems reasonable
> to use that, it can be done in very little code, and it saves on ugly
> and inaccurate casts.  Vladimir, what do you think?
>
>   
grub_size_t is always of the same length as long. Perhaps we should
define it as unsigned long, use "%lx" and get rid of useless difference
on 32-bit and 64-bit platforms.
Are there any reasons not to do this way?
Alternatively %zx can be aliased to %lx in one code line.
> (If this is unacceptable for whatever reason, then the proper fix would
> be to cast to (unsigned long long) and to use %02llx - but I prefer
> having an accurate length modifier, really.)
>
> 2010-06-04  Colin Watson  <cjwatson@ubuntu.com>
>
> 	* kern/misc.c (grub_vsnprintf_real): Support 'z' length
> 	modifier, for grub_size_t.
> 	* disk/usbms.c (grub_usbms_transfer): Use it.
>
> === modified file 'disk/usbms.c'
> --- disk/usbms.c	2010-06-01 00:10:19 +0000
> +++ disk/usbms.c	2010-06-04 12:04:11 +0000
> @@ -312,7 +312,7 @@ grub_usbms_transfer (struct grub_scsi *s
>        grub_dprintf ("usb", "buf:\n");
>        if (size <= 64)
>          for (i=0; i<size; i++)
> -          grub_dprintf ("usb", "0x%02x: 0x%02x\n", i, buf[i]);
> +          grub_dprintf ("usb", "0x%02zx: 0x%02x\n", i, buf[i]);
>        else
>            grub_dprintf ("usb", "Too much data for debug print...\n");
>      }
> @@ -330,7 +330,7 @@ grub_usbms_transfer (struct grub_scsi *s
>        /* Debug print of sent data. */
>        if (size <= 256)
>          for (i=0; i<size; i++)
> -          grub_dprintf ("usb", "0x%02x: 0x%02x\n", i, buf[i]);
> +          grub_dprintf ("usb", "0x%02zx: 0x%02x\n", i, buf[i]);
>        else
>            grub_dprintf ("usb", "Too much data for debug print...\n");
>      }
>
> === modified file 'kern/misc.c'
> --- kern/misc.c	2010-05-28 13:48:45 +0000
> +++ kern/misc.c	2010-06-04 12:04:11 +0000
> @@ -688,6 +688,7 @@ grub_vsnprintf_real (char *str, grub_siz
>  	  int n;
>  	  int longfmt = 0;
>  	  int longlongfmt = 0;
> +	  int sizetfmt = 0;
>  	  int unsig = 0;
>  
>  	  if (*fmt && *fmt =='-')
> @@ -740,6 +741,11 @@ grub_vsnprintf_real (char *str, grub_siz
>  		  c = *fmt++;
>  		}
>  	    }
> +	  else if (c == 'z')
> +	    {
> +	      sizetfmt = 1;
> +	      c = *fmt++;
> +	    }
>  
>  	  switch (c)
>  	    {
> @@ -770,6 +776,11 @@ grub_vsnprintf_real (char *str, grub_siz
>  		  long l = va_arg (args, long);
>  		  grub_lltoa (tmp, c, l);
>  		}
> +	      else if (sizetfmt)
> +		{
> +		  grub_size_t sz = va_arg (args, grub_size_t);
> +		  grub_lltoa (tmp, c, sz);
> +		}
>  	      else if (unsig)
>  		{
>  		  unsigned u = va_arg (args, unsigned);
>
> Thanks,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

Re: Bug#584474: FTBFS: usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’

[Colin Watson]

On Sat, Jun 05, 2010 at 12:17:57AM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 06/04/2010 02:09 PM, Colin Watson wrote:
> > As Vladimir pointed out on IRC: no, that's a minimum field width, not a
> > precision.  It shows *at least* two digits.
> >
> > I think I'd prefer to add a grub_printf length modifier to print
> > grub_size_t values.  'z' is standard for size_t, so it seems reasonable
> > to use that, it can be done in very little code, and it saves on ugly
> > and inaccurate casts.  Vladimir, what do you think?
> 
> grub_size_t is always of the same length as long. Perhaps we should
> define it as unsigned long, use "%lx" and get rid of useless difference
> on 32-bit and 64-bit platforms.
> Are there any reasons not to do this way?

Good point; I hadn't spotted that.  Mostly clarity, I think.  If it
comes out as unsigned long anyway then it doesn't make any difference in
the machine representation, but the different definitions do seem useful
for clarity.  If we change that (I had to read over it a few times to
convince myself), then we should comment it very carefully.  A comment
near the #error if GRUB_CPU_SIZEOF_VOID_P != GRUB_CPU_SIZEOF_LONG to say
that we'll need to change the grub_size_t definition if we ever support
an architecture where void * isn't the same length as long would also be
good, I think.

There is no assertion that GRUB_TARGET_SIZEOF_VOID_P ==
GRUB_TARGET_SIZEOF_LONG, although it's currently true for all targets.
Should there be?

> Alternatively %zx can be aliased to %lx in one code line.

I do like being explicit about the length modifier but I understand that
kern/misc.c needs to be as small as possible, so I'm not going to be
precious about it.  How about a compromise along the lines of C99's
<inttypes.h> to reduce the probability of introducing errors when we add
new targets?  Following that (unfortunately ugly) pattern, we'd get
something like:

  #define PRIxGRUB_SIZE "lx"

If you think it's unlikely that grub_size_t will ever need to be
anything other than unsigned long even on future targets, then maybe we
don't need to worry about that.

Thanks,

-- 
Colin Watson                                       [cjwatson@ubuntu.com]

Re: Bug#584474: FTBFS: usbms.c:315: error: format ‘%02x’ expects type ‘unsigned int’

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 06/05/2010 12:40 AM, Colin Watson wrote:
> On Sat, Jun 05, 2010 at 12:17:57AM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>   
>> On 06/04/2010 02:09 PM, Colin Watson wrote:
>>     
>>> As Vladimir pointed out on IRC: no, that's a minimum field width, not a
>>> precision.  It shows *at least* two digits.
>>>
>>> I think I'd prefer to add a grub_printf length modifier to print
>>> grub_size_t values.  'z' is standard for size_t, so it seems reasonable
>>> to use that, it can be done in very little code, and it saves on ugly
>>> and inaccurate casts.  Vladimir, what do you think?
>>>       
>> grub_size_t is always of the same length as long. Perhaps we should
>> define it as unsigned long, use "%lx" and get rid of useless difference
>> on 32-bit and 64-bit platforms.
>> Are there any reasons not to do this way?
>>     
> Good point; I hadn't spotted that.  Mostly clarity, I think.  If it
> comes out as unsigned long anyway then it doesn't make any difference in
> the machine representation, but the different definitions do seem useful
> for clarity.  
Of course, I've never suggested an elimination of grub_size_t only doing
like:
typedef unsigned long grub_size_t;
> If we change that (I had to read over it a few times to
> convince myself), then we should comment it very carefully.  A comment
> near the #error if GRUB_CPU_SIZEOF_VOID_P != GRUB_CPU_SIZEOF_LONG to say
> that we'll need to change the grub_size_t definition if we ever support
> an architecture where void * isn't the same length as long would also be
> good, I think.
>   
Ok. We can do the same for grub_addr_t too.
> There is no assertion that GRUB_TARGET_SIZEOF_VOID_P ==
> GRUB_TARGET_SIZEOF_LONG, although it's currently true for all targets.
> Should there be?
>
>   
It can be added but currently GRUB_CPU_SIZEOF_* changes between TARGET_*
and HOST_* so if sizes mismatch either util or target part won't compile.
>> Alternatively %zx can be aliased to %lx in one code line.
>>     
> I do like being explicit about the length modifier but I understand that
> kern/misc.c needs to be as small as possible, so I'm not going to be
> precious about it.  How about a compromise along the lines of C99's
> <inttypes.h> to reduce the probability of introducing errors when we add
> new targets?  Following that (unfortunately ugly) pattern, we'd get
> something like:
>
>   #define PRIxGRUB_SIZE "lx"
>   
It's a good idea. Since it will be something heavily used perhaps it's
better to make an exception to usual GRUB style and shorten it somehow?
But if we do e.g. PRIxG_SIZE we may end up with conflicts with other
headers in utils
> If you think it's unlikely that grub_size_t will ever need to be
> anything other than unsigned long even on future targets, then maybe we
> don't need to worry about that.
>
>   
Actualy it's not so much of a "target" as a "compiler-target" pair.
Different compilers pick different machine-available sizes on same
platform for the same type of int.
> Thanks,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]