Date: Sun, 12 Nov 2006 16:05:39 -0700 (MST) From: "M. Warner Losh" <imp@bsdimp.com> To: sam@errno.com Cc: arm@freebsd.org, current@freebsd.org, keramida@freebsd.org, kabaev@gmail.com Subject: Re: [head tinderbox] failure on arm/arm Message-ID: <20061112.160539.-1350496508.imp@bsdimp.com> In-Reply-To: <4557825E.3070009@errno.com> References: <20061112132105.6bac38d6@kan.dnsalias.net> <20061112192810.GC1173@rambler-co.ru> <4557825E.3070009@errno.com>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <4557825E.3070009@errno.com>
Sam Leffler <sam@errno.com> writes:
: Ruslan Ermilov wrote:
: > On Sun, Nov 12, 2006 at 01:21:05PM -0500, Alexander Kabaev wrote:
: >> GCC expects 4-byte aligned structured on ARM but does not necessarily
: >> have to. We can change the default at the expense of possible more
: >> inefficient code generated and lost binary compatibility with other ARM
: >> OSes out there. So this is trade off between unclear performance
: >> penalty and an unspecified but certainly sizable number of other
: >> landmines like this lurking on the code.
: >>
: >> We should decide what evil we regard as lesser.
: >>
: > This is the only buildworld problem so far on FreeBSD/ARM, so my
: > feeling is that we can actually benefit from leaving it "as is",
: > as it has a potential of making our code more portable. Of course
: > if binary compatibility for structs across platforms is an issue,
: > a structure should be "packed", because otherwise the C standard
: > says that "Each non-bit-field member of a structure or union object
: > is aligned in an implementation-defined manner appropriate to its
: > type."
: >
: > On the other hand, having GCC align "struct foo { char x[11]; }"
: > on a four-byte boundary (other than for backward compatibility)
: > doesn't make too much sense to me.
: >
: > I don't know GCC rules for alignment of structure members. For
: > example, if it's guaranteed (in GCC) that offsetof(struct foo, bar)
: > is always 1 for "struct foo { char foo; char bar; }" (without the
: > "packed" attribute) on all platforms and OSes GCC supports?
: > I'd expect the latter to be "4" for FreeBSD/ARM but fortunately
: > it stays "1", i.e., only the structure alignment is affected,
: > and not of structure members (which is POLA but makes the 4 byte
: > for structure alignment questionable).
:
: This issue appears to have broken if_bridge. On my avila board I align
: rx packets to be aligned s.t. the ip header lands on a 32-bit boundary.
: This results in the ethernet header being 2-byte aligned which is ok on
: other platforms. But the compiler takes this code in bridge_forward:
:
: /*
: * If the interface is learning, and the source
: * address is valid and not multicast, record
: * the address.
: */
: if ((bif->bif_flags & IFBIF_LEARNING) != 0 &&
: ETHER_IS_MULTICAST(eh->ether_shost) == 0 &&
: (eh->ether_shost[0] == 0 &&
: eh->ether_shost[1] == 0 &&
: eh->ether_shost[2] == 0 &&
: eh->ether_shost[3] == 0 &&
: eh->ether_shost[4] == 0 &&
: eh->ether_shost[5] == 0) == 0) {
: (void) bridge_rtupdate(sc, eh->ether_shost,
: src_if, 0, IFBAF_DYNAMIC);
: }
:
: and converts the 6 byte compares to a 32-bit and 16-bit compare. Since
: the data is only 16-bit aligned the 32-bit load faults.
Yea, that's clearly bogus of it. It does this because it thinks that
eh is going to be 4-byte aligned, which it isn't in this case. I
think that we may need to change:
/*
* Structure of a 10Mb/s Ethernet header.
*/
struct ether_header {
u_char ether_dhost[ETHER_ADDR_LEN];
u_char ether_shost[ETHER_ADDR_LEN];
u_short ether_type;
};
to be
struct ether_header {
u_char ether_dhost[ETHER_ADDR_LEN];
u_char ether_shost[ETHER_ADDR_LEN];
u_short ether_type;
} __packed;
since that would fit.
There's one caveat that I'd caution people about. NetBSD had lots of
issues with gcc4 and packed when the struct doesn't need to be packed.
But, I must say, that they do flag these as packed:
/*
* Ethernet address - 6 octets
* this is only used by the ethers(3) functions.
*/
struct ether_addr {
u_int8_t ether_addr_octet[ETHER_ADDR_LEN];
} __attribute__((__packed__));
/*
* Structure of a 10Mb/s Ethernet header.
*/
struct ether_header {
u_int8_t ether_dhost[ETHER_ADDR_LEN];
u_int8_t ether_shost[ETHER_ADDR_LEN];
u_int16_t ether_type;
} __attribute__((__packed__));
(note: in FreeBSD we have #define in sys/cdefs.h:
#define __packed __attribute__((__packed__))
)
: So the point is that just because things compile doesn't necessarily
: mean they work. And worse code that works on many/most other
: architectures may not work.
I've fought this same issue in the boot code (look at boot2 for how I
worked around it). The arm really really hates unaligned accesses...
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061112.160539.-1350496508.imp>