Ah well, Werner, I've had the error of my ways pointed out to me, turns out this packed stuff is a standard practice in coreboot now. I must have missed the memo. So, I'm not a fan but if that's how we do it, it's how we do it.
thanks and apologies ron On Wed, Jul 27, 2016 at 10:07 AM ron minnich <rminn...@gmail.com> wrote: > On Tue, Jul 26, 2016 at 9:25 PM Zeh, Werner <werner....@siemens.com> > wrote: > >> >> >> >> In the case of ACPI we need to provide a table which has constrains on >> the used data types and alignment >> as the contents of the table will be interpreted by the ACPI interpreter >> of the OS. >> So if we omit the usage of packed it may result in gaps in between the >> single members of the data structure >> which will in turn lead to errors while the OS interprets the contents. >> >> So in my point of view, we really need this structure to be packed in >> this case. >> >> >> > > no, please don't do this. We and others have made this mistake before and > it took a lot of work to unwind it where we did it. Using packed structs to > create aligned data in memory is a frequent source of bugs. > > If you're creating a desired data layout in memory, from a struct, using > packed will fail badly should we ever have a big endian machine, but it can > even have weird problems between gcc versions. When you pack data into > memory with a specified alignment, byte order, and padding, you are > converting it to an external data representation, i.e. XDR. You need to do > that explicitly, not by using packed structs. > > We've got code in the repo which does this. Please look at util/cbfstool/ > for example code, or see the mptable generation code. just git grep xdr to > see some usage. > > But using packed structs to try to force layout of data in memory is > almost always going to end badly. For a fun read, check this out > https://sourceware.org/bugzilla/show_bug.cgi?id=5070 > > This one section is applicable: > "Tom Hunter 2007-10-02 05:45:39 UTC > ARM is one of the architectures supported by glibc. You may not like it, > but it > is a fact. > > Independently of the architecture, the padding done is not valid. You > can't > (and shouldn't) make any assumption about the alignment and associated > padding > done by the compiler for any architecture. GCC is free to change the > alignment > rules in any future versions. It seems rather silly to have the assert() > which > is meant to verify at runtime that your invalid assumption holds true. > > I would also suggest that you don't use structures to format packets for > networking. Packets for networking should be treated as byte streams to > avoid > any alignment/padding/byte-order issues. A standard way of doing this is > something like this: > > unsigned char buf[MaxPacketSize]; > unsigned char *bp; > int skt; > size_t len; > > ... > bp = buf; > *bp++ = ...; > *bp++ = ...; > *bp++ = ...; > ... > len = send(skt, buf, bp - buf, 0); > " > > The key realization is that Tom is right even when you're packing to > memory. > > ron >
-- coreboot mailing list: coreboot@coreboot.org https://www.coreboot.org/mailman/listinfo/coreboot