[PATCH v2] declance: Fix 64-bit compilation warnings

[PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

This fixes compiler warnings:

drivers/net/ethernet/amd/declance.c: In function 'lance_init_ring':
drivers/net/ethernet/amd/declance.c:478: warning: format '%8.8x' expects type 'unsigned int', but argument 3 has type 'long unsigned int'
drivers/net/ethernet/amd/declance.c:487: warning: format '%8.8x' expects type 'unsigned int', but argument 3 has type 'long unsigned int'
drivers/net/ethernet/amd/declance.c:503: warning: cast from pointer to integer of different size
drivers/net/ethernet/amd/declance.c:520: warning: cast from pointer to integer of different size

in 64-bit compilation.  Where the value printed is an offset (whose range 
will always fit) the cast uses a 32-bit type, otherwise, where it is a 
host memory address, the `long' type is used and the width of the number 
printed adjusted according to the size of this type.

Tested with both 32-bit and 64-bit compilation, as well as at the run time 
(with the debug messages affected enabled).

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
 Update from v1 fixes issues with the field width requested observed by 
Joe (thanks!) and myself.  Please apply.

  Maciej

linux-declance-desc-printk.patch
Index: linux-20140623-4maxp64/drivers/net/ethernet/amd/declance.c
===================================================================
--- linux-20140623-4maxp64.orig/drivers/net/ethernet/amd/declance.c
+++ linux-20140623-4maxp64/drivers/net/ethernet/amd/declance.c
@@ -475,7 +475,7 @@ static void lance_init_ring(struct net_d
 	*lib_ptr(ib, rx_ptr, lp->type) = leptr;
 	if (ZERO)
 		printk("RX ptr: %8.8x(%8.8x)\n",
-		       leptr, lib_off(brx_ring, lp->type));
+		       leptr, (uint)lib_off(brx_ring, lp->type));
 
 	/* Setup tx descriptor pointer */
 	leptr = offsetof(struct lance_init_block, btx_ring);
@@ -484,7 +484,7 @@ static void lance_init_ring(struct net_d
 	*lib_ptr(ib, tx_ptr, lp->type) = leptr;
 	if (ZERO)
 		printk("TX ptr: %8.8x(%8.8x)\n",
-		       leptr, lib_off(btx_ring, lp->type));
+		       leptr, (uint)lib_off(btx_ring, lp->type));
 
 	if (ZERO)
 		printk("TX rings:\n");
@@ -499,8 +499,9 @@ static void lance_init_ring(struct net_d
 						/* The ones required by tmd2 */
 		*lib_ptr(ib, btx_ring[i].misc, lp->type) = 0;
 		if (i < 3 && ZERO)
-			printk("%d: 0x%8.8x(0x%8.8x)\n",
-			       i, leptr, (uint)lp->tx_buf_ptr_cpu[i]);
+			printk("%d: 0x%8.8x(%#0*lx)\n",
+			       i, leptr, 2 * (int)sizeof(long) + 2,
+			       (long)lp->tx_buf_ptr_cpu[i]);
 	}
 
 	/* Setup the Rx ring entries */
@@ -516,8 +517,9 @@ static void lance_init_ring(struct net_d
 							     0xf000;
 		*lib_ptr(ib, brx_ring[i].mblength, lp->type) = 0;
 		if (i < 3 && ZERO)
-			printk("%d: 0x%8.8x(0x%8.8x)\n",
-			       i, leptr, (uint)lp->rx_buf_ptr_cpu[i]);
+			printk("%d: 0x%8.8x(%#0*lx)\n",
+			       i, leptr, 2 * (int)sizeof(long) + 2,
+			       (long)lp->rx_buf_ptr_cpu[i]);
 	}
 	iob();
 }

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[David Miller]

From: "Maciej W. Rozycki" <macro@linux-mips.org>
Date: Sun, 29 Jun 2014 03:10:47 +0100 (BST)

> @@ -499,8 +499,9 @@ static void lance_init_ring(struct net_d
>  						/* The ones required by tmd2 */
>  		*lib_ptr(ib, btx_ring[i].misc, lp->type) = 0;
>  		if (i < 3 && ZERO)
> -			printk("%d: 0x%8.8x(0x%8.8x)\n",
> -			       i, leptr, (uint)lp->tx_buf_ptr_cpu[i]);
> +			printk("%d: 0x%8.8x(%#0*lx)\n",
> +			       i, leptr, 2 * (int)sizeof(long) + 2,
> +			       (long)lp->tx_buf_ptr_cpu[i]);

Please just use "%p", no casts required.

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Wed, 2 Jul 2014, David Miller wrote:

> > @@ -499,8 +499,9 @@ static void lance_init_ring(struct net_d
> >  						/* The ones required by tmd2 */
> >  		*lib_ptr(ib, btx_ring[i].misc, lp->type) = 0;
> >  		if (i < 3 && ZERO)
> > -			printk("%d: 0x%8.8x(0x%8.8x)\n",
> > -			       i, leptr, (uint)lp->tx_buf_ptr_cpu[i]);
> > +			printk("%d: 0x%8.8x(%#0*lx)\n",
> > +			       i, leptr, 2 * (int)sizeof(long) + 2,
> > +			       (long)lp->tx_buf_ptr_cpu[i]);
> 
> Please just use "%p", no casts required.

 Hmm, there was something about %p that made me reject it, however I can't 
recall what it was and I can get the desired output with this format 
specifier (the NULL special case difference can be ignored, the pointers 
printed here won't ever be NULL).  Sending an update right away.

  Maciej

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Thu, 3 Jul 2014, Maciej W. Rozycki wrote:

> > > @@ -499,8 +499,9 @@ static void lance_init_ring(struct net_d
> > >  						/* The ones required by tmd2 */
> > >  		*lib_ptr(ib, btx_ring[i].misc, lp->type) = 0;
> > >  		if (i < 3 && ZERO)
> > > -			printk("%d: 0x%8.8x(0x%8.8x)\n",
> > > -			       i, leptr, (uint)lp->tx_buf_ptr_cpu[i]);
> > > +			printk("%d: 0x%8.8x(%#0*lx)\n",
> > > +			       i, leptr, 2 * (int)sizeof(long) + 2,
> > > +			       (long)lp->tx_buf_ptr_cpu[i]);
> > 
> > Please just use "%p", no casts required.
> 
>  Hmm, there was something about %p that made me reject it, however I can't 
> recall what it was and I can get the desired output with this format 
> specifier (the NULL special case difference can be ignored, the pointers 
> printed here won't ever be NULL).  Sending an update right away.

 Ah, there it is:

drivers/net/ethernet/amd/declance.c: In function 'lance_init_ring':
drivers/net/ethernet/amd/declance.c:503: warning: '#' flag used with '%p' printf format
drivers/net/ethernet/amd/declance.c:520: warning: '#' flag used with '%p' printf format

That's obviously GCC's incompatibility to our implementation.  I'm not 
sure if that can be worked around, but I'll see what I can do about it.

  Maciej

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Joe Perches]

On Thu, 2014-07-03 at 03:34 +0100, Maciej W. Rozycki wrote:
> On Thu, 3 Jul 2014, Maciej W. Rozycki wrote:
> 
> > > > @@ -499,8 +499,9 @@ static void lance_init_ring(struct net_d
> > > >  						/* The ones required by tmd2 */
> > > >  		*lib_ptr(ib, btx_ring[i].misc, lp->type) = 0;
> > > >  		if (i < 3 && ZERO)
> > > > -			printk("%d: 0x%8.8x(0x%8.8x)\n",
> > > > -			       i, leptr, (uint)lp->tx_buf_ptr_cpu[i]);
> > > > +			printk("%d: 0x%8.8x(%#0*lx)\n",
> > > > +			       i, leptr, 2 * (int)sizeof(long) + 2,
> > > > +			       (long)lp->tx_buf_ptr_cpu[i]);
> > > 
> > > Please just use "%p", no casts required.
> > 
> >  Hmm, there was something about %p that made me reject it, however I can't 
> > recall what it was and I can get the desired output with this format 
> > specifier (the NULL special case difference can be ignored, the pointers 
> > printed here won't ever be NULL).  Sending an update right away.
> 
>  Ah, there it is:
> 
> drivers/net/ethernet/amd/declance.c: In function 'lance_init_ring':
> drivers/net/ethernet/amd/declance.c:503: warning: '#' flag used with '%p' printf format
> drivers/net/ethernet/amd/declance.c:520: warning: '#' flag used with '%p' printf format
> 
> That's obviously GCC's incompatibility to our implementation.  I'm not 
> sure if that can be worked around, but I'll see what I can do about it.

The kernel vsprintf implementation doesn't prefix
pointers with 0x, so you can use 0x%p if you really
want that with a leading prefix, but you don't have
to use it.

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Wed, 2 Jul 2014, Joe Perches wrote:

> > >  Hmm, there was something about %p that made me reject it, however I can't 
> > > recall what it was and I can get the desired output with this format 
> > > specifier (the NULL special case difference can be ignored, the pointers 
> > > printed here won't ever be NULL).  Sending an update right away.
> > 
> >  Ah, there it is:
> > 
> > drivers/net/ethernet/amd/declance.c: In function 'lance_init_ring':
> > drivers/net/ethernet/amd/declance.c:503: warning: '#' flag used with '%p' printf format
> > drivers/net/ethernet/amd/declance.c:520: warning: '#' flag used with '%p' printf format
> > 
> > That's obviously GCC's incompatibility to our implementation.  I'm not 
> > sure if that can be worked around, but I'll see what I can do about it.
> 
> The kernel vsprintf implementation doesn't prefix
> pointers with 0x, so you can use 0x%p if you really
> want that with a leading prefix, but you don't have
> to use it.

 It does, when the `#' format modifier is used (go try yourself!).

 However GCC does not like it and (for the record) the requirement is 
buried in the maze of structures descending from `format_types_orig' in 
gcc/c-family/c-format.c, or more specifically the "p" entry of 
`print_char_table' that only allows "-w" characters as %p format 
modifiers.  There seems to be no way to override it.  Then the ISO C 
standard (at least version C90 that I have handy), after listing all the 
cases for the octal, hex and floating-point specifiers says about the `#' 
modifier that: "For other conversions, the behavior is undefined."

 So both GCC's complaint and our implementation are valid, it's just there 
appears to be no straightforward way to make them agree with each other. 
OK, I guess the format string could be indirected at which point I reckon 
GCC would stop parsing it (at least at the point it would start believing 
the variable used isn't constant), but I'm too lazy to check it and I 
think this code isn't worth complicating any further (I shall consider 
switching to using `dev_dbg' instead).

 I've noticed however that the other place this patch addresses does not 
use the `0x' prefix anyway so I decided to drop it after all.  These are 
debug messages anyway, so it doesn't matter much.  And I think using 0x%p 
would be ugly; here it wouldn't really matter, but ordinarily allowing a 
format to produce `0x (null)' would be rather lame, so I don't want to 
spread examples someone might foolishly copy.

 Thanks for your input, I think I may have a use for the findings in the 
future.

  Maciej

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Joe Perches]

