svn commit: r203343 - head/sys/netinet

[Luigi Rizzo]

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 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
> :