tags:

views:

83

answers:

3

While making an edit to a class with a long history, I was stymied by a particular habit of the architect of wrapping his va_start -> va_end sequence in a mutex. The changelog for that addition (which was made some 15 years ago, and not revised since) noted that it was because va_start et. all was not reentrant.

I was not aware of any such issues with va_start, as I always thought it was just a macro for some stack-pointer math. Is there something here I'm not aware of? I don't want to change this code if there will be side-effects.

Specifically, the function in question looks a lot like this:

void write(const char *format, ...)
{
    mutex.Lock();
    va_list args;
    va_start(args, format);
    _write(format, args);
    va_end(args);
    mutex.Unlock();
}

This is called from multiple threads.

+2  A: 

Well, the way the variable argument access is implemented in C makes it rather obvious that the va_list objects stores some internal state. That makes it not reentrant, meaning that calling va_start on a va_list object would invalidate the effect of the previous va_start. But even more precisely, C explicitly prohibits invoking va_start again on a va_list object before "closing " the previously invoked va_start session with va_end.

A va_list object is supposed to be used in "non-overlapping" fashion: va_start...va_end. After that you can do another va_start on the same va_list object. But trying to overlap the va_start...va_end sessions on the same va_list object will not work.

P.S. Actually, in theory it is, of course, possible to implement some LIFO-based internal state in any session-based iterator. I.e. it is theoretically possible to allow nested va_start...va_end sessions on the same va_list object (thus making it reentrant in that sense). But C library specification does not provide anything like that.

Note though that in C99 va_list objects are copyable by va_copy. So, if you need to browse the same argument list by several overlapping va_start...va_end sessions, you can always achieve that by creating several independent copies of the original va_list.

P.P.S. Looking at the code sample you provided... There's absolutely no need for any mutexes in this case (as far as the integrity of va_list is concerned). And there's no need for a reentrant va_list object. You code is perfectly fine without any mutexes. It will work fine in multiple-thread environment. Macros from va_... group do not operate on the actual "stack pointer". Instead, they operate on a complete independent va_list object that can be used to iterate over the values stored in the stack. You can think of it as your own, private local stack pointer. Each thread invoking your function will gets its own copy of that va_list iterating over its own stack. There will be no conflict between the threads.

AndreyT
Yes, two calls of va_start on a single va_list would invalidate that list. So, for a static va_list (or global, or a member of the class...), perhaps it would be invalidated, but a future call to the same function would make a new va_list.
Nate
@Nate: Yes, but one doesn't necessarily need a static `va_list` to run into the issue. Issues like this ususally arise whe one operates on the same local `va_list` from within a single call to the same variadic function. There's nothing illegal about that.
AndreyT
+1  A: 

As far as being serially-reentrant (ie., if foo() uses va_start is it safe for foo() to call bar() which also uses va_start), the answer is that's fine - as long as the va_list instance isn't the same. The standard says,

Neither the va_start nor va_copy macro shall be invoked to reinitialize ap without an intervening invocation of the va_end macro for the same ap.

So, you're OK as long as a different va_list (referred to above as ap) is used.

If by reentrant you mean thread-safe (which I assume you are, since mutexes are involved), you'll need to look to the implementation for the specifics. Since the C standard doesn't talk about multi-threading, this issue is really up to the implementation to ensure. I could imagine that it might be difficult to make va_start thread-safe on some oddball or small architectures, but I think if you're working on a modern mainstream platform you're likely to have no problems.

On the more mainstream platforms as long as a different va_list argument is being passed to the va_start macro you should have no problem with multiple threads passing through the 'same' va_start. And since the va_list argument is typically on the stack (and therefore different threads will have different instances) you're generally dealing with different instances of the va_list.

I think that in your example, the mutexes are unnecessary for the varargs use. However, if the write(), it certainly would make sense for a write() call to be serialized so that you don't have multiple write() threads screwing up each other's output.

Michael Burr
About the serialized calling - Yes, a mutex belongs here, I just wanted to put it at a lower level. It was originally at a lower level, but this commit 15 years ago moved it up to where you see it, claiming that the mutex needed to protect the va_list too, which I did not believe!
Nate
A: 

There are some platforms where va_list would have problems with re-entrancy, but on that same platform all local variables have such problems. I'm curious, though: what is your _write function expecting to do? If it's using parameters which are set up before calling write, that in and of itself might cause threading issues unless either (1) any particular instance of the object containing _write will only be used by one thread at a time, or (2) all threads using an object to _write will want the same setup parameters.

supercat
_write will still be protected. I just wanted to move the protection down to that level, and not have to worry about all the va_list pointers.
Nate