| Message ID | 1379459317-13046-6-git-send-email-daniel.santos@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2013-09-17 at 18:08 -0500, danielfsantos@att.net wrote: > This adds an extension for the integral format specifier suffix of 'e', > so that the format %[duxXo]e will result in printing an number (as > before) in addition to a name and descrption for an error code, if such > support is enabled and a name and descrption is found. > > My initial thought was to use the glibc extension of '%m', but this > format specifier uses the value of libc's errno and adding a value > breaks gcc's printf parsing. I'm not convinced that the 'e' extension > is optimal, although there are only four instances of this pattern in > the kernel that would need to be changed. > > git grep -E '".*%[#0\ +\-]*[0-9]*[hljzt]*[idoxXu]e' > > Alternatively, 'E' was another thought for a suffix as well. I'd much rather see another %p extension used instead of generating odd output given normal printf formats. %pE might work -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches <joe@perches.com> wrote: > On Tue, 2013-09-17 at 18:08 -0500, danielfsantos@att.net wrote: > > This adds an extension for the integral format specifier suffix of 'e', > > so that the format %[duxXo]e will result in printing an number (as > > before) in addition to a name and descrption for an error code, if such > > support is enabled and a name and descrption is found. > > > > My initial thought was to use the glibc extension of '%m', but this > > format specifier uses the value of libc's errno and adding a value > > breaks gcc's printf parsing. I'm not convinced that the 'e' extension > > is optimal, although there are only four instances of this pattern in > > the kernel that would need to be changed. > > > > git grep -E '".*%[#0\ +\-]*[0-9]*[hljzt]*[idoxXu]e' > > > > Alternatively, 'E' was another thought for a suffix as well. > > I'd much rather see another %p extension used instead > of generating odd output given normal printf formats. > > %pE might work I guess you'd use that with ERR_PTR(). On one hand, it would look decidedly odd, but on the other hand, we have errors in pointers in a lot of places already which wouldn't then need converting back - so it doesn't seem entirely unreasonable. David -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/18/2013 06:04 AM, David Howells wrote: > Joe Perches <joe@perches.com> wrote: > >> On Tue, 2013-09-17 at 18:08 -0500, danielfsantos@att.net wrote: >>> This adds an extension for the integral format specifier suffix of 'e', >>> so that the format %[duxXo]e will result in printing an number (as >>> before) in addition to a name and descrption for an error code, if such >>> support is enabled and a name and descrption is found. >>> >>> My initial thought was to use the glibc extension of '%m', but this >>> format specifier uses the value of libc's errno and adding a value >>> breaks gcc's printf parsing. I'm not convinced that the 'e' extension >>> is optimal, although there are only four instances of this pattern in >>> the kernel that would need to be changed. >>> >>> git grep -E '".*%[#0\ +\-]*[0-9]*[hljzt]*[idoxXu]e' >>> >>> Alternatively, 'E' was another thought for a suffix as well. >> I'd much rather see another %p extension used instead >> of generating odd output given normal printf formats. >> >> %pE might work > I guess you'd use that with ERR_PTR(). On one hand, it would look decidedly > odd, but on the other hand, we have errors in pointers in a lot of places > already which wouldn't then need converting back - so it doesn't seem entirely > unreasonable. > > David Hmm, this discussion makes me pine for the gnu libc '%m' extension even further. I wish there was an easy way to tell gcc that you want it to check your format, but here are my extensions. Oh well. Honestly, I'm not too keen on the %pE idea, although I can see that %p is where all of the kernel extensions currently are. Unless I'm incorrect, if I use ERR_PTR() on a signed int on a x86_64 where pointer is 64 bits and int is 32, wouldn't that mean a signed conversion instruction where the sign bit has to be moved from bit 31 to 63? Either way, %pE does seem to make a lot of sense for conditions where we already have a pointer that we would otherwise use PTR_ERR() to convert, but it just seems klunky to use it on an int, to have it treated like a pointer and then re-interpreted as an int. Maybe we can add %pE as well as %dE and leave [ioxXu] out of it? Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-09-18 at 20:27 -0500, Daniel Santos wrote: > if I use ERR_PTR() on a signed int on a x86_64 where pointer > is 64 bits and int is 32, wouldn't that mean a signed conversion > instruction where the sign bit has to be moved from bit 31 to 63? No. It's cast to long static inline void * __must_check ERR_PTR(long error) { return (void *) error; } > Either way, %pE does seem to make a lot of sense for conditions where we > already have a pointer that we would otherwise use PTR_ERR() to convert, > but it just seems klunky to use it on an int, to have it treated like a > pointer and then re-interpreted as an int. Maybe we can add %pE as well > as %dE and leave [ioxXu] out of it? I think having just one way to format is better. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/19/2013 08:07 PM, Joe Perches wrote: > On Wed, 2013-09-18 at 20:27 -0500, Daniel Santos wrote: >> if I use ERR_PTR() on a signed int on a x86_64 where pointer >> is 64 bits and int is 32, wouldn't that mean a signed conversion >> instruction where the sign bit has to be moved from bit 31 to 63? > No. It's cast to long > > static inline void * __must_check ERR_PTR(long error) > { > return (void *) error; > } Yes, but it is that cast from int to long that costs us a signed extend instruction on platforms where sizeof(int) != sizeof(long). This example should demonstrate the issue: extern void funca(void *ptr); static inline void * ERR_PTR(long error) { return (void *) error; } void funcb(int i) { funca(ERR_PTR(i)); } void funcc(long l) { funca(ERR_PTR(l)); } And here is the generated code on x86_64 with -O2: 0000000000000000 <funcb>: return (void *) error; } void funcb(int i) { funca(ERR_PTR(i)); 0: 48 63 ff movslq %edi,%rdi 3: e9 00 00 00 00 jmpq 8 <funcb+0x8> 4: R_X86_64_PC32 funca-0x4 8: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) f: 00 0000000000000010 <funcc>: } void funcc(long l) { funca(ERR_PTR(l)); 10: e9 00 00 00 00 jmpq 15 <funcc+0x5> 11: R_X86_64_PC32 funca-0x4 So on x86_64 this movslq is 3 bytes of text, plus register pollution for each time you convert an int to a pointer. I don't know the precise number of cases where error numbers are passed to printk as ints, but I presume it is enough that this could add several kilobytes of text. Either way, for a popular function like vsnprintf, it's better to take a moderate bloat in the function than a little bloat at many call sites, especially when it's not performance critical. >> Either way, %pE does seem to make a lot of sense for conditions where we >> already have a pointer that we would otherwise use PTR_ERR() to convert, >> but it just seems klunky to use it on an int, to have it treated like a >> pointer and then re-interpreted as an int. Maybe we can add %pE as well >> as %dE and leave [ioxXu] out of it? > I think having just one way to format is better. > Yeah, I do agree, I just don't see how to do it without introducing unnecessary bloat. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-09-20 at 00:21 -0500, Daniel Santos wrote: [nice example about bloat] > Yeah, I do agree, I just don't see how to do it without introducing > unnecessary bloat. I think the code size cost of %pE is pretty trivial compared to the log size/runtime cost. I would not want to see it used generically. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches <joe@perches.com> wrote: > > Yeah, I do agree, I just don't see how to do it without introducing > > unnecessary bloat. > > I think the code size cost of %pE is pretty trivial > compared to the log size/runtime cost. > > I would not want to see it used generically. I suspect he doesn't really need to implement a "strerror()" function but should rather build it straight into printk()/vsprintf(). David -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/20/2013 06:45 AM, Joe Perches wrote: > On Fri, 2013-09-20 at 00:21 -0500, Daniel Santos wrote: > > [nice example about bloat] > >> Yeah, I do agree, I just don't see how to do it without introducing >> unnecessary bloat. > I think the code size cost of %pE is pretty trivial > compared to the log size/runtime cost. I suppose that would depend upon the conditions. If no printks with error codes/names/messages are triggered, then the cost on the syslog is zero. Either way, we're already looking at adding one byte to the format string for the extra 'E' character and the 3-4 bytes for sign extend instruction would not exist on arm and most other archs. I guess I'm usually trying to consider the memory constrained system where you wouldn't enable this feature but still suffer the sign extend bloat -- then again, I don't know of many embedded x86_64 systems. :) > I would not want to see it used generically. Hmm, I had considered this as a new mechanism to emit error codes to the ring buffer. What sort of guidelines do you think are reasonable for when to use this proposed extension and when not to? Maybe on printks for error conditions that are most likely? Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/20/2013 08:07 AM, David Howells wrote: > I suspect he doesn't really need to implement a "strerror()" function but > should rather build it straight into printk()/vsprintf(). > > David Indeed we don't necessarily *need* a strerror() function per-se, but it is an addition to the libc support in the kernel. I'll leave it up to the community to decide rather or not it should be separate or integrated into vsprintf.c. Also, there is no POSIX or GNU libc extension (that I am aware of) to print an error name -- a real shame imho, so the name strerror_name() is pretty much artificial. Is there a significant overhead in having these functions exported? Personally, I would think that the only reason to have interest in an error name or message in the kernel is to output via printk, but I like to allow room for the conditions that I may not have thought of. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" 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 26559bd..d96d675 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -345,6 +345,12 @@ int num_to_str(char *buf, int size, unsigned long long num) #define SMALL 32 /* use lowercase in hex (must be 32 == 0x20) */ #define SPECIAL 64 /* prefix hex with "0x", octal with "0" */ +#if defined(CONFIG_PRINTK_ERROR_NAMES) || defined(CONFIG_PRINTK_ERROR_DESCS) +# define ERROR 128 /* error name and/or descrption */ +#else +# define ERROR 0 +#endif + enum format_type { FORMAT_TYPE_NONE, /* Just a string part */ FORMAT_TYPE_WIDTH, @@ -1346,6 +1352,75 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return number(buf, end, (unsigned long) ptr, spec); } +/** + * error - format an error code into a descrption + * + * Basically, we print the number, name and descrption, but only if enabled. + * If there is no string for an error number, it's printed as if the option is + * disabled. The format is basically equivalent to: + * + * printk("%d (%s: %s)", num, name, descrption); + * + * If there's no descrption, it would end up like: + * + * printk("%d (%s)", num, name); + * + * etc. + * + * Note that we ignore width and precision here, although they are applied to + * the numerical portion of the output in number(). + * + * Since we're initializing desc to NULL and then only setting it based upon + * a pre-compiler constant value, all code expecting it to be non-zero should + * compile-out when CONFIG_PRINTK_ERROR_DESCS is not enabled. + */ +#if defined(CONFIG_PRINTK_ERROR_NAMES) || defined(CONFIG_PRINTK_ERROR_DESCS) +static noinline_for_stack +char *error(char *buf, char *end, int err, struct printf_spec spec) +{ + const char *name = strerror_name(err); + const char *desc = NULL; + +#if defined(CONFIG_PRINTK_ERROR_DESCS) + desc = strerror(err); +#endif + + if (!(name || desc)) + return buf; + + if (&buf[1] < end) { + *buf++ = ' '; + *buf++ = '('; + } + + if (name) + while (buf < end && *name) + *buf++ = *name++; + + if (desc) { + if (name && &buf[1] < end) { + *buf++ = ':'; + *buf++ = ' '; + } + + while (buf < end && *desc) + *buf++ = *desc++; + } + + if (buf < end) + *buf++ = ')'; + + return buf; +} +#else +static inline +char *error(char *buf, char *end, int err, struct printf_spec spec) +{ + return buf; +} +#endif /* CONFIG_PRINTK_ERROR_NAMES || CONFIG_PRINTK_ERROR_DESCS */ + + /* * Helper function to decode printf style format. * Each call decode a token from the format and return the @@ -1501,12 +1576,20 @@ qualifier: case 'X': spec->base = 16; + if (fmt[1] == 'e') { + spec->flags |= ERROR; + ++fmt; + } break; case 'd': case 'i': spec->flags |= SIGN; case 'u': + if (fmt[1] == 'e') { + spec->flags |= ERROR; + ++fmt; + } break; default: @@ -1577,6 +1660,9 @@ qualifier: * %*ph[CDN] a variable-length hex string with a separator (supports up to 64 * bytes of the input) * %n is ignored + * %[idxXu]e will print the name of error code, if one exists. If the number + * doesn't match a known error code, it is printed as a normal signed int. + * TODO: explain CONFIG_PRINTK_ERROR* * * ** Please update Documentation/printk-formats.txt when making changes ** * @@ -1738,6 +1824,9 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) } str = number(str, end, num, spec); + + if (spec.flags & ERROR) + str = error(str, end, num, spec); } } @@ -2185,6 +2274,9 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf) } str = number(str, end, num, spec); + + if (spec.flags & ERROR) + str = error(str, end, num, spec); } /* default: */ } /* switch(spec.type) */ } /* while(*fmt) */
This adds an extension for the integral format specifier suffix of 'e', so that the format %[duxXo]e will result in printing an number (as before) in addition to a name and descrption for an error code, if such support is enabled and a name and descrption is found. My initial thought was to use the glibc extension of '%m', but this format specifier uses the value of libc's errno and adding a value breaks gcc's printf parsing. I'm not convinced that the 'e' extension is optimal, although there are only four instances of this pattern in the kernel that would need to be changed. git grep -E '".*%[#0\ +\-]*[0-9]*[hljzt]*[idoxXu]e' Alternatively, 'E' was another thought for a suffix as well. Signed-off-by: Daniel Santos <daniel.santos@pobox.com> --- lib/vsprintf.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+)