From owner-svn-src-all@FreeBSD.ORG Fri Jun 11 19:26:32 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 060C1106566C; Fri, 11 Jun 2010 19:26:32 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id 8751B8FC24; Fri, 11 Jun 2010 19:26:30 +0000 (UTC) Received: from besplex.bde.org (c122-106-175-69.carlnfd1.nsw.optusnet.com.au [122.106.175.69]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o5BJQNJC031881 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 12 Jun 2010 05:26:29 +1000 Date: Sat, 12 Jun 2010 05:26:23 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Andriy Gapon In-Reply-To: <201006111826.o5BIQrj1054320@svn.freebsd.org> Message-ID: <20100612044012.Q3254@besplex.bde.org> References: <201006111826.o5BIQrj1054320@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r209057 - head/sys/ufs/ffs X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Jun 2010 19:26:32 -0000 On Fri, 11 Jun 2010, Andriy Gapon wrote: > Log: > ffs_softdep: change K&R in function defintions to ANSI prototypes > > Apparently it's bad when we first have an ANSI prototype in function > declaration, but then use K&R in its defintion. No, it's bad when the prototype doesn't match the definition. Then the behaviour is undefined. The actual behaviour in gcc is to silently modify the definition to match the prototype. This conforms to Standard C since undefined behaviour can be anything, but it is bad because it is unportable and rewards bad code. > Complaint from: clang clang apparently doesn't reward this particular bad code. > Modified: head/sys/ufs/ffs/ffs_softdep.c > ============================================================================== > --- head/sys/ufs/ffs/ffs_softdep.c Fri Jun 11 18:20:56 2010 (r209056) > +++ head/sys/ufs/ffs/ffs_softdep.c Fri Jun 11 18:26:53 2010 (r209057) > @@ -3251,12 +3251,8 @@ newjmvref(dp, ino, oldoff, newoff) > * the jsegdep when we're done. > */ > static struct jremref * > -newjremref(dirrem, dp, ip, diroff, nlink) > - struct dirrem *dirrem; > - struct inode *dp; > - struct inode *ip; > - off_t diroff; > - nlink_t nlink; The type mismatch was for nlink. THe K&R function takes an nlink arg of type __promoteof(nlink_t), but the prototype says that it takes an nlink arg of type nlink_t. Since nlink_t is uint16_t and int has > 16 bits on all supported machines, __promoteof(nlink_t) != nlink_t on any supported machines. The prototype would be correct on machines with 16-bit ints, so a protoytype that declared it as int would be incorrect on these machines, since then the promotion is u_int. __promoteof() is pseudo-code to handle this problem. Most working but still unportable prototypes for K&R functions ignore this problem and hard-code the promotion as int/u_int/long/etc. There would be type mismatches for ino_t if ints were larger than 32 bits, and type mismatches for off_t if ints were larger than 64 bits. Most prototypes for K&R functions completely ignore this problem. > +newjremref(struct dirrem *dirrem, struct inode *dp, struct inode *ip, > + off_t diroff, nlink_t nlink) > { > struct jremref *jremref; > > @@ -3271,13 +3267,8 @@ newjremref(dirrem, dp, ip, diroff, nlink > } > > static inline void > -newinoref(inoref, ino, parent, diroff, nlink, mode) > - struct inoref *inoref; > - ino_t ino; > - ino_t parent; > - off_t diroff; > - nlink_t nlink; > - uint16_t mode; Now there was also a type for the `mode' arg. Its type uint16_t may be a misspelling of mode_t. Though if_mode has type uint16_t, it has to have a type like mode_t to work, and the system type is used for almost everything else (ino_t and nlink_t here). The type mismatch problem is larger for mode_t than for ino_t, since mode_t is used in several syscall interfaces, including open(2) which is variadic so it cannot take the "mode_t mode" arg that it is documented to take in the FreeBSD man page. POSIX.1-2001 and POSIX.1-2004 are little better. They say that the third arg to open() is "taken" as type mode_t; they should say that the third arg is an integer converted to a mode_t, where the type of the integer is __promoteof(mode_t) which is hard to specify, or possibly that the type of the integer is int (if all the mode_t's needed for open() fit in the 15 value bits of a 16-bit int). > +newinoref(struct inoref *inoref, ino_t ino, ino_t parent, off_t diroff, > + nlink_t nlink, uint16_t mode) The prototype still exists and is more bogus than before for this function, since this function is inline. Inline functions need to be defined before they are used so that they are actually inlined (except -funit-at-a-time may allow them to be anywhere in a file), so a forward declaration for them is never needed. However, the forward declaration used to be sort of needed to turn the K&R definition into a prototype. However2, K&R compilers don't support inlining, so using K&R definitions for inline functions is bogus. > { > > inoref->if_jsegdep = newjsegdep(&inoref->if_list); > @@ -3296,12 +3287,8 @@ newinoref(inoref, ino, parent, diroff, n > * to have the correct FMT. > */ > static struct jaddref * > -newjaddref(dp, ino, diroff, nlink, mode) > - struct inode *dp; > - ino_t ino; > - off_t diroff; > - int16_t nlink; > - uint16_t mode; > +newjaddref(struct inode *dp, ino_t ino, off_t diroff, int16_t nlink, > + uint16_t mode) As above (non-inline case). > { > struct jaddref *jaddref; > > Bruce