On Thu, 2014-07-03 at 05:51 +0100, Maciej W. Rozycki wrote:
> > The kernel vsprintf implementation doesn't prefix
> > pointers with 0x, so you can use 0x%p if you really
> > want that with a leading prefix, but you don't have
> > to use it.
> 
>  It does, when the `#' format modifier is used (go try yourself!).

I know it does, but it's incidental.

I phrased it badly though.
There's no code that uses it.

$ git grep "%#p" | wc -l
0

And I know that code pretty well thanks.

> I think using 0x%p 
> would be ugly; here it wouldn't really matter, but ordinarily allowing a 
> format to produce `0x (null)' would be rather lame, so I don't want to 
> spread examples someone might foolishly copy.

$ git grep "0x%p" | wc -l
1747

<shrug>  What's one more...

cheers, Joe

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Wed, 2 Jul 2014, Joe Perches wrote:

> > > The kernel vsprintf implementation doesn't prefix
> > > pointers with 0x, so you can use 0x%p if you really
> > > want that with a leading prefix, but you don't have
> > > to use it.
> > 
> >  It does, when the `#' format modifier is used (go try yourself!).
> 
> I know it does, but it's incidental.

 Is it?  Someone took the effort to handle it:

	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);

while they could do:

	int default_width = 2 * sizeof(void *);

	spec.flags &= ~SPECIAL;

instead (clearing the flag to suit `number' called later on, that is).  
And I actually find it a natural consequence of how we implement `%p'; 
using `#' to switch between the two formats seems to fit ISO C perfectly 
(as an extension, that is, of course, but that does not contradict the 
requirement of "undefined behaviour").

 Hmm, actually I wonder if GCC maintainers could be persuaded to accept a 
