| Message ID | 1404593114.6384.72.camel@joe-AO725 |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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') { /*
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(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html