Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
On Thu, Sep 6, 2018 at 3:20 PM Emil Velikov wrote: > Hi Andrii, > > I think you might be overthinking my comments. > There's nothing special, nor am I trying to force my opinion. > > If you think my suggestions make sense, great. Otherwise don't bother too > much. > Thanks a lot for your advices and reviewing) > > HTH > Emil > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
Hi Andrii, I think you might be overthinking my comments. There's nothing special, nor am I trying to force my opinion. If you think my suggestions make sense, great. Otherwise don't bother too much. HTH Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
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
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
Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
On 5 September 2018 at 13:03, 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
Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
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 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
Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
On 4 September 2018 at 10:04, andrey simiklit 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
Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
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.
Regards,
Andrii.
On Mon, Sep 3, 2018 at 6:27 PM Emil Velikov
wrote:
> On 3 September 2018 at 15:49, wrote:
> > From: Andrii Simiklit
> >
> > MSDN:
> > "va_end must be called on each argument list that's initialized
> > with va_start or va_copy before the function returns."
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107810
> > Signed-off-by: Andrii Simiklit
> >
> > ---
> > src/util/u_string.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/src/util/u_string.h b/src/util/u_string.h
> > index ce45430..883fa64 100644
> > --- a/src/util/u_string.h
> > +++ b/src/util/u_string.h
> > @@ -81,6 +81,7 @@ util_vsnprintf(char *str, size_t size, const char
> *format, va_list ap)
> > if (ret < 0) {
> >ret = _vscprintf(format, ap_copy);
> > }
> > + va_end(ap_copy);
> > return ret;
>
> These WIN32 compat functions seem iffy.
> Namely - often we'll issue a va_copy only to use the original va.
>
> 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.
>
> There are also some util_vsnprintf users which use va_copy and some
> who do not :-\
>
> I think those should be looked first, otherwise adding this va_end
> could easily break things.
>
> HTH
> Emil
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
On 3 September 2018 at 15:49, wrote:
> From: Andrii Simiklit
>
> MSDN:
> "va_end must be called on each argument list that's initialized
> with va_start or va_copy before the function returns."
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107810
> Signed-off-by: Andrii Simiklit
>
> ---
> src/util/u_string.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/src/util/u_string.h b/src/util/u_string.h
> index ce45430..883fa64 100644
> --- a/src/util/u_string.h
> +++ b/src/util/u_string.h
> @@ -81,6 +81,7 @@ util_vsnprintf(char *str, size_t size, const char *format,
> va_list ap)
> if (ret < 0) {
>ret = _vscprintf(format, ap_copy);
> }
> + va_end(ap_copy);
> return ret;
These WIN32 compat functions seem iffy.
Namely - often we'll issue a va_copy only to use the original va.
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.
There are also some util_vsnprintf users which use va_copy and some
who do not :-\
I think those should be looked first, otherwise adding this va_end
could easily break things.
HTH
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()
From: Andrii Simiklit
MSDN:
"va_end must be called on each argument list that's initialized
with va_start or va_copy before the function returns."
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107810
Signed-off-by: Andrii Simiklit
---
src/util/u_string.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/util/u_string.h b/src/util/u_string.h
index ce45430..883fa64 100644
--- a/src/util/u_string.h
+++ b/src/util/u_string.h
@@ -81,6 +81,7 @@ util_vsnprintf(char *str, size_t size, const char *format,
va_list ap)
if (ret < 0) {
ret = _vscprintf(format, ap_copy);
}
+ va_end(ap_copy);
return ret;
}
--
2.7.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
