Date: Wed, 29 Dec 2004 23:09:14 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Peter Wemm <peter@wemm.org> Cc: Archie Cobbs <archie@dellroad.org> Subject: Re: mpd@amd64 Message-ID: <20041229221042.X92684@delplex.bde.org> In-Reply-To: <200412281526.48335.peter@wemm.org> References: <1936407230.20041214120051@bk.ru> <200412281432.56309.peter@wemm.org> <41D1E0B2.3020500@dellroad.org> <200412281526.48335.peter@wemm.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 28 Dec 2004, Peter Wemm wrote:
> On Tuesday 28 December 2004 02:39 pm, Archie Cobbs wrote:
> > Peter Wemm wrote:
> > >>Attached is a slightly different patch. This is against the source
> > >>currently in ports, and it fixes two additional questionable uses
> > >> of va_start ().
> > >
> > > Just as a BTW; powerpc platforms need this series of changes too,
> > > not just amd64.
> >
> > Thanks.. this change should work for all.
> >
> > > BTW2: I wonder if some of the attached changes really could be
> > > done more simply with va_copy()... Not that it matters of course.
> >
> > Probably.. that's new though? No va_copy() on 4.x.
>
> Yes, its formerly part of C99, but gcc has had it for a while. In
> gcc-2.95 it is implemented in the gnu version of stdarg.h as
> __va_copy(), but we neglected to add it to our includes in 4.x:
Probably not. va_copy() is only needed for restarting at a place other
than the start, and possibly for acting on multiple active copies of
the args (is this required to work?), but the patch seemed to only
need restarting at the start. va_start() is ideal for restarting at
the start; using va_copy() for this is just illogical, gratuitously
unportable to systems that don't implement this part of C99 (e.g.,
RELENG_4), and takes slightly more code (for declaring the variable
that holds the copy):
dump/optr.c still gives an example of how not to use va_copy()
(rev.1.27). Fix:
% Index: optr.c
% ===================================================================
% RCS file: /home/ncvs/src/sbin/dump/optr.c,v
% retrieving revision 1.30
% diff -u -2 -r1.30 optr.c
% --- optr.c 19 Jun 2004 22:41:18 -0000 1.30
% +++ optr.c 20 Jun 2004 07:05:14 -0000
% @@ -234,5 +234,4 @@
% {
% va_list ap;
% - va_list ap2;
%
% (void) fprintf(stderr," DUMP: ");
The patch removes the extra declaration which is the only extra code
(by line count) needed to use va_copy. (But having an extra line of code
is another style bug -- in KNF, local declarations of the same type should
be on the same line if possible.)
% @@ -241,11 +240,11 @@
% #endif
% va_start(ap, fmt);
% - va_copy(ap2, ap);
The patch removes the va_copy() which gives 2 active copies throughout
the function. We only need 1 active copy at a time.
% (void) vfprintf(stderr, fmt, ap);
% + va_end(ap);
va_end() as soon as we have finished with a pass over the args is more
natural than at the end of the function, and is needed to reuse the ap
variable.
% (void) fflush(stdout);
% (void) fflush(stderr);
% - (void) vsnprintf(lastmsg, sizeof(lastmsg), fmt, ap2);
% + va_start(ap, fmt);
% + (void) vsnprintf(lastmsg, sizeof(lastmsg), fmt, ap);
To make another pass over the args restarting at the start, just start
with va_start().
% va_end(ap);
% - va_end(ap2);
Using va_copy() doesn't even simplify cleaning up, since a va_end() is
needed for each va_copy() just like it is needed for each va_start().
% }
%
>
> default implementation:
> gcc/ginclude/stdarg.h:#define __va_copy(dest, src) (dest) = (src)
> gcc/ginclude/varargs.h:#define __va_copy(dest, src) (dest) = (src)
>
> platform override for ppc:
> gcc/ginclude/va-ppc.h:#define __va_copy(dest, src) *(dest) = *(src)
>
> The same variation for ppc there would work for amd64 as well. I have
> run into quite a number of ports that have this knowledge built in so
> that they can compile on ppc with older gcc's without the includes.
Apparently va_copy() wasn't much missed, since it wasn't in C90. Did
the ports really need va_copy() or just not understand va_start()?
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20041229221042.X92684>