Hello,
Thanks for your reply.
I think that we just misunderstand each other)
static inline int
util_vsnprintf(char *str, size_t size, const char *format, va_list ap)
{
/* We need to use _vscprintf to calculate the length as vsnprintf returns -1
* if the number of characters to write is greater than count.
*/
va_list ap_copy;
int ret;
va_copy(ap_copy, ap);
ret = _vsnprintf(str, size, format, ap);
if (ret < 0) {
ret = _vscprintf(format, ap_copy);
}
+*va_end(ap_copy);*
return ret;
}
As far as I understand this fix will just release the resource which was
allocated locally.
If you care about an 'va_list' invalidation after call of 'util_vsnprintf' function
it works as was. The same result will be on Linux with 'vsnprintf'.
Please pay attention that the 'va_end' function is called
only for a copy of input argument not for original.
I guess it can not cause problems.
Please correct me If I am incorrect.
1. fix util_vsnprintf users which do not va_copy
I found just one case in the mesa source code where 'va_copy'
should be added. It is in 'util_vasprintf' function.
Actually there already is the copy of va but for some reason it is not used.
https://cgit.freedesktop.org/mesa/mesa/tree/src/util/u_string.h#n121
2. use the copied va when using util_vsnprintf and in the u_string.h
You mean to use 'va_copy' for the each 'util_vsnprintf' function usage
in mesa source code? I do not see reasons for that.
(optional) 4. replace opencoded instances of util_vsnprintf
Do you mean find all places where 'vsnprintf' function is used directly
and replace it by our more compatible 'util_vsnprintf'?
Like following example:
https://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/main/imports.c#n256
#ifdef _WIN32 #define vsnprintf _vsnprintf #else ....................
int _mesa_vsnprintf(char *str, size_t size, const char *fmt, va_list
args) { return vsnprintf( str, size, fmt, args); }
I think it is good idea because win32 '_vsnprintf' function works a bit
differently
at least for case when the input buffer size less than the required size:
windows '_vsnprintf' will return -1 for this case.
linux 'vsnprintf' will return the required size.
Normally you'd want separate patches for each piece. Wrt 4. might even
split per-subsystem.
It is good idea.
Regards,
Andrii.
On 04.09.18 23:49, Emil Velikov wrote:
On 4 September 2018 at 10:04, andrey simiklit <asimiklit.w...@gmail.com> wrote:
Hi,
Thanks for your reply.
These WIN32 compat functions seem iffy.
Namely - often we'll issue a va_copy only to use the original va.
As far as I found there is the following reason to use va_copy:
https://linux.die.net/man/3/vsnprintf
"These functions do not call the va_end macro. Because they
invoke the va_arg macro, the value of ap is undefined after the call"
http://www.cplusplus.com/reference/cstdio/vsnprintf/
"nternally, the function retrieves arguments from the list identified by arg
as
if va_arg was used on it, and thus the state of arg is likely to be altered
by the call."
but unfortunately nothing about it on MSDN
so I presume the worst case - it works in the same way.
So the main reason to use va_copy it is when we need pass
our 'va_list' instance to more then one functions because the 'va_list'
instance
can be changed by the called function.
I think that the 'util_vasprintf' on line 116 also should be fixed
because it creates the the copy of 'va_list' but passes
the original 'va_list' instance into both function.
Looking through the rest of the code base:
There are plenty of va_start + util_vsnprintf + va_free cases that
could be replaced with util_snprintf.
I think that I misunderstand you here.
I don't know way to do it. For example:
static inline void
trace_dump_writef(const char *format, ...)
{
static char buf[1024];
unsigned len;
va_list ap;
va_start(ap, format);
len = util_vsnprintf(buf, sizeof(buf), format, ap);
va_end(ap);
trace_dump_write(buf, len);
}
if I try to replace 3 lines above by util_snprintf what should I pass to
parameter '...' ?
Could you please give me an example how to do it?
There are also some util_vsnprintf users which use va_copy and some
who do not :-\
How I explained above it is needed because
we can not pass the same instance of 'va_list' to multiple functions
because 'va_list' instance can be changed by the called function.
I will send new patch as soon as we came agreement about it.
Please correct me if I am incorrect.
You are correct. I'm saying that you're fixing things in the wrong order.
Which could easily cause problems.
1. fix util_vsnprintf users which do not va_copy
2. use the copied va when using util_vsnprintf and in the u_string.h
3. add the missing va_end - the original patch
(optional) 4. replace opencoded instances of util_vsnprintf
Normally you'd want separate patches for each piece. Wrt 4. might even
split per-subsystem.
HTH
Emil
P.S. Please use plan-text + interleaved quoting when working with mailing lists.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