`linux_printk' format checker, that would accurately match our semantics 
and could handle some of our other extensions too.  There are precedents 
already, `cmn_err' and `CFString' (for Solaris and Darwin), so it's not 
like a no-no outright.  WDYT?

> I phrased it badly though.
> There's no code that uses it.
> 
> $ git grep "%#p" | wc -l
> 0

 Yeah, I did this before too; no surprise given the warning GCC produces.

> > I think using 0x%p 
> > would be ugly; here it wouldn't really matter, but ordinarily allowing a 
> > format to produce `0x (null)' would be rather lame, so I don't want to 
> > spread examples someone might foolishly copy.
> 
> $ git grep "0x%p" | wc -l
> 1747
> 
> <shrug>  What's one more...

 Heh, see what I mean.  And somehow I don't feel compelled to follow 1747 
mistakes. ;)  We could fix them all though if we had a proper 
`linux_printk' checker though.

  Maciej

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Joe Perches]

On Thu, 2014-07-03 at 07:01 +0100, Maciej W. Rozycki wrote:
> On Wed, 2 Jul 2014, Joe Perches wrote:
> 
> > > > The kernel vsprintf implementation doesn't prefix
> > > > pointers with 0x, so you can use 0x%p if you really
> > > > want that with a leading prefix, but you don't have
> > > > to use it.
> > > 
> > >  It does, when the `#' format modifier is used (go try yourself!).
> > 
> > I know it does, but it's incidental.
> 
>  Is it?  Someone took the effort to handle it:
> 
> 	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
> 
> while they could do:
> 
> 	int default_width = 2 * sizeof(void *);
> 
> 	spec.flags &= ~SPECIAL;

Grant Likely did that a couple of years ago.

commit 725fe002d315c2501c110b7245d3eb4f4535f4d6
Author: Grant Likely <grant.likely@secretlab.ca>
Date:   Thu May 31 16:26:08 2012 -0700

    vsprintf: correctly handle width when '#' flag used in %#p format
    
    The '%p' output of the kernel's vsprintf() uses spec.field_width to
    determine how many digits to output based on 2 * sizeof(void*) so that all
    digits of a pointer are shown.  ie.  a pointer will be output as
    "001A2B3C" instead of "1A2B3C".  However, if the '#' flag is used in the
    format (%#p), then the code doesn't take into account the width of the
    '0x' prefix and will end up outputing "0x1A2B3C" instead of "0x001A2B3C".
    
    This patch reworks the "pointer()" format hook to include 2 characters for
    the '0x' prefix if the '#' flag is included.

Not sure it ever mattered myself.

>  Hmm, actually I wonder if GCC maintainers could be persuaded to accept a 
> `linux_printk' format checker, that would accurately match our semantics 
> and could handle some of our other extensions too.  There are precedents 
> already, `cmn_err' and `CFString' (for Solaris and Darwin), so it's not 
> like a no-no outright.  WDYT?
> 

