[libvirt] [PATCH] build: fix virBufferVasprintf on mingw
Gnulib documents that mingw [v]snprintf is broken (it returns -1 on out-of-space, instead of the count of what would have been printed); but while we were using the snprintf wrapper, we had not yet been using the vsnprintf wrapper. * bootstrap.conf (gnulib_modules): Add vsnprintf. Reported by Matthias Bolte. --- bootstrap.conf | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/bootstrap.conf b/bootstrap.conf index 581d60b..302cce8 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -97,6 +97,7 @@ usleep vasprintf verify vc-list-files +vsnprintf waitpid warnings ' -- 1.7.4.4
On 06/30/2011 12:00 PM, Eric Blake wrote:
Gnulib documents that mingw [v]snprintf is broken (it returns -1 on out-of-space, instead of the count of what would have been printed); but while we were using the snprintf wrapper, we had not yet been using the vsnprintf wrapper.
* bootstrap.conf (gnulib_modules): Add vsnprintf. Reported by Matthias Bolte. --- bootstrap.conf | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
Followup. There are two forks of mingw - the older mingw project is 32-bit only, and has added wrappers into their w32api libraries that substitute the broken [v]snprintf of msvcrt with a mingw-specific one that is POSIX-compliant out of the box. Then there is the mingw64 project, which can compile for both 32-bit and 64-bit, and where their w32api libraries do not yet override [v]snprintf. [For reference, Fedora 15 uses the older mingw project, but Fedora 16 is hoping to switch to the newer mingw64 project; meanwhile, cygwin ships with cross-compilers for both flavors, which is how I tested the vsnprintf behavior of both flavors] The gnulib documentation tends to lag various mingw fixes, and (to date) has not been making any distinction between the mingw and mingw64 projects. This patch will help mingw64, so it is worth applying. However, Matthias and I spent some time on IRC and we are quite confused at why his mingw build is having issues - since mingw uses wrappers that work and do not need the gnulib replacement in the first place, at least at configure time, this change to bootstrap.conf did not change anything for his build. I'm wondering if maybe libtools attempts to directly invoke ld instead of going through gcc as the linker are causing problems, where the configure test used gcc and sees the working wrapper vsnprintf, but then virsh is compiled via libtool and ends up using the native broken vsnprintf. At least, that's all I was able to guess :( -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/6/30 Eric Blake <eblake@redhat.com>:
On 06/30/2011 12:00 PM, Eric Blake wrote:
Gnulib documents that mingw [v]snprintf is broken (it returns -1 on out-of-space, instead of the count of what would have been printed); but while we were using the snprintf wrapper, we had not yet been using the vsnprintf wrapper.
* bootstrap.conf (gnulib_modules): Add vsnprintf. Reported by Matthias Bolte. --- bootstrap.conf | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
Followup. There are two forks of mingw - the older mingw project is 32-bit only, and has added wrappers into their w32api libraries that substitute the broken [v]snprintf of msvcrt with a mingw-specific one that is POSIX-compliant out of the box. Then there is the mingw64 project, which can compile for both 32-bit and 64-bit, and where their w32api libraries do not yet override [v]snprintf. [For reference, Fedora 15 uses the older mingw project, but Fedora 16 is hoping to switch to the newer mingw64 project; meanwhile, cygwin ships with cross-compilers for both flavors, which is how I tested the vsnprintf behavior of both flavors]
The gnulib documentation tends to lag various mingw fixes, and (to date) has not been making any distinction between the mingw and mingw64 projects.
This patch will help mingw64, so it is worth applying. However, Matthias and I spent some time on IRC and we are quite confused at why his mingw build is having issues - since mingw uses wrappers that work and do not need the gnulib replacement in the first place, at least at configure time, this change to bootstrap.conf did not change anything for his build. I'm wondering if maybe libtools attempts to directly invoke ld instead of going through gcc as the linker are causing problems, where the configure test used gcc and sees the working wrapper vsnprintf, but then virsh is compiled via libtool and ends up using the native broken vsnprintf. At least, that's all I was able to guess :(
Simple tests show that [v]snprintf works correctly with mingw (I didn't test mingw64) in case of a too small buffer. It's only broken in the context of libvirt. I finally figured out that libintl is the cause for this, as Eric already suggested as a possible cause on IRC. It's not related to libtool at all. libintl.h is included by gnulib's gettext.h, that is included by internal.h, that is included by buf.h, that is included by buf.c. This it how we get it there to break in libvirt, because libintl.h (from http://ftp.gnome.org/pub/gnome/binaries/win32/dependencies/gettext-runtime-d...) contains this section #if 1 #if !(defined snprintf && defined _GL_STDIO_H) /* don't override gnulib */ #undef snprintf #define snprintf libintl_snprintf extern int snprintf (char *, size_t, const char *, ...); #endif #if !(defined vsnprintf && defined _GL_STDIO_H) /* don't override gnulib */ #undef vsnprintf #define vsnprintf libintl_vsnprintf extern int vsnprintf (char *, size_t, const char *, va_list); #endif #endif gnulib's stdio.h is included prior to the inclusion of libintl.h so _GL_STDIO_H is defined, but gnulib detected that it doesn't need to replace [v]snprintf, therefore [v]snprintf isn't defined and both #if's are true and libintl.h replaces [v]snprintf with it's own broken version. I can reproduce the problem in a test program by including said libintl.h. -- Matthias Bolte http://photron.blogspot.com
[adding bug-gnulib, bug-gnu-libiconv] On 07/01/2011 04:50 AM, Matthias Bolte wrote:
2011/6/30 Eric Blake <eblake@redhat.com>:
On 06/30/2011 12:00 PM, Eric Blake wrote:
Gnulib documents that mingw [v]snprintf is broken (it returns -1 on out-of-space, instead of the count of what would have been printed); but while we were using the snprintf wrapper, we had not yet been using the vsnprintf wrapper.
* bootstrap.conf (gnulib_modules): Add vsnprintf. Reported by Matthias Bolte. --- bootstrap.conf | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
Followup. There are two forks of mingw - the older mingw project is 32-bit only, and has added wrappers into their w32api libraries that substitute the broken [v]snprintf of msvcrt with a mingw-specific one that is POSIX-compliant out of the box. Then there is the mingw64 project, which can compile for both 32-bit and 64-bit, and where their w32api libraries do not yet override [v]snprintf. [For reference, Fedora 15 uses the older mingw project, but Fedora 16 is hoping to switch to the newer mingw64 project; meanwhile, cygwin ships with cross-compilers for both flavors, which is how I tested the vsnprintf behavior of both flavors]
The gnulib documentation tends to lag various mingw fixes, and (to date) has not been making any distinction between the mingw and mingw64 projects.
This patch will help mingw64, so it is worth applying. However, Matthias and I spent some time on IRC and we are quite confused at why his mingw build is having issues - since mingw uses wrappers that work and do not need the gnulib replacement in the first place, at least at configure time, this change to bootstrap.conf did not change anything for his build. I'm wondering if maybe libtools attempts to directly invoke ld instead of going through gcc as the linker are causing problems, where the configure test used gcc and sees the working wrapper vsnprintf, but then virsh is compiled via libtool and ends up using the native broken vsnprintf. At least, that's all I was able to guess :(
Simple tests show that [v]snprintf works correctly with mingw (I didn't test mingw64) in case of a too small buffer. It's only broken in the context of libvirt. I finally figured out that libintl is the cause for this, as Eric already suggested as a possible cause on IRC. It's not related to libtool at all.
libintl.h is included by gnulib's gettext.h, that is included by internal.h, that is included by buf.h, that is included by buf.c. This it how we get it there to break in libvirt, because libintl.h (from http://ftp.gnome.org/pub/gnome/binaries/win32/dependencies/gettext-runtime-d...) contains this section
#if 1
#if !(defined snprintf && defined _GL_STDIO_H) /* don't override gnulib */ #undef snprintf #define snprintf libintl_snprintf extern int snprintf (char *, size_t, const char *, ...); #endif #if !(defined vsnprintf && defined _GL_STDIO_H) /* don't override gnulib */ #undef vsnprintf #define vsnprintf libintl_vsnprintf extern int vsnprintf (char *, size_t, const char *, va_list); #endif
#endif
gnulib's stdio.h is included prior to the inclusion of libintl.h so _GL_STDIO_H is defined, but gnulib detected that it doesn't need to replace [v]snprintf, therefore [v]snprintf isn't defined and both #if's are true and libintl.h replaces [v]snprintf with it's own broken version. I can reproduce the problem in a test program by including said libintl.h.
Why is libintl's [v]snprintf broken on mingw? Even if libintl is compiled against an older mingw where there is no mingw snprintf replacement, it seems like libintl should be honoring the correct return values. And what can gnulib do to work around the case where mingw has fixed snprintf, but libintl still has broken snprintf, and thus the gnulib headers did not define snprintf? Should the gnulib <stdio.h> replacement _always_ define snprintf, even if only by: #define snprintf snprintf so that inclusion of the gnulib header prior to the libintl headers forces libintl to leave well enough alone? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/01/2011 07:06 AM, Eric Blake wrote:
Why is libintl's [v]snprintf broken on mingw? Even if libintl is compiled against an older mingw where there is no mingw snprintf replacement, it seems like libintl should be honoring the correct return values.
It is because libintl on mingw is specifically using _vsnprintf (the broken msvcrt version) rather than vsnprintf (the fixed mingw override): http://git.savannah.gnu.org/cgit/gettext.git/tree/gettext-runtime/intl/print... in order to fix the fact that both Microsoft and mingw's override do not understand %1$d, but libintl must support argument reordering.
And what can gnulib do to work around the case where mingw has fixed snprintf, but libintl still has broken snprintf, and thus the gnulib headers did not define snprintf? Should the gnulib <stdio.h> replacement _always_ define snprintf, even if only by:
#define snprintf snprintf
so that inclusion of the gnulib header prior to the libintl headers forces libintl to leave well enough alone?
But now we have a problem - if gnulib did _not_ replace snprintf because it probed the mingw version and found that the return value was correct, then the libintl override violates gnulib's assumptions. If gnulib _does_ replace snprintf, but does not support %1$d, then gnulib violates libintl's assumptions. So it sounds like the LGPLv2+ gnulib modules [v]snprintf need to guarantee %1$d parsing, since libvirt is not in a position to upgrade to the LGPLv3+ gnulib modules [v]snprintf-posix. And since mingw's replacement snprintf does not (currently) support %1$d, then we will be back in the scenario of gnulib always replacing snprintf on mingw, avoiding the fact that libintl_snprintf defers to the broken Microsoft _snprintf. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/01/2011 07:50 AM, Eric Blake wrote:
But now we have a problem - if gnulib did _not_ replace snprintf because it probed the mingw version and found that the return value was correct, then the libintl override violates gnulib's assumptions. If gnulib _does_ replace snprintf, but does not support %1$d, then gnulib violates libintl's assumptions.
One bit of good news - I confirmed (by modifying test-vsnprintf, then testing on mingw64, where the gnulib replacement _does_ kick in) that gnulib's vsnprintf replacement supports %1$d out of the box without any further m4 tests, and without having to drag in the vsnprintf-posix module. So at this point, the patch for mingw is as simple as ensuring that the gnulib snprintf replacement always kicks in. Proposed patch coming up soon. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Gnulib documents that mingw vsnprintf is broken (it returns -1 on out-of-space, instead of the count of what would have been printed); but while we were using the snprintf wrapper, we had not yet been using the vsnprintf wrapper. Meanwhile, mingw (but not mingw64) has a replacement snprintf that fixes return values, but still lacks %1$s support; so in that case, gnulib didn't replace snprintf, but libintl then went ahead and installed a version that supported %1$s but not return values. Gnulib has since been fixed to guarantee that the snprintf module will always guarantee the constraints needed by libintl. * .gnulib: Update to latest, for vsnprintf fix. * bootstrap.conf (gnulib_modules): Add vsnprintf. Reported by Matthias Bolte. --- This patch cannot be applied until commit d1776f5 goes into upstream gnulib (possibly under a different commit id); but you can test it now in order to give a conditional ACK. To guarantee that you have the right commit, do: cd .gnulib git fetch git://repo.or.cz/gnulib/ericb.git vasnprintf git reset --hard d1776f5 cd .. before using 'git am' on this message. * .gnulib 7269b35...d1776f5 (8):
snprintf: guarantee %1$d, for libintl xnanosleep: Rewrite to use new dtotimespec module. timespec-add, timespec-sub: new modules dtotimespec: new module * lib/timespec.h (timespectod): New inline function. * lib/timespec.h (timespec_sign): New inline function. pselect: new module sys_select: don't depend on sys_socket
.gnulib | 2 +- bootstrap.conf | 1 + 2 files changed, 2 insertions(+), 1 deletions(-) diff --git a/.gnulib b/.gnulib index 7269b35..d1776f5 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 7269b35c8d9be1a6f97906b9e29b8c422b92fc31 +Subproject commit d1776f5a9b91ae1dcbcd7cac2adc1efcbfc8ba5a diff --git a/bootstrap.conf b/bootstrap.conf index 581d60b..302cce8 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -97,6 +97,7 @@ usleep vasprintf verify vc-list-files +vsnprintf waitpid warnings ' -- 1.7.4.4
On 07/01/2011 11:52 AM, Eric Blake wrote:
* .gnulib: Update to latest, for vsnprintf fix. * bootstrap.conf (gnulib_modules): Add vsnprintf.
This patch cannot be applied until commit d1776f5 goes into upstream gnulib (possibly under a different commit id); but you can test it now in order to give a conditional ACK. To guarantee that you have the right commit, do:
cd .gnulib git fetch git://repo.or.cz/gnulib/ericb.git vasnprintf git reset --hard d1776f5 cd ..
before using 'git am' on this message.
Also, run 'make check gl_public_submodule_commit=' to get make to ignore the fact that you are using a non-public gnulib commit. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/01/2011 11:52 AM, Eric Blake wrote:
Gnulib documents that mingw vsnprintf is broken (it returns -1 on out-of-space, instead of the count of what would have been printed); but while we were using the snprintf wrapper, we had not yet been using the vsnprintf wrapper.
Oh well, this missed 0.9.3 (a holiday weekend slowed down the gnulib side of the equation). This patch is still worth applying in libvirt, but now that we've missed the release, I don't feel comfortable claiming it as a build-breaker, so I'll wait for an ACK.
Meanwhile, mingw (but not mingw64) has a replacement snprintf that fixes return values, but still lacks %1$s support; so in that case, gnulib didn't replace snprintf, but libintl then went ahead and installed a version that supported %1$s but not return values. Gnulib has since been fixed to guarantee that the snprintf module will always guarantee the constraints needed by libintl.
* .gnulib: Update to latest, for vsnprintf fix. * bootstrap.conf (gnulib_modules): Add vsnprintf. Reported by Matthias Bolte. ---
This patch cannot be applied until commit d1776f5 goes into upstream gnulib (possibly under a different commit id);
The gnulib side is now commit c3153d2c...
+++ b/.gnulib @@ -1 +1 @@ -Subproject commit 7269b35c8d9be1a6f97906b9e29b8c422b92fc31 +Subproject commit d1776f5a9b91ae1dcbcd7cac2adc1efcbfc8ba5a
so any upstream gnulib commit after that point is sufficient. I won't bother to send a v3 of this patch with the new commit id unless anyone asks for it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Matthias Bolte