From owner-svn-src-all@FreeBSD.ORG Mon Feb 1 16:01:54 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 ECCCC1065679; Mon, 1 Feb 2010 16:01:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 66E598FC1D; Mon, 1 Feb 2010 16:01:52 +0000 (UTC) Received: from besplex.bde.org (c122-106-174-165.carlnfd1.nsw.optusnet.com.au [122.106.174.165]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o11G1npt017284 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 2 Feb 2010 03:01:50 +1100 Date: Tue, 2 Feb 2010 03:01:49 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Luigi Rizzo In-Reply-To: <201002011413.o11EDiPm074656@svn.freebsd.org> Message-ID: <20100202012830.B1230@besplex.bde.org> References: <201002011413.o11EDiPm074656@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org 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 16:01:54 -0000 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