From owner-cvs-src@FreeBSD.ORG Mon Nov 13 09:26:12 2006 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 77CBC16AEA4; Mon, 13 Nov 2006 09:26:12 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1-3.pacific.net.au [61.8.2.210]) by mx1.FreeBSD.org (Postfix) with ESMTP id CD64B43D5D; Mon, 13 Nov 2006 09:26:11 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.2.163]) by mailout1.pacific.net.au (Postfix) with ESMTP id 316BA32881D; Mon, 13 Nov 2006 19:30:14 +1100 (EST) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy2.pacific.net.au (Postfix) with ESMTP id 84EE92740D; Mon, 13 Nov 2006 19:30:12 +1100 (EST) Date: Mon, 13 Nov 2006 19:30:11 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Joseph Koshy In-Reply-To: <200611130428.kAD4ST0U093715@repoman.freebsd.org> Message-ID: <20061113173927.Q75708@delplex.bde.org> References: <200611130428.kAD4ST0U093715@repoman.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/include ar.h X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Nov 2006 09:26:12 -0000 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 , 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 . 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 . 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. 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. Bruce