[coreboot] What is the best way to treat warnings reported by checkpatch.pl

[ron minnich]

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 at gmail.com> wrote:

> On Tue, Jul 26, 2016 at 9:25 PM Zeh, Werner <werner.zeh at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20160727/951e6bc7/attachment.html>