From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753831AbbLCXeL (ORCPT ); Thu, 3 Dec 2015 18:34:11 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:44260 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751492AbbLCXeJ (ORCPT ); Thu, 3 Dec 2015 18:34:09 -0500 Date: Thu, 3 Dec 2015 15:34:07 -0800 From: Andrew Morton To: Andy Shevchenko Cc: Joe Perches , Rasmus Villemoes , Kees Cook , Martin Kletzander , Andy Shevchenko , Maurizio Lombardi , Tejun Heo , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 04/14] lib/vsprintf.c: expand field_width to 24 bits Message-Id: <20151203153407.85761faeab07e7c2fed84e81@linux-foundation.org> In-Reply-To: <1449178138.15393.161.camel@linux.intel.com> References: <1449175873-1780-1-git-send-email-linux@rasmusvillemoes.dk> <1449175873-1780-5-git-send-email-linux@rasmusvillemoes.dk> <1449176047.17296.4.camel@perches.com> <1449178138.15393.161.camel@linux.intel.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 03 Dec 2015 23:28:58 +0200 Andy Shevchenko 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.