238842 - Avoid the copying of va_list in prprf.c

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Wan-Teh Chang]

When we added the support for numbered argument list
to prprf.c, we added the VARARGS_ASSIGN macro, which
has become a porting nightmare:

 /*
 ** Note: on some platforms va_list is defined as an array,
 ** and requires array notation.
 */
 #if (defined(LINUX) && defined(__x86_64__))
 #define VARARGS_ASSIGN(foo, bar) __va_copy((foo), (bar))
 #elif (defined(LINUX) && defined(__powerpc__)) || \
     (defined(LINUX) && defined(__s390__)) || \
     (defined(LINUX) && defined(__s390x__)) || \
     defined(WIN16) || defined(QNX) || \
     (defined(__NetBSD__) && defined(__powerpc__) && \
     __NetBSD_Version__ < 105000000)
 #define VARARGS_ASSIGN(foo, bar) foo[0] = bar[0]
 #else
 #define VARARGS_ASSIGN(foo, bar) (foo) = (bar)
 #endif

The VARARGS_ASSIGN macro is actually the new va_copy
macro added in C99.  However, the C Standard says
that va_copy must be matched with a corresponding
va_end in the same function.  It is hard to meet
that requirement.  Furthermore, it is not clear
whether older compilers, which don't support va_copy,
require VARAGS_ASSIGN be matched with a corresponding
va_end in the same function.  We would need to
handle the platform differences in that aspect as
well.

Another problem is the following assignment statement
in dosprintf:

            ap = nas[i-1].ap;

It is not clear whether that is legal portable C.
A possible solution is to declare a local variable:

    va_list *pap = &ap;

which gets initialized to the address of the argument
ap, replace the above assignment statement by:

            pap = &nas[i-1].ap;

and replace all occurrences of ap in the function by
*pap, that is, va_arg(ap, xxx) would become
va_arg(*pap, xxx).  This looks a little strange.

To fix this problems, I propose that we rewrite the
code so that we don't need to copy any va_list.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

The key to understanding this patch is the change to
the NumArgState structure.

Instead of storing a va_list that points to the
position of the current numbered argument in the
variable argument list:

 struct NumArgState{
     int type;
     va_list ap;
 };

I simply store the value of that argument:

 struct NumArg{
     int type;
     union {
	int i;
	unsigned int ui;
	PRInt32 i32;
	PRUint32 ui32;
	PRInt64 ll;
	PRUint64 ull;
	double d;
	const char *s;
	int *ip;
     } u;
 };

Because of this change, I also renamed the structure.

Then, in dosprintf, wherever we had va_arg(ap, xxx)
before, I now test to see whether we are dealing with
numbered arguments (which is true if 'nas' is non-null),
and either use the stored argument value (nap->u.yyy)
or get the argument from the va_list (va_arg(ap, xxx)).

Comment 2

[Darin Fisher]

Comment on attachment 144881 [details] [diff] [review]
Proposed patch

the patch looks correct to me.

my only nit is that the field names in NumArg seem like they are not
consistently named.  why isn't "ll" named "i64" for example?

Comment 3

[John G. Myers]

Comment on attachment 144881 [details] [diff] [review]
Proposed patch

Minor issues introduced:

The TYPE_UINTN case does a va_arg(ap, int) where before it always did a
va_arg(ap, unsigned int).  That case also stores the value through the i member
of the union and then reads it through the ui member.


Issues which exist both before and after the patch:

The Mode: comment at the top of the file does not match the indenting style.

The code interchangably uses int and PRIntn for the TYPE_INTSTR case.

Comment 4

[John G. Myers]

... there is also a typo "poiont" in a comment.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch #2

Darin and John, thank you for the code review.

This patch should be applied on top of the first
patch.	This patch addresses the issues John
pointed out.

Darin, the dosprintf function already has a
union that has a PRInt64 member named 'll'.
This is why I continue to use 'll' for the
PRInt64 member in the union inside NumArg.

Comment 6

[Wan-Teh Chang]

Comment on attachment 145352 [details] [diff] [review]
Proposed patch #2

John, please review this patch.  What is the right
value for the Mode?  I don't use emacs.

Comment 7

[John G. Myers]

Comment on attachment 145352 [details] [diff] [review]
Proposed patch #2

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Comment 8

[Wan-Teh Chang]

I checked in patch, patch #2, and the new Mode line
on the NSPR trunk (NSPR 4.6).

Comment 9

[Wan-Teh Chang]

I checked in the changes on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla 1.8 alpha).