cvs commit: src/include ar.h

[M. Warner Losh]

In message: <20061113173927.Q75708 at delplex.bde.org>
            Bruce Evans <bde at zeta.org.au> writes:
: In <ar.h>, all struct members are char arrays so there will be no
: padding in practice.

Actually, that isn't the case at all.  There are many systems that
pad/align structs to 4 byte boundaries regardless.  NetBSD has many of
these places tagged with __packed since they run on more architectures
than they can hand tweek for.

: Most system headers depend on the padding being
: the same for all compilers, even when the struct member types are very
: varied.  Many system headers use explicit manual padding to try to
: give a fixed layout.  This works in practice, and the not-unused file
: system ffs and the not-unused networking component netinet depend on
: it working.  Most file systems probably depend on it working across
: OS's for very varied struct types.  One exception is msdosfs -- since
: msdosfs's disk data structures are poorly laid out even for an 8-bit
: system, they are almost perfectly misaligned even for a 16-bit system,
: so manual packing is not practical, and msdosfs declares most of the
: structures as byte arrays in much the same way as <ar.h>.  It doesn't
: go as far as using a single array of bytes with fake struct members
: defined via offsets into the array, as might be required to be portable
: in theory.

"almost" "in theory".  These are nice words, but don't match reality.
On the arm, the __packed is absolutely required to not only compile,
but produce correct code.

: netinet uses __packed a bit, but this just gives unportability and
: pessimizations in practice.  E.g., struct ip is manually packed and
: has a natural 4-byte alignment requirement, but declaring it as __packed
: reduces its alignment requirement to 1 byte; thus in internal structs
: like "struct x { int16_t x_foo; struct ip x_ip; };", the x_ip is
: normally misaligned on a 16-bit boundary and thus:

struct ip should be packed.  It is the wire format of a data
structure, and the compiler is free to add extra padding that you
don't think is necessary.

: - the layout of the internal struct is unportable in practice instead of
:    unportable in theory
: - compilers for arches with strict alignment requirements have to
:    generate extra instructions to access the 32-bit members in struct
:    ip (just 2 in_addr_t's), so the accesses are pessimized and not
:    atomic.  gcc on i64's seems to do this correctly.  The pessimization
:    is by a factor of 14 according to the instruction count (a single
:    32-bit load instruction for an in_addr_t gets replaced by 4 8-bit
:    load instructions and 10 more instructions to combine the bytes).
: - code like "struct x *xp; ... foo(&xp->x_ip.ip_src);" is broken on
:    on arches with strict alignment requirements.  After the address of
:    the in_addr_t is passed to another function, the compiler can no
:    longer easiliy see that it has to generate pessimal loads for accesses
:    to the in_addr_t.  A variant of this bug occurred in practice in
:    rev.1.18 of <netipx/ipx.h>.  There, almost eveything is naturally
:    aligned to a 16-bit boundary, and __packed is misused to give 8-bitr
:    alignment as a bad side effect.  Rev.1.18 added a 32-bit member in
:    one of the structs contained in the __packed struct.  This should
:    have just broken binary compatibility and might not have mattered
:    depending on how internal the __packed struct is.  Instead, it gave
:    misaligned 32-bit values instide the __packed struct, thus defeating
:    the reason for existence of rev.1.18, since the reason was to add a
:    union hack to allow accessing "char c_net[4];" as "u_int u_net;", but
:    union hacks like this depend on not using __packed in any struct
:    containing c/u_net (recursively).

There's no guarantee that struct ip is 4 byte aligned from the
drivers.  In fact, there are several that require the packet to start
on a 4 byte boundary and lack scatter gather, thus guaranteeing that
struct ip is not 4 byte aligned.  This works well for almost all the
code in the tree (the one that it doesn't work for is in the nfs root
code somewhere).

: gcc -Wpacked can be used to find bugs like this.  See gcc.info for more
: details.  Passing the address of a non-char member in a packed struct
: should be an error, but gcc doesn't seem to diagnose this.

Agreed.

: <netinet/ip.h> was broken by adding __packed in the same commit that
: removed an unportable bitfield from it.  The bitfield was the actual
: problem.  Bitfields should never be used in structs that have to match
: an externally imposed layouts since they are unportable in practice.
: gcc's bitfields are especially unportable.  E.g. "int:8" gives the
: same alignment requirement as int, and this is undocumented.  In gcc,
: you have to use "char:8" or __packed to get 8-bit alignment of structs
: containing bitfields, but these are constraint and syntax errors in
: C.
:
: Using __packed in <ar.h> isn't as bad as in <netinet/ip.h> since struct
: ar has a natural 1-byte alignment requirement so using __packed is a
: no-op in practice.

Except on the arm, where it is critically required for the generated
code to be correct.

Warner