Date: Fri, 17 Nov 2006 20:48:13 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Yar Tikhiy <yar@comp.chem.msu.su> Cc: src-committers@freebsd.org, jkoshy@freebsd.org, cvs-all@freebsd.org, phk@phk.freebsd.dk, cvs-src@freebsd.org, "M. Warner Losh" <imp@bsdimp.com> Subject: Re: cvs commit: src/include ar.h Message-ID: <20061117201432.X11101@delplex.bde.org> In-Reply-To: <20061117065555.GE49602@comp.chem.msu.su> References: <20061113214928.P76443@delplex.bde.org> <20061113.101958.-861030824.imp@bsdimp.com> <20061116090412.GB37133@comp.chem.msu.su> <20061116.165207.1661914048.imp@bsdimp.com> <20061117065555.GE49602@comp.chem.msu.su>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 17 Nov 2006, Yar Tikhiy wrote: > On Thu, Nov 16, 2006 at 04:52:07PM -0700, M. Warner Losh wrote: >> In message: <20061116090412.GB37133@comp.chem.msu.su> >> Yar Tikhiy <yar@comp.chem.msu.su> writes: >> : Many years ago I was taught that comments in code could help to >> : avoid such clashes in software development. Is this true no more? ;-) >> >> You don't add comments like: >> >> i++; // Add one to i. >> >> This is a similar class. It is for any compiler that has differing >> alignment requirements than i386. But you do add comments like: i++; /* Don't add 1 to i (sic). */ when `i' is a const variable in magic device memory that needs to be used by read/add 1/write (where the non-atomicness of this doesn't matter for some reason). > This is an oversimplification of the case. If it were so simple, > no doubts about it would be raised. That's why I suggested adding > a comment explaining that historically struct ip was lucky to be > packed/aligned properly, but that wasn't backed by the C standard > in fact, and eventually architectures appeared, e.g., ARM, which > broke the false assumption. It's a rather edifying case. Then There is no luck about the alignment. Structs are always aligned, especially for arm, unless you break them using __packed. It never took much luck for the struct to be packed, since the struct has its largest members at the end so padding for alignment would affect most arches. It now doesn't take any luck to detect mispacking, since packing is now __CTASSERT()ed. Packing occurs naturally wthout using __packed for all arches supported by FreeBSD and Linux-2.6.10 (**), especially for arm since arm is on 32-bits so padding at the end to make the struct size a multiple of 64 bits would be weird. > you'll have a smaller chance of having to yell in capital letters > again, "DO NOT REMOVE IT. IT IS ABSOLUTELY REQUIRED FOR ARM TO > WORK RIGHT." -- hopefully, not only regarding struct ip. For that the comment should be something like: __packed; /* Align (sic) to work around bugs on arm (*). */ but I doubt that arm is that broken. (*) See this thread for more details. (**) Linux-2.6.10 seems to have only struct iphdr, and has gratuitous unportabilities in that like the ones that were removed in FreeBSD in the commit that added __packed: it uses bitfields to unportabalize the header length and version, and endianness ifdefs to work around the unportabilities; in addition, it uses bitfields of type char to additionally unportabalize the header length and version. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061117201432.X11101>