From owner-cvs-src@FreeBSD.ORG Sat Mar 13 14:27:47 2004 Return-Path: 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 D9D0316A4CE; Sat, 13 Mar 2004 14:27:47 -0800 (PST) Received: from mailout2.pacific.net.au (mailout2.pacific.net.au [61.8.0.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2C79843D3F; Sat, 13 Mar 2004 14:27:47 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.0.87])i2DMRbnX023144; Sun, 14 Mar 2004 09:27:38 +1100 Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) i2DMRYGi001928; Sun, 14 Mar 2004 09:27:36 +1100 Date: Sun, 14 Mar 2004 09:27:34 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Tony Finch In-Reply-To: Message-ID: <20040314085842.K20090@gamplex.bde.org> References: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: Tom Rhodes cc: src-committers@freebsd.org cc: Ruslan Ermilov cc: cvs-all@freebsd.org cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sys/boot/arc/include arcfuncs.hsrc/sys/boot/i386/boot2 boot2.c src/sys/dev/aic7xxx/aicasm aicasm.c src/sys/dev/cx machdep.h src/sys/dev/ichsmb ichsmb.c src/sys/dev/iir iir.h src/s X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 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: Sat, 13 Mar 2004 22:27:48 -0000 On Sat, 13 Mar 2004, Tony Finch wrote: > On Sat, 13 Mar 2004, Ruslan Ermilov wrote: > > On Fri, Mar 12, 2004 at 01:45:45PM -0800, Tom Rhodes wrote: > > > > > > -#ifdef __GNUC__ > > > -#if __GNUC__ >= 2 > > > +#if defined(__GNUC__) || defined(__INTEL_COMPILER) > > > +#if __GNUC__ >= 2 || defined(__INTEL_COMPILER) > > > #pragma pack(4) > > > #endif > > > #endif > > > > These ifdefs are broken. > > > > #if (defined(__GNUC__) && __GNUC >= 2) || defined(__INTEL_COMPILER) > > > > would be more correct. > > Why, given that an undefined macro is equivalent to 0 in this context? I think there are only style bugs. The outer ifdefs have no effect in either the old version or the new version. Also, the ifdefs shouldn't be needed, and using "#pragma pack" is mainly a style bug. This pragma is used in only 2 headers in /sys (mcdreg.h and scdreg.h). We use __packed almost everywhere else. The definition of __packed is centralized so that only 1 ifdef is needed for it. A few broken places hard-code the gccism "__attribute__(())". This unportability is especially unneeded in mcdreg.h and scdreg.h. All structs in these files are wrapped with "#pragma pack(1)" and #pragma pack(4)" (except scdreg.h is missing the second pragma, so the first pragma leaks out of it). But these structs need packing less than most, since they consist mainly of char fields. Normal packing gives the same results as "#pragma pack(1)" for all reasonable compilers/machines, except possibly for some problematic bit-fields. In mcdreg.h: the main problems are here: % struct mcd_qchninfo { % u_char addr_type:4; % u_char control:4; % u_char trk_no; % u_char idx_no; % bcd_t trk_size_msf[3]; % u_char :8; % bcd_t hd_pos_msf[3]; % }; (bcd_t is u_char.) The u_char bit-fields can't be compiled by C compilers. If they were replaced by u_int and "#pragma pack(1)" were removed, then gcc would pack this struct internally (to 10 bytes) but pad it externally to have size a multiple of sizeof(u_int). Misdeclaring the bit-fields as u_char prevents the external padding. "#pragma pack(1)" seems to have no additional affect. It would just prevent the extern padding if the struct were declared in C. scdreg.h is similar except it has many more unportable bit-fields. Bruce