Re: [Qemu-devel] [PATCH for-3.1 v2] build: qga: add macro to force use of native mingw32 assert()

[Michael Roth]

Quoting Daniel P. Berrangé (2018-11-14 10:54:56)
> On Wed, Nov 14, 2018 at 10:44:38AM -0600, Michael Roth wrote:
> > When building qemu-ga for w32 with VSS support, some parts of qemu-ga
> > are not linked against glib, specifically the C++ bits used to
> > create the VSS provider DLL. With 3ebee3b191e, we now define assert()
> > as g_assert() for all mingw32 builds via osdep.h, which results in the
> > following build breakage:
> > 
> >   x86_64-w64-mingw32-g++ -o qga/vss-win32/qga-vss.dll 
> > qga/vss-win32/requester.o qga/vss-win32/provider.o qga/vss-win32/install.o 
> > /home/mdroth/w/qemu4.git/qga/vss-win32/qga-vss.def  -shared 
> > -Wl,--add-stdcall-alias,--enable-stdcall-fixup -lole32 -loleaut32 -lshlwapi 
> > -luuid -static
> >   qga/vss-win32/requester.o: In function `requester_freeze':
> >   /home/mdroth/w/qemu4.git/qga/vss-win32/requester.cpp:284: undefined 
> > reference to `g_assertion_message_expr'
> >   qga/vss-win32/requester.o: In function `requester_thaw':
> >   /home/mdroth/w/qemu4.git/qga/vss-win32/requester.cpp:508: undefined 
> > reference to `g_assertion_message_expr'
> >   /home/mdroth/w/qemu4.git/qga/vss-win32/requester.cpp:509: undefined 
> > reference to `g_assertion_message_expr'
> >   collect2: error: ld returned 1 exit status
> >   make: *** [/home/mdroth/w/qemu4.git/qga/vss-win32/Makefile.objs:10: 
> > qga/vss-win32/qga-vss.dll] Error 1
> >   make: *** Waiting for unfinished jobs....
> > 
> > Fix this by introducing a USE_NATIVE_MINGW32_ASSERT macro that can
> > be defined prior to inclusion of osdep.h for build targets that
> > don't link against glib.
> 
> Why doesn't it link to glib and can that be fixed ?

Not sure, the original commit (b39297ae) takes some steps specifically to
avoid a glib dependency, but I'm not sure what the underlying reasons were.
One downside is that it would require a static mingw glib dependency even
for non-static qemu/qemu-ga builds, but that's not too hard to deal with if
we add the appropriate configure checks/errors for it (especially
compared to satisfying the VSS SDK dependency...)

> 
> Glib is considered a mandatory dependancy on anything in QEMU that
> uses osdep.h, since that header pulls in the glib.h header unconditionally.

Of the 3 source files in qga/vss-win32/:

install.cpp uses it for:

  #include <stdio.h>
  #include "config-host.h"
  #include "qemu-version.h"

requester.cpp uses it for:

  #include <stdio.h>
  #include <assert.h>
  #include "qemu/compiler.h" //for GCC_FMT_ATTR

and provider.cpp doesn't need it.

It's not much, but if linking with static glib isn't too bad that seems
like the simplest option, and might allow for some cleanups later (like
removing the struct Error stubs re-implementations in requester.h).

Thanks for the suggestions.

> 
> > Cc: Richard Henderson <address@hidden>
> > Cc: Philippe Mathieu-Daudé <address@hidden>
> > Signed-off-by: Michael Roth <address@hidden>
> > ---
> > v2:
> >  * define USE_NATIVE_MINGW32_ASSERT via build recipe and avoid moving
> >    #include's before osdep.h (Philippe)
> > ---
> >  include/qemu/osdep.h        | 2 +-
> >  qga/vss-win32/Makefile.objs | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 3bf48bcdec..59364bfeb0 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -129,7 +129,7 @@ extern int daemon(int, int);
> >   * code that is unreachable when features are disabled.
> >   * All supported versions of Glib's g_assert() satisfy this requirement.
> >   */
> > -#ifdef __MINGW32__
> > +#if defined(__MINGW32__) && !defined(USE_NATIVE_MINGW32_ASSERT)
> 
> I don't think we should have these kind of hacks.
> 
> Either make vss-win32 link to glib, or stop it importing
> osdep.h entirely if it really must be built completely
> standalone from normal QEMU dependancies.
> 
> >  #undef assert
> >  #define assert(x)  g_assert(x)
> >  #endif
> > diff --git a/qga/vss-win32/Makefile.objs b/qga/vss-win32/Makefile.objs
> > index 23d08da225..5773bfd868 100644
> > --- a/qga/vss-win32/Makefile.objs
> > +++ b/qga/vss-win32/Makefile.objs
> > @@ -3,7 +3,7 @@
> >  qga-vss-dll-obj-y += requester.o provider.o install.o
> >  
> >  obj-qga-vss-dll-obj-y = $(addprefix $(obj)/, $(qga-vss-dll-obj-y))
> > -$(obj-qga-vss-dll-obj-y): QEMU_CXXFLAGS = $(filter-out -Wstrict-prototypes 
> > -Wmissing-prototypes -Wnested-externs -Wold-style-declaration 
> > -Wold-style-definition -Wredundant-decls -fstack-protector-all 
> > -fstack-protector-strong, $(QEMU_CFLAGS)) -Wno-unknown-pragmas 
> > -Wno-delete-non-virtual-dtor
> > +$(obj-qga-vss-dll-obj-y): QEMU_CXXFLAGS = $(filter-out -Wstrict-prototypes 
> > -Wmissing-prototypes -Wnested-externs -Wold-style-declaration 
> > -Wold-style-definition -Wredundant-decls -fstack-protector-all 
> > -fstack-protector-strong, $(QEMU_CFLAGS)) -Wno-unknown-pragmas 
> > -Wno-delete-non-virtual-dtor -DUSE_NATIVE_MINGW32_ASSERT
> >  
> >  $(obj)/qga-vss.dll: LDFLAGS = -shared 
> > -Wl,--add-stdcall-alias,--enable-stdcall-fixup -lole32 -loleaut32 -lshlwapi 
> > -luuid -static
> >  $(obj)/qga-vss.dll: $(obj-qga-vss-dll-obj-y) $(SRC_PATH)/$(obj)/qga-vss.def
> > -- 
> > 2.17.1
> > 
> > 
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>