Date: Tue, 28 May 2002 19:15:23 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Dima Dorfman <dima@trit.org> Cc: Dag-Erling Smorgrav <des@ofug.org>, <audit@FreeBSD.ORG> Subject: Re: %j for printf(9) Message-ID: <20020528184446.W19885-100000@gamplex.bde.org> In-Reply-To: <20020528001615.4BAC93E5E@turbine.trit.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 28 May 2002, Dima Dorfman wrote:
> Dag-Erling Smorgrav <des@ofug.org> wrote:
> > Dima Dorfman <dima@trit.org> 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 majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020528184446.W19885-100000>