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

[ron minnich]

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/53d05fc8/attachment.html>