cvs commit: src/include ar.h

[Bruce Evans]

On Mon, 13 Nov 2006, Joseph Koshy wrote:

> jkoshy      2006-11-13 04:28:29 UTC
>
>  FreeBSD src repository
>
>  Modified files:
>    include              ar.h
>  Log:
>  Attempt to improve application portability by marking `struct ar_hdr'
>  as `packed'.
>
>  The C standard leaves the alignment of individual members of a C
>  struct upto the implementation, so pedantically speaking portable
>  code cannot assume that the layout of a `struct ar_hdr' in memory
>  will match its layout in a file.  Using a __packed attribute
>  declaration forces file and memory layouts for this structure to
>  match.
>
>  Submitted by:   ru

I don't see how this can be more portable.  It uses an unportable
extension, but packing is automatic on all compilers that are known
to support this extension.  On compilers that are not known to support
this extension (all except gcc >= 2.7 and icc), using __packed gives
a syntax error and thus changes code that is portable in practice into
code that doesn't compile.  On lints that known not to support this
extension (FreeBSD lint), linting for portability is broken by not
giving the syntax error.

In <ar.h>, all struct members are char arrays so there will be no
padding in practice.  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.

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:
- 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).

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.

<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.

Bruce