Date: Mon, 1 Feb 2010 19:39:23 +0100 From: Luigi Rizzo <rizzo@iet.unipi.it> To: "M. Warner Losh" <imp@bsdimp.com> 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 Message-ID: <20100201183923.GA32901@onelab2.iet.unipi.it> In-Reply-To: <20100201.103803.850602504823287588.imp@bsdimp.com> References: <201002011413.o11EDiPm074656@svn.freebsd.org> <20100202012830.B1230@besplex.bde.org> <20100201.103803.850602504823287588.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <brde@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
> :
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100201183923.GA32901>