| Message ID | alpine.LFD.2.11.1406290246120.15455@eddie.linux-mips.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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 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 -- 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 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 -- 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 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. -- 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 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 -- 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 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 -- 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 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 -- 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 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. -- 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 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. -- 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 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 -- 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 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 -- 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 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 -- 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 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 #. -- 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 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 -- 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 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. -- 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 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 -- 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 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. -- 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, 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 -- 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 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. -- 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
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(); }
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 -- 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