Date: Mon, 13 Nov 2006 10:18:08 -0700 (MST) From: "M. Warner Losh" <imp@bsdimp.com> To: bde@zeta.org.au Cc: jkoshy@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org, cvs-src@freebsd.org Subject: Re: cvs commit: src/include ar.h Message-ID: <20061113.101808.-1540392146.imp@bsdimp.com> In-Reply-To: <20061113173927.Q75708@delplex.bde.org> References: <200611130428.kAD4ST0U093715@repoman.freebsd.org> <20061113173927.Q75708@delplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <20061113173927.Q75708@delplex.bde.org>
Bruce Evans <bde@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061113.101808.-1540392146.imp>