<div dir="ltr">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.<div><br></div><div>thanks and apologies</div><div><br></div><div>ron</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Jul 27, 2016 at 10:07 AM ron minnich <<a href="mailto:rminnich@gmail.com">rminnich@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Jul 26, 2016 at 9:25 PM Zeh, Werner <<a href="mailto:werner.zeh@siemens.com" target="_blank">werner.zeh@siemens.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><br>
<br>
In the case of ACPI we need to provide a table which has constrains on the used data types and alignment<br>
as the contents of the table will be interpreted by the ACPI interpreter of the OS.<br>
So if we omit the usage of packed it may result in gaps in between the single members of the data structure<br>
which will in turn lead to errors while the OS interprets the contents.<br>
<br>
So in my point of view, we really need this structure to be packed in this case.<br>
<br><br></blockquote><div><br></div><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>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.</div><div><br></div><div>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. </div><div><br></div><div>We've got code in the repo which does this. Please look at util/cbfstool/</div><div>for example code, or see the mptable generation code. just git grep xdr to see some usage.</div><div><br></div><div>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</div><div><a href="https://sourceware.org/bugzilla/show_bug.cgi?id=5070" target="_blank">https://sourceware.org/bugzilla/show_bug.cgi?id=5070</a><br></div><div><br></div><div>This one section is applicable:</div><div>"Tom Hunter 2007-10-02 05:45:39 UTC</div><div>ARM is one of the architectures supported by glibc. You may not like it, but it </div><div>is a fact.</div><div><br></div><div>Independently of the architecture, the padding done is not valid. You can't </div><div>(and shouldn't) make any assumption about the alignment and associated padding </div><div>done by the compiler for any architecture. GCC is free to change the alignment </div><div>rules in any future versions. It seems rather silly to have the assert() which </div><div>is meant to verify at runtime that your invalid assumption holds true.</div><div><br></div><div>I would also suggest that you don't use structures to format packets for </div><div>networking. Packets for networking should be treated as byte streams to avoid </div><div>any alignment/padding/byte-order issues. A standard way of doing this is </div><div>something like this:</div><div><br></div><div>unsigned char buf[MaxPacketSize];</div><div>unsigned char *bp;</div><div>int skt;</div><div>size_t len;</div><div><br></div><div>...</div><div>bp = buf;</div><div>*bp++ = ...;</div><div>*bp++ = ...;</div><div>*bp++ = ...;</div><div>...</div><div>len = send(skt, buf, bp - buf, 0);</div><div>"</div><div><br></div><div>The key realization is that Tom is right even when you're packing to memory.</div></div></div><div dir="ltr"><div class="gmail_quote"><div><br></div><div>ron</div></div></div></blockquote></div>