svn commit: r203343 - head/sys/netinet

[Luigi Rizzo]

On Mon, Feb 01, 2010 at 12:03:07PM -0700, M. Warner Losh wrote:
> 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. ...

not 100% sure. 

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

yes and no
> : > 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.

fair enough. I realize i myself said that we need "actual verification
that things work as we want with a given compiler" but did not go
further after the 'grep' below returnes so many relevant hits.

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

thanks a lot.

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

i cannot comment on this -- certainly this specific change 12-15 years
ago did not seem to be made to fix actual breakage.

cheers
luigi