Maybe via a gcc plugin.

https://gcc.gnu.org/wiki/plugins

If you're going to write one, it'd be nice to validate all
the %p<foo> extensions too.

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Grant Likely]

On Thu, Jul 3, 2014 at 7:25 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2014-07-03 at 07:01 +0100, Maciej W. Rozycki wrote:
>> On Wed, 2 Jul 2014, Joe Perches wrote:
>>
>> > > > The kernel vsprintf implementation doesn't prefix
>> > > > pointers with 0x, so you can use 0x%p if you really
>> > > > want that with a leading prefix, but you don't have
>> > > > to use it.
>> > >
>> > >  It does, when the `#' format modifier is used (go try yourself!).
>> >
>> > I know it does, but it's incidental.
>>
>>  Is it?  Someone took the effort to handle it:
>>
>>       int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
>>
>> while they could do:
>>
>>       int default_width = 2 * sizeof(void *);
>>
>>       spec.flags &= ~SPECIAL;
>
> Grant Likely did that a couple of years ago.
>
> commit 725fe002d315c2501c110b7245d3eb4f4535f4d6
> Author: Grant Likely <grant.likely@secretlab.ca>
> Date:   Thu May 31 16:26:08 2012 -0700
>
>     vsprintf: correctly handle width when '#' flag used in %#p format
>
>     The '%p' output of the kernel's vsprintf() uses spec.field_width to
>     determine how many digits to output based on 2 * sizeof(void*) so that all
>     digits of a pointer are shown.  ie.  a pointer will be output as
>     "001A2B3C" instead of "1A2B3C".  However, if the '#' flag is used in the
>     format (%#p), then the code doesn't take into account the width of the
>     '0x' prefix and will end up outputing "0x1A2B3C" instead of "0x001A2B3C".
>
>     This patch reworks the "pointer()" format hook to include 2 characters for
>     the '0x' prefix if the '#' flag is included.
>
> Not sure it ever mattered myself.

I had code using it. It was long enough ago that I'd don't remember
exactly why. Apparently it never made it into the kernel or it got
reverted by someone else.

g.

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Wed, 2 Jul 2014, Joe Perches wrote:

> > > > > The kernel vsprintf implementation doesn't prefix
> > > > > pointers with 0x, so you can use 0x%p if you really
> > > > > want that with a leading prefix, but you don't have
> > > > > to use it.
> > > > 
> > > >  It does, when the `#' format modifier is used (go try yourself!).
> > > 
> > > I know it does, but it's incidental.
> > 
> >  Is it?  Someone took the effort to handle it:
> > 
> > 	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
> > 
> > while they could do:
> > 
> > 	int default_width = 2 * sizeof(void *);
> > 
> > 	spec.flags &= ~SPECIAL;
> 
> Grant Likely did that a couple of years ago.
> 
> commit 725fe002d315c2501c110b7245d3eb4f4535f4d6
> Author: Grant Likely <grant.likely@secretlab.ca>
> Date:   Thu May 31 16:26:08 2012 -0700
> 
>     vsprintf: correctly handle width when '#' flag used in %#p format
>     
>     The '%p' output of the kernel's vsprintf() uses spec.field_width to
>     determine how many digits to output based on 2 * sizeof(void*) so that all
>     digits of a pointer are shown.  ie.  a pointer will be output as
>     "001A2B3C" instead of "1A2B3C".  However, if the '#' flag is used in the
>     format (%#p), then the code doesn't take into account the width of the
>     '0x' prefix and will end up outputing "0x1A2B3C" instead of "0x001A2B3C".
>     
>     This patch reworks the "pointer()" format hook to include 2 characters for
>     the '0x' prefix if the '#' flag is included.
> 
> Not sure it ever mattered myself.
> 
> >  Hmm, actually I wonder if GCC maintainers could be persuaded to accept a 
> > `linux_printk' format checker, that would accurately match our semantics 
> > and could handle some of our other extensions too.  There are precedents 
> > already, `cmn_err' and `CFString' (for Solaris and Darwin), so it's not 
> > like a no-no outright.  WDYT?
> > 
> 
> Maybe via a gcc plugin.
> 
> https://gcc.gnu.org/wiki/plugins
> 
> If you're going to write one, it'd be nice to validate all
> the %p<foo> extensions too.

 Well, it would be easier for me to tweak GCC itself, I have never had 
anything to do with plugins.

* Advantages of having it in GCC: everyone will have it, eventually.  
  Disadvantages: they'll have to upgrade the compiler to a version that's 
  recent enough (or backport the change).

* Advantages of having it in a plugin: GCC versions from 4.5 up can be 
  supported, many people won't have to upgrade the compiler.  
  Disadvantages: most likely nobody will use it. ;)

