Hello,
Thanks for your reply.
The 'util_vsnprintf' function under Linux is just an alias for 'vsnprintf':
#define util_vsnprintf vsnprintf
So as far as I understood we should ensure compatibility between
'vsnprintf' function under Linux and '_vsnprintf' function under Windows using
our wrapper to be able to use 'util_vsnprintf' in the similar way
on different platforms.
Could you please share your thoughts about it?
So going back to:
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);
The 'ap_copy' and 'ap' both are valid here.
One of them can't affect other one.
So the invalidation of 'ap' should not affect 'ap_copy' and vice versa
MSDN https://msdn.microsoft.com/en-us/library/kb57fad8.aspx :
"va_copy makes a copy of a list of arguments in its current state.
The src parameter must already be initialized with va_start; it may have been
updated with va_arg calls,
but must not have been reset with va_end.
The next argument that's retrieved by va_arg from dest is the same as the next
argument that's retrieved from src."
Please correct me if I am incorrect.
ret = _vsnprintf(str, size, format, ap);
The 'ap_copy' is valid but 'ap' is invalid here.
Could you please correct me if I am incorrect.
task 2: use the copied va
Could you please explain the reason?
if you want to avoid the invalidation of original 'ap' parameter it will work
on windows
but Linux uses the original 'vsnprintf' function which anyway invalidates it.
Do you want to implement some wrapper instead of:
#define util_vsnprintf vsnprintf
for Linux to avoid invalidation of 'ap'?
if (ret < 0) {
ret = _vscprintf(format, ap_copy);
The 'ap_copy' is invalid here
}
+ va_end(ap_copy);
MSDN:
"va_end must be called on each argument list that's initialized
with va_start or va_copy before the function returns."
return ret;
}
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.
I unable to find any problems with 'va_end' which releases local resource.
The behavior of original 'va_list' parameter called 'ap'
will not be changed by the 'va_end' function which is applied to the copy of it.
So as far as I understood for the callers/users of the 'util_vsnprintf' nothing
will be changed.
Could you please share your thoughts about it?
Could you please provide some example how it could cause problems?
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.
Grepping through for "va_copy" and checking the new one is actually used shows:
src/glx/apple/apple_glx_log.c -> missing va_end
I agree. I will provide a patch for this case.
src/intel/tools/imgui/imgui.cpp -> using the orig va, then the copy
It seems correct for me.
'orig va' is used for a first call
'copy' is used for another call
src/util/u_string.h: util_vsnprintf -> using the orig va, then the copy
It seems correct for me.
'orig va' is used for a first call
'copy' is used for another call
Could you please correct me if I am incorrect.
src/util/u_string.h: util_vasprintf -> using the orig va
I agree. 'copy' is created but 'orig va' is used for both calls.
I will provide a patch for this case.
Regards,
Andrii.
On 05.09.18 15:52, Emil Velikov wrote:
On 5 September 2018 at 13:03, asimiklit.w...@gmail.com
<asimiklit.w...@gmail.com> wrote:
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);
task 2: use the copied va
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.
Reasonable assumption, which I would [personally] not dare to make
since we're talking about MSVC and C99 compliance.
Looking at MSDN
Beginning with the UCRT in Visual Studio 2015 and Windows 10,
vsnprintf is no longer identical to _vsnprintf. The vsnprintf function
complies with the C99 standard; _vnsprintf is retained for backward
compatibility with older Visual Studio code.
We have various workarounds for missing va_copy [and in general string
handling] which one gets picked where is fairly magical.
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
Right, I've noticed that one and didn't look if there are more.
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.
Grepping through for "va_copy" and checking the new one is actually used shows:
src/glx/apple/apple_glx_log.c -> missing va_end
src/intel/tools/imgui/imgui.cpp -> using the orig va, then the copy
src/util/u_string.h: util_vsnprintf -> using the orig va, then the copy
src/util/u_string.h: util_vasprintf -> using the orig va
(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.
That's one case, yes. Skimming through src/intel there is a bunch of
va_start + vsnprintf + va_end which could be swapped with
util_s[n]printf.
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
