On Sun, 23 Feb 2020, Pali Rohár wrote:
On Sunday 23 February 2020 22:37:43 Martin Storsjö wrote:
On Sat, 22 Feb 2020, Pali Rohár wrote:
Also another detail I noticed in your patch:
diff --git a/mingw-w64-crt/stdio/snprintf.c b/mingw-w64-crt/stdio/snprintf.c
index 93300113..0bb5556f 100644
--- a/mingw-w64-crt/stdio/snprintf.c
+++ b/mingw-w64-crt/stdio/snprintf.c
@@ -12,7 +12,27 @@ int __cdecl __ms_snprintf(char* buffer, size_t n, const char
*format, ...)
va_list argptr;
va_start(argptr, format);
+
+ /* _vsnprintf() does not work with zero length buffer
+ * so count number of character by _vscprintf() call */
+ if (n == 0 || !buffer)
+ {
+ retval = _vscprintf(format, argptr);
+ va_end(argptr);
+ return retval;
+ }
+
retval = _vsnprintf (buffer, n, format, argptr);
+
+ /* _vsnprintf() returns negative number if buffer is too small
+ * so count number of character by _vscprintf() call */
+ if (retval < 0)
+ retval = _vscprintf(format, argptr);
Here you're reusing argptr again, after already using it once for _vsnprintf
above. I think the correct solution to that is to make a new va_arg with
va_copy, before calling _vsnprintf.
I was thinking about it prior sending patch. But mingw has already
similar code which does not call va_copy() and also in MS documentation
is similar code without va_copy()
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/vsprintf-vsprintf-l-vswprintf-vswprintf-l-vswprintf-l
And because it worked without va_copy() I decided to not use it.
But if you think that it is really needed, it should be fixed on all
places in mingw-w64 code where va_list is passed to other functions more
times.
Ok, if we consistently skip it in other places, I guess it's ok this way.
On Windows, va_list is a plain pointer, so reusing it (as it is passed by
value to the called functions) should be fine, as long as one keeps in mind
that behaviour isn't portable. (On x86_64 linux, va_list is a struct and is
passed by reference, so there va_copy is essential.)
I have already looked that va_list is much more complicated on x86_64 or
aarch64 linux. On x86_64 it is not struct, but (one member) array of
struct, so even plain assignment of va_list cause compile error and
memcpy() instead needs to be used.
Plus Liu had a good point that va_copy() is available since C99...
But there is __builtin_va_copy() which could be used.
The fact that va_copy only is C99 isn't relevant here as far as I see.
It's not used in the headers, but only as part of the implementation
files, which only are built by mingw-w64-crt's build system. With all
compilers that can compile mingw-w64-crt (non-ancieng cc, or modern
clang), va_copy is available even if you don't specify -std=c99. And even
if it wasn't, one could add -std=c99 to the compilation of that source
file (or all of them).
But in any case, va_copy doesn't seem to be strictly necessary on the
windows ABI, so there's probably no point in continuing splitting hairs...
// Martin
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
