Date: Tue, 2 Feb 2010 03:01:49 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Luigi Rizzo <luigi@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r203343 - head/sys/netinet Message-ID: <20100202012830.B1230@besplex.bde.org> In-Reply-To: <201002011413.o11EDiPm074656@svn.freebsd.org> References: <201002011413.o11EDiPm074656@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100202012830.B1230>