From owner-freebsd-current@FreeBSD.ORG Sun Nov 12 23:07:00 2006 Return-Path: X-Original-To: current@freebsd.org Delivered-To: freebsd-current@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 199B616A415; Sun, 12 Nov 2006 23:07:00 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (vc4-2-0-87.dsl.netrack.net [199.45.160.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id B5F8143D8C; Sun, 12 Nov 2006 23:06:26 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.13.4/8.13.4) with ESMTP id kACN5313081507; Sun, 12 Nov 2006 16:05:03 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Sun, 12 Nov 2006 16:05:39 -0700 (MST) Message-Id: <20061112.160539.-1350496508.imp@bsdimp.com> To: sam@errno.com From: "M. Warner Losh" In-Reply-To: <4557825E.3070009@errno.com> References: <20061112132105.6bac38d6@kan.dnsalias.net> <20061112192810.GC1173@rambler-co.ru> <4557825E.3070009@errno.com> X-Mailer: Mew version 4.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.0 (harmony.bsdimp.com [127.0.0.1]); Sun, 12 Nov 2006 16:05:04 -0700 (MST) Cc: arm@freebsd.org, current@freebsd.org, keramida@freebsd.org Subject: Re: [head tinderbox] failure on arm/arm X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 12 Nov 2006 23:07:00 -0000 In message: <4557825E.3070009@errno.com> Sam Leffler 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