Anyway, I won't rush implementing it, sorry, there's too much other stuff 
I want to do I consider important, though I can place it somewhere down my 
to-do list.

 One question though, does either of you or anybody else know why we're 
inconsistent about this 0x prefixing of virtual addresses vs physical 
addresses?  Specifically %p vs e.g. %pad.  I have fixed (patch just 
posted) and made use of some debug code in the defxx driver and I get 
output like this:

Descriptor block virt = a8000000ce0ec000, phys = 0x00000000ce0ec000
Command Request buffer virt = a8000000ce0ed380, phys = 0x00000000ce0ed380
Command Response buffer virt = a8000000ce0ed580, phys = 0x00000000ce0ed580
Receive buffer block virt = a8000000ce0ed780, phys = 0x00000000ce0ed780
Consumer block virt = a8000000ce0ed780, phys = 0x00000000ce0ed780

For %p the SPECIAL flag is set according to the presence or absence of the 
`#' modifier.  For %pad and friends the flag is forced on.  I find it 
messy.  Can we decide one way or the other?  I have no strong preference 
towards either format, but I think it would be good to stay consistent, 
also in the handling of the modifier.

 Given that (as you've noticed) people want to see that 0x prefix anyway 
perhaps we can add it to %p by default too (and then let it be switched 
off with `#' across all the relevant %p formats)?  Any thoughts?

 Adding LKML for some more feedback perhaps.

  Maciej

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Joe Perches]

On Sat, 2014-07-05 at 15:56 +0100, Maciej W. Rozycki wrote:
>  One question though, does either of you or anybody else know why we're 
> inconsistent about this 0x prefixing of virtual addresses vs physical 
> addresses?  Specifically %p vs e.g. %pad.

I think it's a mistake and I agree.

I submitted a patch to remove the prefix from %pad.

https://lkml.org/lkml/2014/3/21/333

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Sat, 5 Jul 2014, Joe Perches wrote:

> >  One question though, does either of you or anybody else know why we're 
> > inconsistent about this 0x prefixing of virtual addresses vs physical 
> > addresses?  Specifically %p vs e.g. %pad.
> 
> I think it's a mistake and I agree.
> 
> I submitted a patch to remove the prefix from %pad.
> 
> https://lkml.org/lkml/2014/3/21/333

 Great!  Your proposal looks good to me in principle, however you need to 
factor in SPECIAL having been set by `#' somehow as `number' will respect 
it.  I suggest using the same field width calculation that `pointer' uses 
for `default_width' (sans the type used with `sizeof' of course, that is).

  Maciej

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Joe Perches]

On Sat, 2014-07-05 at 18:39 +0100, Maciej W. Rozycki wrote:
> On Sat, 5 Jul 2014, Joe Perches wrote:
> 
> > >  One question though, does either of you or anybody else know why we're 
> > > inconsistent about this 0x prefixing of virtual addresses vs physical 
> > > addresses?  Specifically %p vs e.g. %pad.
> > 
> > I think it's a mistake and I agree.
> > 
> > I submitted a patch to remove the prefix from %pad.
> > 
> > https://lkml.org/lkml/2014/3/21/333
> 
>  Great!  Your proposal looks good to me in principle, however you need to 
> factor in SPECIAL having been set by `#' somehow as `number' will respect 
> it.  I suggest using the same field width calculation that `pointer' uses 
> for `default_width' (sans the type used with `sizeof' of course, that is).

I don't think %#p is valid so it
shouldn't have been set by #.

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Sat, 5 Jul 2014, Joe Perches wrote:

> > > I think it's a mistake and I agree.
> > > 
> > > I submitted a patch to remove the prefix from %pad.
> > > 
> > > https://lkml.org/lkml/2014/3/21/333
> > 
> >  Great!  Your proposal looks good to me in principle, however you need to 
> > factor in SPECIAL having been set by `#' somehow as `number' will respect 
> > it.  I suggest using the same field width calculation that `pointer' uses 
> > for `default_width' (sans the type used with `sizeof' of course, that is).
> 
> I don't think %#p is valid so it
> shouldn't have been set by #.

 Huh?  As recently as last Wednesday you pointed me at the specific commit 
from Grant that made it valid (GCC format complaints aside).

  Maciej

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Joe Perches]

