Bug 105662

Created attachment 138244 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

Created attachment 138246 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

- remove obsolate include

Comment on attachment 138244 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

Review of attachment 138244 [details] [review]:
-----------------------------------------------------------------

::: .travis.yml
@@ +39,5 @@
> +  - ci_host=mingw32 ci_variant=debug
> +  - ci_host=mingw32 ci_buildsys=cmake
> +  - ci_host=mingw64
> +  - ci_host=mingw64 ci_variant=debug
> +  - ci_host=mingw64 ci_buildsys=cmake

I'm a bit concerned that adding three more builds is going to make Travis-CI even slower.

Perhaps we could combine some of these builds, for instance

- ci_host=mingw32
- ci_host=mingw64 ci_variant=debug
- ci_host=mingw64 ci_buildsys=cmake
- ci_host=mingw32 ci_buildsys=cmake ci_variant=debug    (maybe)

or something like that? That way we test 32- and 64-bit, we test production and debug builds, and we test CMake and Autotools, but we don't do the full combinatorial explosion.

::: cmake/x86_64-w64-mingw32.cmake
@@ +9,5 @@
> +set(CMAKE_CROSSCOMPILING TRUE)
> +set(WIN32 TRUE)
> +set(MINGW TRUE)
> +
> +include(CMakeForceCompiler)

As with Bug #105636, can this include() be removed now that you're not using the cmake_force_*_compiler macros?

::: tools/ci-build.sh
@@ +94,4 @@
>  make="make -j${ci_parallel} V=1 VERBOSE=1"
>  
>  case "$ci_host" in
> +    (mingw32)

There seems to be a lot of duplication between the 32- and 64-bit builds: a lot of the differences are just because we're cross-compiling and using MSYS libraries, not because of the word size.

I think it would make this script simpler if we used the GNU tuple as the value of ci_host (when not "native"), so ci_host=i686-w64-mingw32 or ci_host=x86_64-w64-mingw32 instead of mingw32 or mingw64? That's already what the documentation in tools/ci-install.sh says it should be - I'm not sure why I used plain "mingw" in the actual implementation.

Then we could use

case "$ci_host" in
    (*-w64-mingw32)
        ...

and

mirror=http://repo.msys2.org/mingw/${ci_host%%-*}

(the %%-* modifier removes the longest possible suffix matching the pattern -*, i.e. everything after the i686 or x86_64)

and later on

wget ${mirror}/mingw-w64-${ci_host%%-*}-${pkg}-any.pkg.tar.xz

You'd still need to use

if [ "${ci_host%%-*}" = i686 ]; then
    mingw="$(pwd)/mingw32"
else
    mingw="$(pwd)/mingw64"
fi

to set ${mingw}, but I think that's the only place where we want that form?

@@ +225,3 @@
>                  set _ "$@"
>                  set "$@" --build="$(build-aux/config.guess)"
>                  set "$@" --host=i686-w64-mingw32

... and then we could use --host="${ci_host}" here to cover both ways, which seems nicer.

@@ +305,5 @@
>                  ci_test=no
>                  ;;
> +            (mingw64)
> +                set _ "$@"
> +                set "$@" -D CMAKE_TOOLCHAIN_FILE="${srcdir}/cmake/x86_64-w64-mingw32.cmake"

Similarly we could use "${srcdir}/cmake/${ci_host}.cmake" here.

::: tools/ci-install.sh
@@ +87,5 @@
> +                $sudo dpkg --add-architecture i386
> +                ;;
> +            (mingw64)
> +                $sudo dpkg --add-architecture amd64
> +                ;;

Travis-CI machines are amd64 anyway, so we only need to add the i386 architecture to get wine:i386. We should be able to install wine:amd64 already.

@@ +105,5 @@
> +                $sudo apt-get -qq -y install \
> +                    binutils-mingw-w64-x86-64\
> +                    g++-mingw-w64-x86-64 \
> +                    wine:amd64 \
> +                    ${NULL}

This is one of the few places where you'd still need separate cases for i686-w64-mingw32 and x86_64-w64-mingw32 (and unfortunately you can't even use ${ci_host%%-*} here because it's x86-64 not x86_64 in the package name).

Created attachment 138251 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

- renamed mingwxx to *-w64-mingw
- reduced number of builds

