svn commit: r203343 - head/sys/netinet
M. Warner Losh
imp at bsdimp.com
Mon Feb 1 17:48:26 UTC 2010
I concur with Bruce's assessment. Leave well enough alone. We don't
support those other compilers in the rest of the tree.
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?
Warner
In message: <20100202012830.B1230 at besplex.bde.org>
Bruce Evans <brde at optusnet.com.au> 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
:
More information about the svn-src-all
mailing list