On Sat, 2014-07-05 at 19:20 +0100, Maciej W. Rozycki wrote:
> On Sat, 5 Jul 2014, Joe Perches wrote:
> > I don't think %#p is valid so it
> > shouldn't have been set by #.
> 
>  Huh?  As recently as last Wednesday you pointed me at the specific commit 
> from Grant that made it valid (GCC format complaints aside).

Those gcc complaints are precisely the thing
that makes it invalid.

I believe you're tilting at windmills.

Hey, it works sometimes.  Knock yourself out.

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Sat, 5 Jul 2014, Joe Perches wrote:

> > > I don't think %#p is valid so it
> > > shouldn't have been set by #.
> > 
> >  Huh?  As recently as last Wednesday you pointed me at the specific commit 
> > from Grant that made it valid (GCC format complaints aside).
> 
> Those gcc complaints are precisely the thing
> that makes it invalid.

 So enforce that in code then, clear the SPECIAL flag where appropriate 
and do not try to handle it in one place while leaving other ones to 
behave randomly (i.e. a supposedly fixed field width varies depending on 
the two uppermost digits).  Please note that it's only your proposed 
change that introduces that randomness, right now code does what's 
supposed and documented to, except a bit inconsistently.

> I believe you're tilting at windmills.
> 
> Hey, it works sometimes.  Knock yourself out.

 I pointed out an inconsistency with the intent to propose a fix once a 
consensus have been reached, one way or another.  And I think shifting the 
inconsistency to a different place, which is what your proposal does, 
isn't really a complete solution, although I do recognise the improvement.

  Maciej

[PATCH] vsprintf: Remove SPECIAL from pointer types

[Joe Perches]

Because gcc issues a complaint about any pointer format with %#p,
remove the use of SPECIAL to prefix 0x to various pointer types.

There are no uses in the kernel tree of %#p.

This removes the capability added by commit 725fe002d315
("vsprintf: correctly handle width when '#' flag used in %#p format").

There are some incidental message logging output changes of %pa
uses with this change.  None are in seq output so there are no
api changes.

Signed-off-by: Joe Perches <joe@perches.com>
---

Fine by me, here...

On Sat, 2014-07-05 at 21:25 +0100, Maciej W. Rozycki wrote:
> On Sat, 5 Jul 2014, Joe Perches wrote:
> 
> > > > I don't think %#p is valid so it
> > > > shouldn't have been set by #.
> > > 
> > >  Huh?  As recently as last Wednesday you pointed me at the specific commit 
> > > from Grant that made it valid (GCC format complaints aside).
> > 
> > Those gcc complaints are precisely the thing
> > that makes it invalid.
> 
>  So enforce that in code then, clear the SPECIAL flag where appropriate 
> and do not try to handle it in one place while leaving other ones to 
> behave randomly (i.e. a supposedly fixed field width varies depending on 
> the two uppermost digits).  Please note that it's only your proposed 
> change that introduces that randomness, right now code does what's 
> supposed and documented to, except a bit inconsistently.
> 
> > I believe you're tilting at windmills.
> > 
> > Hey, it works sometimes.  Knock yourself out.
> 
>  I pointed out an inconsistency with the intent to propose a fix once a 
> consensus have been reached, one way or another.  And I think shifting the 
> inconsistency to a different place, which is what your proposal does, 
> isn't really a complete solution, although I do recognise the improvement.

 lib/vsprintf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 6fe2c84..1cad65b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -632,7 +632,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
 	return string(buf, end, sym, spec);
 #else
 	spec.field_width = 2 * sizeof(void *);
-	spec.flags |= SPECIAL | SMALL | ZEROPAD;
+	spec.flags |= SMALL | ZEROPAD;
 	spec.base = 16;
 
 	return number(buf, end, value, spec);
