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 rminnich@gmail.com wrote:
On Tue, Jul 26, 2016 at 9:25 PM Zeh, Werner werner.zeh@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
-
ron minnich