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
