538216 - Port libffi to x86/msvc

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[[e-mail]]

+++ This bug was initially created as a clone of Bug #528351 +++

js/ctypes/libffi_msvc is our current x86/win32 port, which is old and buggy. Having libffi be able to natively build on this platform would be valuable to us and others, and is a precursor to doing something similar for arm/wince.

There are two issues to solve: the MSVC assembler (masm) uses Intel syntax, distinct from gas AT&T syntax (the translation is trivial); and the libffi build system (autoheader/automake/autoconf/libtool) is, even using recent releases of those tools, incapable of dealing with MSVC properly.

There are two workable approaches, both of which I've got working in a prototype state. Translating the assembly is a given.

i) Hack the libffi build system, specifically the generated files and supporting scripts, to fix the bugs in a way that's compatible with mozillabuild. This won't be upstreamable to libffi; we'd have to upstream to the autotools themselves. But we could maintain these changes as a local patch.

ii) Create wrapper scripts, cc.sh and ld.sh, that behave like gcc/ld but translate commandline arguments for cl, ml, and link. This is the approach taken by Timothy Wall for the jna project (https://jna.dev.java.net/). It requires no significant hacks to the libffi build system, we can customize the wrappers to handle the various build options we need, and we can easily extend them to work with win64 and arm/wince as well. (jna, in fact, uses them for win64.) It has the advantage of keeping all the MSVC-related logic in one place, in a couple of short scripts, and means we can upstream the port without getting bogged down trying to fix the build system.

ted, bsmedberg - how does this sound? Since libffi 3.0.9 was just released, there probably won't be another one for a while. I'll post my patch here and go for upstreaming, and we can carry it as a local patch until release.

Comment 1

[Benjamin Smedberg]

Option 2 sounds better to me, especially given the prior art and the difficulty of dealing with autotools.

Comment 2

[[e-mail]]

Attachment: part 1 - update libffi to git head

I've completed my msvc port, including a bunch of tests (in my patchqueue, but not here - they depend on bug 513788), run tryserver tests and libffi's testsuite on a bunch of platforms, and got it upstreamed. This patch pulls git revision 8d27f68baa365bf883b6053c5f6bc819646d5434, which is libffi 3.0.9 + my patch + a few other minor patches.

Looking for rs= here.

Comment 3

[[e-mail]]

Attachment: part 2 - apply libffi.patch

Looking for rs= here.

Comment 4

[[e-mail]]

Attachment: part 3 - remove solaris hack

This was upstreamed.

Looking for rs= here.

Comment 5

[[e-mail]]

Attachment: part 4 - enable msvc port

This is the meat. Switches over to building libffi on MSVC using the wrapper script, msvcc.sh (also upstreamed). Also tweaks libffi/configure and adds that to libffi.patch.

Looking for actual review here.

Comment 6

[[e-mail]]

Attachment: part 5 - remove libffi_msvc

Removes libffi_msvc now that we don't need it.

Looking for rs= here.

And that's it!

Comment 7

[[e-mail]]

Attachment: part 6 - remove libffi_msvc license

Forgot to remove libffi_msvc from about:license.

Comment 8

[[e-mail]]

Comment on attachment 421886 [details] [diff] [review]
part 1 - update libffi to git head

Switching review over to ted. (See comment 2 - comment 7 for details on each patch; most of these only require rs=.)

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 421886 [details] [diff] [review]
part 1 - update libffi to git head

rs=me

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 421887 [details] [diff] [review]
part 2 - apply libffi.patch

This is our only local patch? Do we have this documented in the source tree?

rs=me

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 421888 [details] [diff] [review]
part 3 - remove solaris hack

rs=me

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 421889 [details] [diff] [review]
part 4 - enable msvc port

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -8564,40 +8564,44 @@ if test "$MOZ_MEMORY"; then
>      export MOZ_MEMORY_LDFLAGS
>    fi
> fi
> AC_OUTPUT_SUBDIRS(js/src)
> ac_configure_args="$_SUBDIR_CONFIG_ARGS"
> 
> # Build jsctypes on the platforms we can.
> if test "$BUILD_CTYPES"; then

You flip between using 'test "$FOO"' and 'test -n "$FOO"'. Please pick one and stick with it. (I don't know what's predominant in configure currently.)

>+  # Run the libffi 'configure' script.
>+  ac_configure_args="--disable-shared --enable-static --disable-raw-api"
>+  if test "$MOZ_DEBUG"; then
>+    ac_configure_args="$ac_configure_args --enable-debug"
>+  fi
>+  if test -n "$DSO_PIC_CFLAGS"; then
>+    ac_configure_args="$ac_configure_args --with-pic"
>+  fi
>+  if test -n "$CROSS_COMPILE"; then
>+    ac_configure_args="$ac_configure_args --build=$build --host=$target"
>+  fi
>+  if test -n "$_MSC_VER"; then
>+    # Use a wrapper script for cl and ml that looks more like gcc.
>+    # autotools can't quite handle an MSVC build environment yet.
>+    ac_configure_args="$ac_configure_args CC=$_topsrcdir/js/ctypes/libffi/msvcc.sh LD=link CPP=\"cl -nologo -EP\""

It's neat that the wrapper got upstreamed, but why doesn't ctypes' configure just pick up and use the wrapper itself? Is that filed upstream? (Or is it because they prefer a mingw build?)

r=me with those changes.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 421890 [details] [diff] [review]
part 5 - remove libffi_msvc

rs=me

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 421944 [details] [diff] [review]
part 6 - remove libffi_msvc license

rs=me

Comment 15

[[e-mail]]

(In reply to comment #10)
> (From update of attachment 421887 [details] [diff] [review])
> This is our only local patch? Do we have this documented in the source tree?

Yup, in js/ctypes/libffi.patch :)

(In reply to comment #12)
> (From update of attachment 421889 [details] [diff] [review])

> It's neat that the wrapper got upstreamed, but why doesn't ctypes' configure
> just pick up and use the wrapper itself? Is that filed upstream? (Or is it
> because they prefer a mingw build?)

Their build system is libtoolized, which makes doing anything to the build system rather difficult. Rather than spend time rolling around in that mess, I just did it this way. ;)

They do also prefer a mingw build. And the wrapper was tested for an MSYS/mozillabuild environment, and probably won't work outside of that - so upstreaming it was more of a "here, this might work for you, good luck" thing than "we advertise that the MSVC build will work with this".

Comment 16

[(not currently active) Ted Mielczarek]

Ok, thanks for the info.

Comment 17

[[e-mail]]

Comment on attachment 421886 [details] [diff] [review]
part 1 - update libffi to git head

http://hg.mozilla.org/mozilla-central/rev/39027a970ec3

Comment 18

[[e-mail]]

Comment on attachment 421887 [details] [diff] [review]
part 2 - apply libffi.patch

http://hg.mozilla.org/mozilla-central/rev/5dc1bb37b2c9

Comment 19

[[e-mail]]

Comment on attachment 421888 [details] [diff] [review]
part 3 - remove solaris hack

http://hg.mozilla.org/mozilla-central/rev/7e7ea9d499da

Comment 20

[[e-mail]]

Comment on attachment 421889 [details] [diff] [review]
part 4 - enable msvc port

http://hg.mozilla.org/mozilla-central/rev/3cae91489a56

Comment 21

[[e-mail]]

Comment on attachment 421890 [details] [diff] [review]
part 5 - remove libffi_msvc

http://hg.mozilla.org/mozilla-central/rev/93eb4a343aa4

Comment 22

[[e-mail]]

Comment on attachment 421944 [details] [diff] [review]
part 6 - remove libffi_msvc license

http://hg.mozilla.org/mozilla-central/rev/567dd07376e4

Comment 23

[[e-mail]]

Fixed on mozilla-central, with 'test "$FOO"' changes as requested.

Thanks!

Comment 24

[Zack Weinberg (:zwol)]

For future reference: test [-n|-z] "$FOO" is safer than test "$FOO" when $FOO might begin with a dash, so defensively coded shell scripts always use that form.   (If you're *really* paranoid you write test x"$FOO" = x, or != x, but IIRC that's not necessary with modern shells.)

Comment 25

[Josh Aas]

I think this broke 32-bit Mac OS X build on 10.6 (cross-compile). This is a big problem for any Mac OS X developers on 10.6. I filed bug 550646.

Comment 26

[Justin Wood (:Callek)]

This seems to have broken my comm-central build with MSVC8-Free.  I don't seem to have ml.exe in my path at all.

The issue is that ml.exe is not provided by the Free versions of VC, nor in any of the SDK's it is only provided by the Pay Visual Studio's.

Is there any way around this problem?

(I am able to compile based on: http://hg.mozilla.org/mozilla-central/rev/5dc1bb37b2c9)

Comment 27

[Rain [:rain1] (inactive)]

<https://connect.microsoft.com/VisualStudio/feedback/details/312421/free-masm-8-0-download-does-not-work-with-visual-studio-express-2008> indicates that MASM is available with VC9 Express SP1. Ted, given that VC9 express is free, how does dropping support for VC8 express altogether sound?

(Using <http://www.microsoft.com/downloads/details.aspx?familyid=7A1C9DA0-0510-44A2-B042-7EF370530C64&displaylang=en> is another possibility, however the EULA prohibits commercial use.)

Comment 28

[[e-mail]]

The Vista SDK includes VC8 Express compilers, including ml.exe, although I can't remember whether mozillabuild supports using the Vista SDK compilers.

Comment 29

[(not currently active) Ted Mielczarek]

It does nowadays:
http://hg.mozilla.org/mozilla-build/file/7ff254547fcd/guess-msvc.bat#l75

Comment 30

[(not currently active) Ted Mielczarek]

Also, Microsoft appears to have MASM for VC8 Express available as a free download:
http://www.microsoft.com/downloads/details.aspx?FamilyID=7a1c9da0-0510-44a2-b042-7ef370530c64&displaylang=en

Comment 31

[Rain [:rain1] (inactive)]

I can't seem to find ml in my Windows 7 SDK installation...

Comment 32

[Jacek Caban]

Attachment: mingw fix

part 4 patch broke mingw compilation. It worked before by accident due to shared config cache. The problem is that $target is passed to libffi configure. It is i686-pc-mingw32 in my case (although I pass i686-mingw32 in mozconfig, but it gets fixed later), but mingw target is special and it should be i686-mingw32 (note lack of -pc). My proposed solution is to check for mingw and pass ${target_cpu}-${target_os} instead of $target in this case. That makes libffi configure script happy.

Comment 33

[[e-mail]]

With that fix, do ctypes unit tests pass? 'make xpcshell-tests' in js/ctypes/tests.

Comment 34

[Jacek Caban]

Yes, they pass (I crosscompile on Linux, so I used hacked runxpcshelltests.py to run tests on Wine).

Comment 35

[[e-mail]]

Awesome. :)

When I land bug 513788 and bug 513778, I could use your help testing the mingw configuration.

Comment 36

[[e-mail]]

(In reply to comment #29)
>(In reply to comment #28)
> > The Vista SDK includes VC8 Express compilers, including ml.exe, although I
> > can't remember whether mozillabuild supports using the Vista SDK compilers.
> It does nowadays:
> http://hg.mozilla.org/mozilla-build/file/7ff254547fcd/guess-msvc.bat#l75
The Vista SDK is 6.0 so I guess that's actually a No.

Comment 37

[(not currently active) Ted Mielczarek]

Sorry, wrong context:
http://hg.mozilla.org/mozilla-build/file/7ff254547fcd/guess-msvc.bat#l173

Comment 38

[Jacek Caban]

Attachment: mingw fix (rebased)

The fix rebased against current m-c.

Comment 39

[[e-mail]]

(In reply to comment #37)
> Sorry, wrong context:
> http://hg.mozilla.org/mozilla-build/file/7ff254547fcd/guess-msvc.bat#l173
Now if only MozillaBuild used that in preference to a VC8EE installation...

Comment 40

[[e-mail]]

Comment on attachment 431842 [details] [diff] [review]
mingw fix (rebased)

http://hg.mozilla.org/mozilla-central/rev/9cd317c204cc