svn commit: r203343 - head/sys/netinet

[M. Warner Losh]

In message: <20100201183923.GA32901 at onelab2.iet.unipi.it>
            Luigi Rizzo <rizzo at iet.unipi.it> writes:
: 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.

Yes.  The brief version is that the we're trying to capture the wire
format in C, which provides no native way of doing that.  The
different byte ordering ifdefs, as well as the __packed and __aligned
stuff tries to cope in a way we hope is the best.

But my comment about not supporting these compilers is still relevant,
I think.  We have special code in sys/defs.h to support the compilers
we do...  So if we don't support the compiler the __packed and
__aligned macros are just defined away...

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

Right.  I cannot think of how the current ARM ABI does some of the
things that it does.

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

ciss isn't known to work on ARM...

: 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

but these are...

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

I'd like to at least verify that ARM works by actually testing it
rather than just speculating that it likely is OK.  I'll queue up a
build or two to make sure.

My main concern here is to make sure that things work, and we've had
better luck with u_int to date...

Warner



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