@@ -1165,18 +1165,18 @@ char *address_val(char *buf, char *end, const void *addr,
 {
 	unsigned long long num;
 
-	spec.flags |= SPECIAL | SMALL | ZEROPAD;
+	spec.flags |= SMALL | ZEROPAD;
 	spec.base = 16;
 
 	switch (fmt[1]) {
 	case 'd':
 		num = *(const dma_addr_t *)addr;
-		spec.field_width = sizeof(dma_addr_t) * 2 + 2;
+		spec.field_width = sizeof(dma_addr_t) * 2;
 		break;
 	case 'p':
 	default:
 		num = *(const phys_addr_t *)addr;
-		spec.field_width = sizeof(phys_addr_t) * 2 + 2;
+		spec.field_width = sizeof(phys_addr_t) * 2;
 		break;
 	}
 
@@ -1259,7 +1259,7 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
-	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
+	int default_width = 2 * sizeof(void *);
 
 	if (!ptr && *fmt != 'K') {
 		/*

Re: [PATCH] vsprintf: Remove SPECIAL from pointer types

[Maciej W. Rozycki]

On Sat, 5 Jul 2014, Joe Perches wrote:

> Because gcc issues a complaint about any pointer format with %#p,
> remove the use of SPECIAL to prefix 0x to various pointer types.
> 
> There are no uses in the kernel tree of %#p.
> 
> This removes the capability added by commit 725fe002d315
> ("vsprintf: correctly handle width when '#' flag used in %#p format").
> 
> There are some incidental message logging output changes of %pa
> uses with this change.  None are in seq output so there are no
> api changes.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> 
> Fine by me, here...
> 
> On Sat, 2014-07-05 at 21:25 +0100, Maciej W. Rozycki wrote:
> > On Sat, 5 Jul 2014, Joe Perches wrote:
> > 
> > > > > I don't think %#p is valid so it
> > > > > shouldn't have been set by #.
> > > > 
> > > >  Huh?  As recently as last Wednesday you pointed me at the specific commit
> > > > from Grant that made it valid (GCC format complaints aside).
> > > 
> > > Those gcc complaints are precisely the thing
> > > that makes it invalid.
> > 
> >  So enforce that in code then, clear the SPECIAL flag where appropriate 
> > and do not try to handle it in one place while leaving other ones to 
> > behave randomly (i.e. a supposedly fixed field width varies depending on 
> > the two uppermost digits).  Please note that it's only your proposed 
> > change that introduces that randomness, right now code does what's 
> > supposed and documented to, except a bit inconsistently.
> > 
> > > I believe you're tilting at windmills.
> > > 
> > > Hey, it works sometimes.  Knock yourself out.
> > 
> >  I pointed out an inconsistency with the intent to propose a fix once a 
> > consensus have been reached, one way or another.  And I think shifting the 
> > inconsistency to a different place, which is what your proposal does, 
> > isn't really a complete solution, although I do recognise the improvement.

 Conceptually good, thanks for your effort, but you still need to clear 
SPECIAL in `pointer' and maybe elsewhere, as that'll have been set for the 
case concerned in `format_decode' by this code:

		case '#': spec->flags |= SPECIAL; break;

(that doesn't check what follows) and then respected once `number' is 
reached.  E.g.:

char *pointer(const char *fmt, char *buf, char *end, void *ptr,
	      struct printf_spec spec)
{
	int default_width = 2 * sizeof(void *);

	spec.flags &= ~SPECIAL;

or suchlike.  Sorry to have been unclear about it.

 Note that obviously GCC will only complain about `#' if the format is 
constant, there's no way for it to work through a variable format, e.g.:

{
	char *f;
	void *const p = NULL;

	printk("%#p\n", p);
	f = kstrdup("%#p\n", GFP_KERNEL);
	printk(f, p);
	kfree(f);
}

-- it'll complain only about the first `printk', not the second.

  Maciej

Re: [PATCH] vsprintf: Remove SPECIAL from pointer types

[Joe Perches]

On Sun, 2014-07-06 at 12:44 +0100, Maciej W. Rozycki wrote:
> On Sat, 5 Jul 2014, Joe Perches wrote:
> 
> > Because gcc issues a complaint about any pointer format with %#p,
> > remove the use of SPECIAL to prefix 0x to various pointer types.
[]
>  Conceptually good, thanks for your effort, but you still need to clear 
> SPECIAL in `pointer' and maybe elsewhere, as that'll have been set for the 
> case concerned in `format_decode' by this code:
> 
> 		case '#': spec->flags |= SPECIAL; break;
> 
> (that doesn't check what follows) and then respected once `number' is 
> reached.  E.g.:
> 
> char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> 	      struct printf_spec spec)
> {
> 	int default_width = 2 * sizeof(void *);
> 
> 	spec.flags &= ~SPECIAL;
> 
> or suchlike.  Sorry to have been unclear about it.

I think you're not right here.
The patch shouldn't remove the capability to prefix.

But neither am I right with the commit log actually.
It should say something like "remove the default extra
width for the 0x prefix from %#p".

Actually, I'm not sure that removing "SPECIAL adds
the pointer prefix length" to width is that good.

I think it doesn't matter much.

I do like removing the prefix it from %pa though.

linux's printf like capability is not exactly like
gcc's.  It doesn't have to be.  linux's implementation
already does not prefix 0x to pointers when gcc does.
gcc uses '(nil)', linux '(null)', etc.

And linux's variant does a bunch of extended outputs
for %p<foo> variants where it overrides any size and
prefixing specified.

The only difference introduced by the proposed patch
here is that a generic pointer type will now have a
variable output width if %#p is used depending on the
high two bytes of the pointer value if a size is not
specified.

fyi: gcc will output a prefix 0x with %#p just as it
     does for %p.

The major difference is that linux uses a default of
sizeof(void *) * 2 for the width and zero fills without
prefix, gcc defaults to the minimum # of chars required
and prefixes.

cheers, Joe

RE: [PATCH] vsprintf: Remove SPECIAL from pointer types

[David Laight]

From: Joe Perches
> Because gcc issues a complaint about any pointer format with %#p,
> remove the use of SPECIAL to prefix 0x to various pointer types.
> 
> There are no uses in the kernel tree of %#p.

I know you guys don't really care about them, but there might
be uses in out of tree drivers.

With the change what is output for %#p ?

I know I've used %#p in some code (possibly userspace) that need
to run under multiple OS because 0x%p generates 0x0x on one of the OS.
(We might have removed them because of the gcc warning though.)

	David

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Grant Likely]

On Sat, 05 Jul 2014 11:31:39 -0700, Joe Perches <joe@perches.com> wrote:
> On Sat, 2014-07-05 at 19:20 +0100, Maciej W. Rozycki wrote:
> > On Sat, 5 Jul 2014, Joe Perches wrote:
> > > I don't think %#p is valid so it
> > > shouldn't have been set by #.
> > 
> >  Huh?  As recently as last Wednesday you pointed me at the specific commit 
> > from Grant that made it valid (GCC format complaints aside).
> 
> Those gcc complaints are precisely the thing
> that makes it invalid.

That's the most inane reason ever for saying something is invalid. "The
tool doesn't recognise it, there for it is invalid?" Seriously?

Tools are just tools. They aren't the source of what is valid/invalid,
they only report on what we as engineers have told them to do, because
*we* define what should be valid/invalid.

If you've got a real reason that explains *why* the tool rejects that
construct, then I'd be happy to hear it, but otherwise that argument
makes no sense.

g.

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Maciej W. Rozycki]

On Mon, 7 Jul 2014, Grant Likely wrote:

> > > > I don't think %#p is valid so it
> > > > shouldn't have been set by #.
> > > 
> > >  Huh?  As recently as last Wednesday you pointed me at the specific commit 
> > > from Grant that made it valid (GCC format complaints aside).
> > 
> > Those gcc complaints are precisely the thing
> > that makes it invalid.
> 
> That's the most inane reason ever for saying something is invalid. "The
> tool doesn't recognise it, there for it is invalid?" Seriously?
> 
> Tools are just tools. They aren't the source of what is valid/invalid,
> they only report on what we as engineers have told them to do, because
> *we* define what should be valid/invalid.
> 
> If you've got a real reason that explains *why* the tool rejects that
> construct, then I'd be happy to hear it, but otherwise that argument
> makes no sense.

 GCC rejects it, because its `printf' format attribute expects format 
specifiers according to ISO C and its formatted input/output functions.  
The syntax of our `printk' is however different, not only for %#p, so I 
agree it's GCC that acts incompatibly and not our design being wrong.  I 
have therefore offered a solution (though not an implementation right now, 
sorry; I'm not even set up to start such development right away) to 
introduce a `linux_printk' format attribute to GCC that would match our 
requirements.

 FAOD I'm in favour to retaining `#' with %p, but then making it 
consistent across variants such as %pad vs %#pad.  This does not preclude 
or require adding `linux_printk' to GCC in the future.

  Maciej

Re: [PATCH] vsprintf: Remove SPECIAL from pointer types

[Joe Perches]

On Mon, 2014-07-07 at 08:26 +0000, David Laight wrote:
> From: Joe Perches
> > Because gcc issues a complaint about any pointer format with %#p,
> > remove the use of SPECIAL to prefix 0x to various pointer types.
> > 
> > There are no uses in the kernel tree of %#p.
> 
> I know you guys don't really care about them, but there might
> be uses in out of tree drivers.
> 
> With the change what is output for %#p ?

Linux's output of %#p for normal, non %p<foo> extension use,
continues to be prefixed with 0x and zero filled.

Prior to this proposed change:
%#p uses a fixed width of sizeof(void *) * 2 + 2.
%p uses a fixed with of sizeof(void *) * 2

Post:
%#p uses a variable width of the minimum of sizeof(void *) * 2
to sizeof(void *) * 2 + 2 depending on the high order 2 bytes
of the pointer value.

There is no in-kernel tree code that uses %#p so it
has no net effect.

Personally, I prefer %#p uses the "+ 2" fixed width.

The real benefit is removing the auto-prefixing of 0x
when using the %pa extension to be consistent with
other naked pointer output types.

Re: [PATCH v2] declance: Fix 64-bit compilation warnings

[Joe Perches]

On Mon, 2014-07-07 at 13:01 +0100, Grant Likely wrote:
> On Sat, 05 Jul 2014 11:31:39 -0700, Joe Perches <joe@perches.com> wrote:
> > On Sat, 2014-07-05 at 19:20 +0100, Maciej W. Rozycki wrote:
> > > On Sat, 5 Jul 2014, Joe Perches wrote:
> > > > I don't think %#p is valid so it
> > > > shouldn't have been set by #.
> > > 
> > >  Huh?  As recently as last Wednesday you pointed me at the specific commit 
> > > from Grant that made it valid (GCC format complaints aside).
> > 
> > Those gcc complaints are precisely the thing
> > that makes it invalid.
> 
> That's the most inane reason ever for saying something is invalid. "The
> tool doesn't recognise it, there for it is invalid?" Seriously?

Inane maybe, but practical.

The tool says "don't do that", so we shouldn't
or we should fix or quiet the tool.

fwiw: I agree with Maciej.

gcc's warning on %#p should be able to be quieted somehow.