From owner-cvs-all@FreeBSD.ORG Mon Nov 13 17:27:46 2006 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id ADE9916A4D0; Mon, 13 Nov 2006 17:27:46 +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 18B6443DFD; Mon, 13 Nov 2006 17:19:00 +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 kADHHV05097604; Mon, 13 Nov 2006 10:17:33 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Mon, 13 Nov 2006 10:18:08 -0700 (MST) Message-Id: <20061113.101808.-1540392146.imp@bsdimp.com> To: bde@zeta.org.au From: "M. Warner Losh" In-Reply-To: <20061113173927.Q75708@delplex.bde.org> References: <200611130428.kAD4ST0U093715@repoman.freebsd.org> <20061113173927.Q75708@delplex.bde.org> 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]); Mon, 13 Nov 2006 10:17:33 -0700 (MST) Cc: jkoshy@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org, cvs-src@freebsd.org Subject: Re: cvs commit: src/include ar.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Nov 2006 17:27:46 -0000 In message: <20061113173927.Q75708@delplex.bde.org> Bruce Evans writes: : In , 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 . 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 . 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. : 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 isn't as bad as in 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