Avoid the copying of va_list in prprf.c

RESOLVED FIXED in 4.6

Status

RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: wtc, Assigned: wtc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
Created attachment 144881 [details] [diff] [review]
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)).
(Assignee)

Updated

15 years ago
Attachment #144881 - Flags: superreview?(darin)
Attachment #144881 - Flags: review?(jgmyers)

Comment 2

15 years ago
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?
Attachment #144881 - Flags: superreview?(darin) → superreview+

Comment 3

15 years ago
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.
Attachment #144881 - Flags: review?(jgmyers) → review+

Comment 4

15 years ago
... there is also a typo "poiont" in a comment.
(Assignee)

Comment 5

15 years ago
Created attachment 145352 [details] [diff] [review]
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.
(Assignee)

Comment 6

15 years ago
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.
Attachment #145352 - Flags: review?(jgmyers)

Comment 7

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

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
Attachment #145352 - Flags: review?(jgmyers) → review+
(Assignee)

Comment 8

15 years ago
I checked in patch, patch #2, and the new Mode line
on the NSPR trunk (NSPR 4.6).
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.6
(Assignee)

Comment 9

15 years ago
I checked in the changes on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla 1.8 alpha).
You need to log in before you can comment on or make changes to this bug.