Re: [PATCH v3 04/14] lib/vsprintf.c: expand field_width to 24 bits

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 03 Dec 2015 23:28:58 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, 2015-12-03 at 12:54 -0800, Joe Perches wrote:
> > On Thu, 2015-12-03 at 21:51 +0100, Rasmus Villemoes 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.
> > > 
> > > [1] http://thread.gmane.org/gmane.linux.kernel/2034835
> > 
> > Thanks for keeping at this Rasmus.
> > This seems quite reasonable.
> 
> I like most of the stuff here, though, Joe, can we avoid open-coded
> BUILD_BUG_ON()?

Well we could just do

--- a/lib/vsprintf.c~lib-vsprintfc-expand-field_width-to-24-bits-fix
+++ a/lib/vsprintf.c
@@ -386,7 +386,6 @@ struct printf_spec {
 	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,
@@ -400,6 +399,8 @@ char *number(char *buf, char *end, unsig
 	int i;
 	bool is_zero = num == 0LL;
 
+	BUILD_BUG_ON(sizeof(struct printf_spec) != 8);
+
 	/* locase = 0 or 0x20. ORing digits or letters with 'locase'
 	 * produces same digits or (maybe lowercased) letters */
 	locase = (spec.flags & SMALL);

Which is better than open-coding it, IMO.



I've been fiddling with a BUILD_BUG_ON which works outside functions
using gcc's __COUNTER__ - something like

#define BBO(expr) typedef char __bbo##__COUNTER__[1-2*(!!expr)]

BBO(1 == 1);
BBO(2 == 2);

but that comes out as

typedef char __bbo__COUNTER__[1-2*(!!1 == 1)];
typedef char __bbo__COUNTER__[1-2*(!!2 == 2)];

instead of

typedef char __bbo0[1-2*(!!1 == 1)];
typedef char __bbo1[1-2*(!!2 == 2)];

There's some trick here but I've forgotten what it is.