Re: %j for printf(9)

On Tue, 28 May 2002, Dima Dorfman wrote:

> Dag-Erling Smorgrav <[email protected]> wrote:
> > Dima Dorfman <[email protected]> writes:
> > > Attached is a patch that implements the %j length modifier in
> > > printf(9).
> >
> > Here's an alternative (IMHO less disruptive) patch, which also fixes
> > the default case.
>
> I still like my restructure, but since other people don't seem to
> share my opinion, I'm fine doing it another (your) way.  That said, I
> think your patch has some bugs that mine doesn't.  For example, this:
>
> 	printf("%ld\n", -4);
>
> yields "4294967292" with your patch, but not with mine (mine, and
> printf(3), yield "-4").  I think the attached patch (relative to
> subr_prf.c *with* your patch applied) fixes it.

Both are not incorrect, since the behaviour is undefined :-).
printf("%ld", -4L) would be a better example.

My objections to your restructuring are mostly related to getting the
conversions for this and other things things right.  Forcing everything
into a uintmax_t isn't really simple or correct, and the restructuring
depends on it.

> +++ subr_prf.c	Tue May 28 00:09:30 2002
> @@ -658,19 +658,19 @@
>  			if (jflag)
>  				num = va_arg(ap, uintmax_t);
>  			else if (qflag)
> -				num = va_arg(ap, u_quad_t);
> +				num = (u_quad_t)va_arg(ap, u_quad_t);
>  			else if (lflag)
> -				num = va_arg(ap, u_long);
> +				num = (u_long)va_arg(ap, u_long);
>  			else
> -				num = va_arg(ap, u_int);
> +				num = (u_int)va_arg(ap, u_int);

These casts are all no-ops.   va_arg() already gives the correct type.  This
type is unsigned, so there are no surprises promoting it to the type of `num'
(uintmax_t).

>  			goto nosign;
>  fetch_number:
>  			if (jflag)
> -				num = va_arg(ap, uintmax_t);
> +				num = va_arg(ap, intmax_t);

An (explicit but unnecessary) cast to uintmax_t would make some sense
here, since we are changing the value of an intmax_t to store it into
a uintmax_t.  The conversion happens by default but only clearly works
right on 2's complement machines.

>  			else if (qflag)
> -				num = va_arg(ap, u_quad_t);
> +				num = (quad_t)va_arg(ap, quad_t);

This cast also has no effect.  Changing the type in the va_arg() also
has no effect on any supported machine, but is necessary on machines
with quad_t smaller than intmax_t.  Explicit but null casts, first to
intmax_t and then to uintmax_t, might be good to show what is happening
here.

>  			else if (lflag)
> -				num = va_arg(ap, u_long);
> +				num = (long)va_arg(ap, long);

u_long was wrong in the same way as u_quad_t was wrong, except the wrongness
actually affects supported machines (ones with long smaller than intmax_t).

>  			else
>  				num = sign ? (uintmax_t)va_arg(ap, int) :
>  				    va_arg(ap, u_int);

I think this `sign' test is needed for the lflag and qflag cases too.  It
is needed to get the correct sign extension for promotion of types smaller
than `num'.  In rev.1.1, `int' was the only such type, but in theory
even long longs might be smaller that intmax_t.  Support for machines
with longs smaller than quads seems to be broken in -current.  This only
affects the %r and %z formats.

Bruce


To Unsubscribe: send mail to [email protected]
with "unsubscribe freebsd-audit" in the body of the message