Closed Bug 538216 Opened 11 years ago Closed 11 years ago

Port libffi to x86/msvc

Categories

(Core :: js-ctypes, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a3

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

Attachments

(8 files)

+++ 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.
Blocks: 519240
Option 2 sounds better to me, especially given the prior art and the difficulty of dealing with autotools.
Assignee: nobody → dwitte
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.
Attachment #421886 - Flags: review?(benjamin)
Attachment #421886 - Attachment is patch: true
Attachment #421886 - Attachment mime type: application/octet-stream → text/plain
Looking for rs= here.
Attachment #421887 - Flags: review?(benjamin)
This was upstreamed.

Looking for rs= here.
Attachment #421888 - Flags: review?(benjamin)
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.
Attachment #421889 - Flags: review?(benjamin)
Removes libffi_msvc now that we don't need it.

Looking for rs= here.

And that's it!
Attachment #421890 - Flags: review?(benjamin)
Forgot to remove libffi_msvc from about:license.
Attachment #421944 - Flags: review?(benjamin)
Blocks: 528129
Attachment #421886 - Flags: review?(benjamin) → review?(ted.mielczarek)
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=.)
Attachment #421887 - Flags: review?(benjamin) → review?(ted.mielczarek)
Attachment #421888 - Flags: review?(benjamin) → review?(ted.mielczarek)
Attachment #421889 - Flags: review?(benjamin) → review?(ted.mielczarek)
Attachment #421890 - Flags: review?(benjamin) → review?(ted.mielczarek)
Attachment #421944 - Flags: review?(benjamin) → review?(ted.mielczarek)
Comment on attachment 421886 [details] [diff] [review]
part 1 - update libffi to git head

rs=me
Attachment #421886 - Flags: review?(ted.mielczarek) → review+
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
Attachment #421887 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 421888 [details] [diff] [review]
part 3 - remove solaris hack

rs=me
Attachment #421888 - Flags: review?(ted.mielczarek) → review+
Attachment #421889 - Flags: review?(ted.mielczarek) → review+
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 on attachment 421890 [details] [diff] [review]
part 5 - remove libffi_msvc

rs=me
Attachment #421890 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 421944 [details] [diff] [review]
part 6 - remove libffi_msvc license

rs=me
Attachment #421944 - Flags: review?(ted.mielczarek) → review+
(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".
Ok, thanks for the info.
Fixed on mozilla-central, with 'test "$FOO"' changes as requested.

Thanks!
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.)
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.
Blocks: 550646
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)
<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.)
The Vista SDK includes VC8 Express compilers, including ml.exe, although I can't remember whether mozillabuild supports using the Vista SDK compilers.
I can't seem to find ml in my Windows 7 SDK installation...
Attached patch mingw fixSplinter Review
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.
Attachment #430989 - Flags: review?(ted.mielczarek)
With that fix, do ctypes unit tests pass? 'make xpcshell-tests' in js/ctypes/tests.
Yes, they pass (I crosscompile on Linux, so I used hacked runxpcshelltests.py to run tests on Wine).
Awesome. :)

When I land bug 513788 and bug 513778, I could use your help testing the mingw configuration.
(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.
Attachment #430989 - Flags: review?(ted.mielczarek) → review+
Keywords: checkin-needed
The fix rebased against current m-c.
Blocks: 551724
(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...
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.3a3
You need to log in before you can comment on or make changes to this bug.