Messages in this thread | | | Date | Tue, 24 Nov 2015 01:05:08 +0200 | Subject | Re: [PATCH 04/14] lib/vsprintf.c: expand field_width to 24 bits | From | Andy Shevchenko <> |
| |
On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > Maurizio Lombardi reported a problem [1] with the %pb extension: It > doesn't work for sufficiently large bitmaps, since the size is stashed > in the field_width field of the struct printf_spec, which is currently > an s16. Concretely, this manifested itself in > /sys/bus/pseudo/drivers/scsi_debug/map being empty, since the bitmap > printer got a size of 0, which is the 16 bit truncation of the actual > bitmap size. > > We do want to keep struct printf_spec at 8 bytes so that it can > cheaply be passed by value. The qualifier field is only used for > internal bookkeeping in format_decode, so we might as well use a local > variable for that. This gives us an additional 8 bits, which we can > then use for the field width. > > To stay in 8 bytes, we need to do a little rearranging and make the > type member a bitfield as well. For consistency, change all the > members to bit fields. gcc doesn't generate much worse code with these > changes (in fact, bloat-o-meter says we save 300 bytes - which I think > is a little surprising). > > I didn't find a BUILD_BUG/compiletime_assertion/... which would work > outside function context, so for now I just open-coded it.
And any objections to put it into vsnprintf() ?
> > [1] http://thread.gmane.org/gmane.linux.kernel/2034835 > > Reported-by: Maurizio Lombardi <mlombard@redhat.com> > Cc: Tejun Heo <tj@kernel.org> > Cc: Joe Perches <joe@perches.com> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > lib/vsprintf.c | 41 +++++++++++++++++++++-------------------- > 1 file changed, 21 insertions(+), 20 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 63ca52366049..01c3aa638582 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -380,13 +380,13 @@ enum format_type { > }; > > struct printf_spec { > - u8 type; /* format_type enum */ > - u8 flags; /* flags to number() */ > - u8 base; /* number base, 8, 10 or 16 only */ > - u8 qualifier; /* number qualifier, one of 'hHlLtzZ' */ > - s16 field_width; /* width of output field */ > - s16 precision; /* # of digits/chars */ > -}; > + unsigned int type:8; /* format_type enum */ > + signed int field_width:24; /* width of output field */ > + unsigned int flags:8; /* flags to number() */ > + unsigned int base:8; /* number base, 8, 10 or 16 only */ > + signed int precision:16; /* # of digits/chars */ > +} __packed; > +extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)]; > > static noinline_for_stack > char *number(char *buf, char *end, unsigned long long num, > @@ -1641,6 +1641,7 @@ static noinline_for_stack > int format_decode(const char *fmt, struct printf_spec *spec) > { > const char *start = fmt; > + char qualifier;
u8 ?
> > /* we finished early by reading the field width */ > if (spec->type == FORMAT_TYPE_WIDTH) { > @@ -1723,16 +1724,16 @@ precision: > > qualifier: > /* get the conversion qualifier */ > - spec->qualifier = -1; > + qualifier = 0; > if (*fmt == 'h' || _tolower(*fmt) == 'l' || > _tolower(*fmt) == 'z' || *fmt == 't') { > - spec->qualifier = *fmt++; > - if (unlikely(spec->qualifier == *fmt)) { > - if (spec->qualifier == 'l') { > - spec->qualifier = 'L'; > + qualifier = *fmt++; > + if (unlikely(qualifier == *fmt)) { > + if (qualifier == 'l') { > + qualifier = 'L'; > ++fmt; > - } else if (spec->qualifier == 'h') { > - spec->qualifier = 'H'; > + } else if (qualifier == 'h') { > + qualifier = 'H'; > ++fmt; > } > } > @@ -1789,19 +1790,19 @@ qualifier: > return fmt - start; > } > > - if (spec->qualifier == 'L') > + if (qualifier == 'L') > spec->type = FORMAT_TYPE_LONG_LONG; > - else if (spec->qualifier == 'l') { > + else if (qualifier == 'l') { > BUILD_BUG_ON(FORMAT_TYPE_ULONG + SIGN != FORMAT_TYPE_LONG); > spec->type = FORMAT_TYPE_ULONG + (spec->flags & SIGN); > - } else if (_tolower(spec->qualifier) == 'z') { > + } else if (_tolower(qualifier) == 'z') { > spec->type = FORMAT_TYPE_SIZE_T; > - } else if (spec->qualifier == 't') { > + } else if (qualifier == 't') { > spec->type = FORMAT_TYPE_PTRDIFF; > - } else if (spec->qualifier == 'H') { > + } else if (qualifier == 'H') { > BUILD_BUG_ON(FORMAT_TYPE_UBYTE + SIGN != FORMAT_TYPE_BYTE); > spec->type = FORMAT_TYPE_UBYTE + (spec->flags & SIGN); > - } else if (spec->qualifier == 'h') { > + } else if (qualifier == 'h') { > BUILD_BUG_ON(FORMAT_TYPE_USHORT + SIGN != FORMAT_TYPE_SHORT); > spec->type = FORMAT_TYPE_USHORT + (spec->flags & SIGN); > } else { > -- > 2.6.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
-- With Best Regards, Andy Shevchenko
| |