(In reply to Ralf Habacker from comment #4)
> Created attachment 138251 [details] [review] [review]
> travis-ci: Add cross building support for mingw 64 bit compiler
> 
> - renamed mingwxx to *-w64-mingw
> - reduced number of builds

ci_host=x86_64-w64-mingw32 ci_variant=debug returns

../../dbus/dbus-transport-socket.c: In function ‘socket_handle_watch’:
../../dbus/dbus-transport-socket.c:1028:9: error: format ‘%Iu’ expects argument of type ‘size_t’, but argument 6 has type ‘int’ [-Werror=format=]
         _dbus_verbose ("asked to handle watch %p on fd %" DBUS_SOCKET_FORMAT " that we don't recognize\n",
         ^

(In reply to Ralf Habacker from comment #5)
> ci_host=x86_64-w64-mingw32 ci_variant=debug returns
> 
> ../../dbus/dbus-transport-socket.c: In function ‘socket_handle_watch’:
> ../../dbus/dbus-transport-socket.c:1028:9: error: format ‘%Iu’ expects
> argument of type ‘size_t’, but argument 6 has type ‘int’ [-Werror=format=]

The CI has found a pre-existing bug here. Presumably nobody previously enabled verbose mode for 64-bit Windows.

        _dbus_verbose ("asked to handle watch %p on fd %" DBUS_SOCKET_FORMAT " that we don't recognize\n",
                       watch, dbus_watch_get_socket (watch));

dbus_watch_get_socket() returns an int, but this _dbus_verbose() call should really be using _dbus_watch_get_socket() to get the DBusSocket instead.

Created attachment 138260 [details] [review]
Mingw 64bit compile fix

Bug:

(In reply to Ralf Habacker from comment #7)
> Created attachment 138260 [details] [review] [review]
> Mingw 64bit compile fix

with this patch all variants are building see https://travis-ci.org/rhabacker/dbus/builds/356548791

Comment on attachment 138260 [details] [review]
Mingw 64bit compile fix

Review of attachment 138260 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-transport-socket.c
@@ +1026,4 @@
>                         flags);
>        else
>          _dbus_verbose ("asked to handle watch %p on fd %" DBUS_SOCKET_FORMAT " that we don't recognize\n",
> +                       watch, _dbus_socket_printable (_dbus_watch_get_socket (watch)));

This part should be a separate commit, something like:

dbus-transport-socket: Correctly print DBusSocket with DBUS_SOCKET_FORMAT

Previously, on 64-bit Windows we were passing a 32-bit int where the format string expects a 64-bit SOCKET.

Comment on attachment 138260 [details] [review]
Mingw 64bit compile fix

Review of attachment 138260 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-win.c
@@ +2662,4 @@
>              DPRINTF ("%3d %s", i++, pSymbol->Name);
>          }
>        else
> +        DPRINTF ("%3d 0x%" OFFSET_FORMAT, i++, sf.AddrPC.Offset);

If AddrPC.Offset is the same size as a pointer, I think you can use "%3d 0x%Ix" (with a capital i) on all Windows platforms? https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx says the I size modifier is correct for ptrdiff_t and size_t.

Or if "%Ix" isn't suitable for some reason, adding OFFSET_FORMAT looks OK. The commit message should be more like this:

"""
sysdeps-win: Print word-size-dependent offset correctly

AddrPC.Offset is the same size as a pointer, but previously we printed it as though it was the same size as a long, which is 32 bits on 64-bit Windows.
"""

(Ideally, change that suggested commit message to mention what the type of AddrPC.Offset is - I'd guess size_t, ptrdiff_t or [u]intptr_t.)

Comment on attachment 138251 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

Review of attachment 138251 [details] [review]:
-----------------------------------------------------------------

Looks good apart from the missing ";;"

::: tools/ci-install.sh
@@ +86,5 @@
> +            (i686-w64-mingw32)
> +                $sudo dpkg --add-architecture i386
> +                ;;
> +            (x86_64-w64-mingw32)
> +                $sudo dpkg --add-architecture amd64

As I said in my previous review, you don't need to add the amd64 architecture, because Travis-CI is an amd64 system already. But if it works, I suppose it's harmless.

@@ +105,5 @@
> +                $sudo apt-get -qq -y install \
> +                    binutils-mingw-w64-x86-64\
> +                    g++-mingw-w64-x86-64 \
> +                    wine:amd64 \
> +                    ${NULL}

This should have a trailing ";;" like the other case

(In reply to Simon McVittie from comment #10)
> Comment on attachment 138260 [details] [review] [review]
> Mingw 64bit compile fix
> 
> Review of attachment 138260 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-sysdeps-win.c
> @@ +2662,4 @@
> >              DPRINTF ("%3d %s", i++, pSymbol->Name);
> >          }
> >        else
> > +        DPRINTF ("%3d 0x%" OFFSET_FORMAT, i++, sf.AddrPC.Offset);
> 
> If AddrPC.Offset is the same size as a pointer, I think you can use "%3d
> 0x%Ix" (with a capital i) on all Windows platforms?
> https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx says the I size
> modifier is correct for ptrdiff_t and size_t.
> 
> Or if "%Ix" isn't suitable for some reason, adding OFFSET_FORMAT looks OK.
> The commit message should be more like this:
> 
> """
> sysdeps-win: Print word-size-dependent offset correctly
> 
> AddrPC.Offset is the same size as a pointer, but previously we printed it as
> though it was the same size as a long, which is 32 bits on 64-bit Windows.
> """
> 
> (Ideally, change that suggested commit message to mention what the type of
> AddrPC.Offset is - I'd guess size_t, ptrdiff_t or [u]intptr_t.)

at least with mingw header it it 

#ifdef __i386__
  sf.AddrPC.Offset    = context.Eip --> DWORD
#elif defined(_M_X64)
  sf.AddrPC.Offset    = context.Rip; -> DWORD64
#elif defined(_M_IA64)
  sf.AddrPC.Offset    = context.StIIP -> ULONGLONG

Created attachment 138283 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

- minor fixes

Created attachment 138284 [details] [review]
dbus-transport-socket: Correctly print DBusSocket with DBUS_SOCKET_FORMAT

Previously, on 64-bit Windows we were passing a 32-bit int where the
format string expects a 64-bit SOCKET.

- separated patch

Created attachment 138285 [details] [review]
sysdeps-win: Print word-size-dependent offset correctly

AddrPC.Offset is the same size as a pointer, but previously
we printed it as though it was the same size as a long,
which is 32 bits on 64-bit Windows.

- separated patch

Comment on attachment 138283 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

Review of attachment 138283 [details] [review]:
-----------------------------------------------------------------

Thanks, go ahead. You might want to apply the compilation fixes first (I'll review them next).

Comment on attachment 138284 [details] [review]
dbus-transport-socket: Correctly print DBusSocket with DBUS_SOCKET_FORMAT

Review of attachment 138284 [details] [review]:
-----------------------------------------------------------------

Go ahead

Comment on attachment 138285 [details] [review]
sysdeps-win: Print word-size-dependent offset correctly

Review of attachment 138285 [details] [review]:
-----------------------------------------------------------------

Yes please

Ralf seems to have applied all these. Fixed in git for 1.13.4