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

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

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