svn commit: r209057 - head/sys/ufs/ffs

[Bruce Evans]

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