From owner-svn-src-all@FreeBSD.ORG Mon Feb 1 18:30:48 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 37A521065679; Mon, 1 Feb 2010 18:30:48 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238]) by mx1.freebsd.org (Postfix) with ESMTP id A3FD78FC26; Mon, 1 Feb 2010 18:30:47 +0000 (UTC) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id EA111730A1; Mon, 1 Feb 2010 19:39:23 +0100 (CET) Date: Mon, 1 Feb 2010 19:39:23 +0100 From: Luigi Rizzo To: "M. Warner Losh" Message-ID: <20100201183923.GA32901@onelab2.iet.unipi.it> References: <201002011413.o11EDiPm074656@svn.freebsd.org> <20100202012830.B1230@besplex.bde.org> <20100201.103803.850602504823287588.imp@bsdimp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100201.103803.850602504823287588.imp@bsdimp.com> User-Agent: Mutt/1.4.2.3i Cc: svn-src-head@FreeBSD.org, luigi@FreeBSD.org, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, brde@optusnet.com.au Subject: Re: svn commit: r203343 - head/sys/netinet X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Feb 2010 18:30:48 -0000 On Mon, Feb 01, 2010 at 10:38:03AM -0700, M. Warner Losh wrote: > I concur with Bruce's assessment. Leave well enough alone. We don't > support those other compilers in the rest of the tree. These are userland-visible system headers so the change is not for building the system but for building generic userland programs on FreeBSD with other compilers. This said (sorry, i did not yet see Bruce's comment except in your email) if i understand Bruce's comment well, the problem is that no matter how we code it, bitfield layout is non specified, and we need non-portable __packed and __align and actual verification that things work as we want with a given compiler. > In particular, ARM generally gets broken when people naively monkey > with these sorts of thing. I'll be quite put-out if this is another > such change. Did you test it? No i did not test it on ARM. But i cannot think how a compiler would pack a u_int bitfield to 1 byte and refuse to do the same with an u_char (or uint8_t if you like it better). At a quick test, bitfields declared using u_char or *int8_t grep -Er 'int8_t|u_char' ~/FreeBSD/head/sys | grep ':[1-9]' | grep -v .svn appear approximately 800 times in 60 files including a number of device drivers, sys/dev/ciss/cissreg.h, sys/dev/ciss/cissio.h, sys/dev/usb/usbdi.h and stuff that might be arm-related e.g. sys/dev/usb/controller/avr32dci.h sys/dev/usb/controller/atmegadci.h On these grounds (we already have 800 such instances, and my changes are meant to improve compatibility of our system) I'd like the changes to remain (possibly replacing u_char with uint8_t if that looks better). cheers luigi > Warner > > In message: <20100202012830.B1230@besplex.bde.org> > Bruce Evans writes: > : On Mon, 1 Feb 2010, Luigi Rizzo wrote: > : > : > Log: > : > use u_char instead of u_int for short bitfields. > : > : This clobbers my ip.h rev.1.15 and tcp.h rev.1.9 (1998), which fixed > : the type of these struct members. In C, bit-fields have type _Bool, > : int, signed int or unsigned int. Other types are unporortable > : (common extensions; C99 J.5.8). > : > : > For our compiler the two constructs are completely equivalent, but > : > some compilers (including MSC and tcc) use the base type for > : > alignment, > : > which in the cases touched here result in aligning the bitfields > : > to 32 bit instead of the 8 bit that is meant here. > : > : Not quite true. gcc also uses the base type for alignment, but > : doesn't > : document the details AFAIK. The alignment requirements for the base > : type > : bogusly affects the alignment of the struct: the struct is padded at > : the > : end as if the base type is use for a non-bit-field struct member. > : Thus > : > : sizeof(struct { int8_t m:1; }) == 1; /* on normal 32-bit arches */ > : sizeof(struct { int16_t m:1; }) == 2; /* on normal 32-bit arches */ > : sizeof(struct { int_32_t m:1; }) == 4; /* on normal 32-bit arches */ > : sizeof(struct { int64_t m:1; }) == 8; /* on 64-bit arches */. > : > : However, gcc is not so broken as to give this excessive alignment for > : the bit-fields themselves. Thus > : > : sizeof(struct c { char c[3]; int_32_t m:1; }) == 4; > : > : The other compilers apparently extend the broken alignment to the > : bit-fields themselves. This is permitted by C99, but it is unwanted > : and is especially undesirable since C99 requires the _Bool/int/u_int > : type and doesn't provide any control over the alignment. > : > : C99 doesn't even require the alignment of bitfields to be documented > : AFAICS. It says that the alignment is "unspecified" (meaning surely > : that C99 doesn't specify it and not so surely that implementations > : document it, but implementations should document it). Anyway, the > : alignment is too MD to depend on, so bit-fields are simply unusable > : to lay out structs where the layout must be precise. (It is > : technically > : impossible to control the layout of a struct even without bit-fields, > : but only bit-fields are certain to cause problems.) > : > : > Note that almost all other headers where small bitfields > : > are used have u_int8_t instead of u_int. > : > : There is a lot of broken code out there, and now a lot in FreeBSD, > : though > : I fixed all instances of this bug in FreeBSD GENERIC during 1993-1998. > : > : > MFC after: 3 days > : > : Backout after: < 3 days. > : > : > Modified: > : > head/sys/netinet/ip.h > : > head/sys/netinet/tcp.h > : > > : > Modified: head/sys/netinet/ip.h > : > ============================================================================== > : > --- head/sys/netinet/ip.h Mon Feb 1 13:30:06 2010 (r203342) > : > +++ head/sys/netinet/ip.h Mon Feb 1 14:13:44 2010 (r203343) > : > @@ -48,11 +48,11 @@ > : > */ > : > struct ip { > : > #if BYTE_ORDER == LITTLE_ENDIAN > : > - u_int ip_hl:4, /* header length */ > : > + u_char ip_hl:4, /* header length */ > : > ip_v:4; /* version */ > : > #endif > : > #if BYTE_ORDER == BIG_ENDIAN > : > - u_int ip_v:4, /* version */ > : > + u_char ip_v:4, /* version */ > : > ip_hl:4; /* header length */ > : > #endif > : > u_char ip_tos; /* type of service */ > : > @@ -142,11 +142,11 @@ struct ip_timestamp { > : > u_char ipt_len; /* size of structure (variable) */ > : > u_char ipt_ptr; /* index of current entry */ > : > #if BYTE_ORDER == LITTLE_ENDIAN > : > - u_int ipt_flg:4, /* flags, see below */ > : > + u_char ipt_flg:4, /* flags, see below */ > : > ipt_oflw:4; /* overflow counter */ > : > #endif > : > #if BYTE_ORDER == BIG_ENDIAN > : > - u_int ipt_oflw:4, /* overflow counter */ > : > + u_char ipt_oflw:4, /* overflow counter */ > : > ipt_flg:4; /* flags, see below */ > : > #endif > : > union ipt_timestamp { > : > > : > : struct ip normally doesn't cause any problems, since although it > : doesn't > : contain any ints which would force its size to a multiple of 4 to > : satisyf alignment requirements, it happens to have size a multiple of > : 4 > : anyway (20). Thus gcc's bogus alignment for the struct doesn't change > : anything. The other compilers are apparently more broken. > : > : It is not easy to fix broken declarations in central headers like > : this. > : wollman tried to do it for some of the above in rev.1.7 (IP_VHL), but > : failed, and the attempt was axed in rev.1.20. > : > : Rev.1.20 also introduced related unportability, ugliness and > : pessimizations > : for struct ip -- it added the __packed directive. This was spelled > : unportably using __attribute__ ((); the unportability was fixed in > : 1.21. Using __packed results in all accesses to members of the struct > : being done as if the struct has alignment 1. On i386, this is only > : slightly slower for accessing the shorts in struct ip, but on machines > : with strict alignment requirements the compiler has to generate slower > : code to access the shorts a byte at a time and there may be atomicity > : issues. > : > : Rev.1.30 attempted to fix this by also adding the __aligned(4) > : attribute. > : I haven't checked that this works. It adds additional unportability > : and > : ugliness. > : > : Apparently the unportable ugliness of __packed and __aligned is not > : enough > : to actually work for the other compilers. If __packed is unavailable, > : then it should cause a syntax error. If it is available, then it > : should > : actually work, and actually working involves not padding the > : bit-fields. > : Then __aligned(4) is needed less strongly to clean up the mess from > : using > : __packed. > : > : > Modified: head/sys/netinet/tcp.h > : > ============================================================================== > : > --- head/sys/netinet/tcp.h Mon Feb 1 13:30:06 2010 (r203342) > : > +++ head/sys/netinet/tcp.h Mon Feb 1 14:13:44 2010 (r203343) > : > @@ -52,11 +52,11 @@ struct tcphdr { > : > tcp_seq th_seq; /* sequence number */ > : > tcp_seq th_ack; /* acknowledgement number */ > : > #if BYTE_ORDER == LITTLE_ENDIAN > : > - u_int th_x2:4, /* (unused) */ > : > + u_char th_x2:4, /* (unused) */ > : > th_off:4; /* data offset */ > : > #endif > : > #if BYTE_ORDER == BIG_ENDIAN > : > - u_int th_off:4, /* data offset */ > : > + u_char th_off:4, /* data offset */ > : > th_x2:4; /* (unused) */ > : > #endif > : > u_char th_flags; > : > > : > : struct tcphdr has some uint32_t's in it, so again the bogus gcc > : alignment > : has no effect (but things need to be packed even more carefully to > : avoid > : padding; fortunately, the wire layout happens to align everything). > : > : struct tcphdr hasn't been affected by the __packed/__aligned(4) > : unportable ugliness. > : > : Bruce > :