551138 - Allow to build against system libffi

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Mike Hommey [:glandium]]

Everything is in the summary. Patch incoming.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Patch

Comment 2

[Mike Hommey [:glandium]]

Attachment: Patch against 1.9.2

Same as previous one, but for 1.9.2

Comment 3

[[e-mail]]

We shouldn't do this. libffi is a tiny amount of code, and doing this requires linking against a shared library rather than just statically linking it in, which we generally prefer. Security issues aren't a significant concern because libffi churn is very, very low and if there is a fix we have a very effective mechanism to push them out. And we only use this in privileged code anyway; it's certainly not exposed to content, which makes the attack surface much, much smaller.

libffi is something that's very prone to bustage due to the fact that it has a separate implementation for each platform, so reducing the number of configurations to test (whether by us or you) is a very, very good thing.

Comment 4

[Mike Hommey [:glandium]]

While Mozilla may not have any interest in using a shared libffi, other people may (I, for one, do).

Anyways, I don't even understand why libffi is being used, when you have all the assembly you need in xptcinvoke. What you need for ctypes could have been taken from there.

Comment 5

[[e-mail]]

Can you explain your interest?

libffi is required for ctypes, because it provides features xptcinvoke doesn't. Struct support, an implementation that doesn't rely on having a vtable, and support for both cdecl and stdcall, to name three...

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> Can you explain your interest?

We have 26 packages currently building against system libffi in Debian. What do you think would happen if all of these were using their own copy, and a bug was found on one of the non mainstream architectures ?

Comment 7

[Benjamin Smedberg]

Comment on attachment 431325 [details] [diff] [review]
Patch

I'd like dwitte to approve this also: dwitte, are there any configure checks we should perform on the system libffi before accepting it?

Comment 8

[[e-mail]]

(In reply to comment #6)
> We have 26 packages currently building against system libffi in Debian. What do
> you think would happen if all of these were using their own copy, and a bug was
> found on one of the non mainstream architectures ?

I agree with the aggregate principle, but we're not talking about all of them using their own copy. We're talking about Mozilla doing it. That's very different.

If we're talking about principles, it's also up to each package to verify that whatever libffi they're incorporating supplies the features they're interested in. (Of course, hardly anyone does this -- but Mozilla certainly does, if you run our unit tests.)

This is important for us because we're actively developing libffi. Many platforms aren't feature-complete, most especially the non-mainstream ones, and we may be adding cross-platform features soon that libffi currently doesn't have. All of these patches are upstreamed and pulled into mozilla-central from libffi git head. Do you run libffi 3.0.9 (the current release) or pull from git head? If you don't do the latter, things will most certainly break, and continue to break. Do you run our unit tests on each platform? If you do, you can catch this. But if you don't, things might compile fine but will inexplicably not work or most likely corrupt the stack and crash.

Given the downside here, and the fact that we're ahead of the curve on updates and are an active contributor to libffi, are you sure this is a good idea?

Comment 9

[Mike Hommey [:glandium]]

And here you hit exactly something we want to avoid: why would we keep these improvements for Mozilla only ? Having to fix our shared libffi for Mozilla use means all libffi users are ensured to have access to a fixed libffi within Debian.

We're also not inviting Mozilla to do that, only to provide the switch, like it does for libpng, libjpeg, libbz2, libz, libsqlite3, libhunspell, etc.

And yes, we do run the test suite, though this is only a recent improvement, which unveiled massive failures on alpha, arm, mips and sparc in 1.9.1 (we're only keeping up, but the unveiled bugs still exist on trunk).

We haven't run the test suite on 1.9.2 on all architectures, though, but I'm fairly sure any bug that will appear from libffi will be fixed. Debian officially supports 10 linux architectures, and 2 freebsd ones. We also have unofficial support for more linux architectures, and hurd. Most of these architectures are *not* supported by Mozilla.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> And here you hit exactly something we want to avoid: why would we keep these
> improvements for Mozilla only ? Having to fix our shared libffi for Mozilla use
> means all libffi users are ensured to have access to a fixed libffi within
> Debian.
I don't think he's saying that you should.  I think the point is that if we used your system libffi we would be more broken than we are now.  Until that changes, I think it is probably best to wait to enable this flag.

Comment 11

[Mike Hommey [:glandium]]

AFAICS the only changes after 3.0.9 are for architectures we don't support (x86 sun studio, msvc...)

Comment 12

[[e-mail]]

(In reply to comment #9)
> And here you hit exactly something we want to avoid: why would we keep these
> improvements for Mozilla only ? Having to fix our shared libffi for Mozilla use
> means all libffi users are ensured to have access to a fixed libffi within
> Debian.

What Shawn said: you will, of course, get these fixes when you update libffi; but building shared means you tie your Mozilla updates to libffi updates.

> We're also not inviting Mozilla to do that, only to provide the switch, like it
> does for libpng, libjpeg, libbz2, libz, libsqlite3, libhunspell, etc.

Of course, I have no problem with you guys using a --with-system-libffi flag if you want -- you're free to do that -- but if we provide it, all the others vendors will think "Ooh, let's use that!" without necessarily (and likely most definitely not) having read this discussion. Most of those other libraries are also more stable, and do not have such wildly different platform-dependent implementation, as libffi.

Some of those other vendors brand their Mozilla builds as Firefox. I don't want this to move into a discussion of branding, but I do want to point out that we want to provide a reasonable expectation of things working. For instance, --with-system-sqlite has configure checks for the features it needs (such as secure delete). We could do something similar for libffi, but that's kinda prohibitive -- it basically means duplicating a bunch of our unit tests as configure checks -- so we can't exactly go there. We could also require that vendors using the Firefox branding run, and pass, our unit tests. That might also be a good way to solve issues like this. So there are ways forward here for a --with-system-libffi flag, but as things stand right now, I think the downside outweighs the upside.

You might also point out that we could make --enable-official-branding (or whatever it is) mutually exclusive with --with-system-libffi. I won't even go there :)

> And yes, we do run the test suite, though this is only a recent improvement,
> which unveiled massive failures on alpha, arm, mips and sparc in 1.9.1 (we're
> only keeping up, but the unveiled bugs still exist on trunk).

Glad to hear you're catching these. I think that's a pretty big deal, and more vendors should do it. :)

(In reply to comment #11)
> AFAICS the only changes after 3.0.9 are for architectures we don't support (x86
> sun studio, msvc...)

So far, yes. The MSVC port was contributed by me. I have several other fixes in my tree for platforms that you do support, and I know of bugs or missing features that I plan to patch in the future.

Comment 13

[[e-mail]]

I should point out, after chatting with mconnor: a simple way to address my concern here would be to implement a configure version check for libffi. This would require that upstream release new versions of libffi more frequently than they've been doing (presently on the order of yearly), to match up with significant patches or feature improvements to specific arches.

That would be a totally workable option, and would provide a reasonable guarantee of stability to anyone using the system library.

I should also point out that you're welcome to locally patch configure to provide this option, until such time as we can do so ourselves.

Comment 14

[Mike Hommey [:glandium]]

Note that the configure test uses pkg-config, which means it only concerns unix builds, for which 3.0.9 is enough, at least now.

Comment 15

[[e-mail]]

Comment on attachment 431325 [details] [diff] [review]
Patch

>diff -r dcfcfca09b45 configure.in

>+dnl system libffi Support
>+dnl ========================================================
>+MOZ_ARG_ENABLE_BOOL(system-ffi,
>+[  --enable-system-ffi       Use system libffi (located with pkgconfig)],
>+    MOZ_NATIVE_FFI=1 )
>+
>+if test -n "$MOZ_NATIVE_FFI"; then
>+    PKG_CHECK_MODULES(MOZ_FFI, libffi)
>+fi

We need a version check here; it has to be > 3.0.9, but we don't want to require 3.0.10 either (since it's not out yet, and there's no guarantee it'll have the changes we want).

So let's call it 3.0.9-moz.

With that change, the patch looks good.

Comment 16

[Mike Hommey [:glandium]]

(In reply to comment #15)
> We need a version check here; it has to be > 3.0.9, but we don't want to
> require 3.0.10 either (since it's not out yet, and there's no guarantee it'll
> have the changes we want).
> 
> So let's call it 3.0.9-moz.
> 
> With that change, the patch looks good.

Can I check that in, then (after, obviously, adding a version check) ?

Comment 17

[[e-mail]]

(In reply to comment #16)
> Can I check that in, then (after, obviously, adding a version check) ?

If you post a new patch, I can take a look at it.

Comment 18

[Mike Hommey [:glandium]]

Attachment: Patch against m-c

There were a lot of changes in m-c since the first patch. Here is an updated patch against current tip. Note I made the version check for system libffi "> 3.0.9", which means 3.0.9 would not be allowed, but 3.0.9-moz or 3.0.10 would.

Comment 19

[Mike Hommey [:glandium]]

Attachment: Unbitrotted patch

Updated the patch to latest m-c, and removed the changes to config/system-headers, since ffi.h is already there.

Comment 20

[Mike Hommey [:glandium]]

Attachment: Allow to build against system libffi

Updated against current m-c.

Comment 21

[Oleg Romashin (:romaxa)]

Some mobile platforms switching to hardfp arm toolchains, and linking with system ffi (which is already ported to hardfp solve the problem)

Also it would be nice to take maemo6 ffi hardfp fix into mozilla tree

Comment 22

[Oleg Romashin (:romaxa)]

http://sourceware.org/ml/libffi-discuss/2010/msg00153.html
who is taking care about sync mozilla ffi with upstream?

Comment 23

[Sudarsana Nagineni]

(In reply to comment #22)
> http://sourceware.org/ml/libffi-discuss/2010/msg00153.html
> who is taking care about sync mozilla ffi with upstream?

Oleg, look at https://bugzilla.mozilla.org/show_bug.cgi?id=605421

Comment 24

[Oleg Romashin (:romaxa)]

Approval for bug 605421 has been canceled, this bug rejected... can we get an approval for at least one bug in order to get js-ctypes working on maemo6 ?

Comment 25

[Doug Turner (:dougt)]

just mark a patch approval 2.0? and it will be approved (assuming it has been reviewed, there is justification (which you have provided), and it doesn't break anything / has tests).

Comment 26

[Oleg Romashin (:romaxa)]

Attachment: Update to trunk, added toplevel iffi configuration

patch update to m-c, added toplevel mozilla configure option.
not sure about dwitte, probably :mitch or ted is better reviewer here...

Comment 27

[Mike Hommey [:glandium]]

(In reply to comment #26)
> Created attachment 502528 [details] [diff] [review]
> Update to trunk, added toplevel iffi configuration
> 
> patch update to m-c, added toplevel mozilla configure option.
> not sure about dwitte, probably :mitch or ted is better reviewer here...

Why add toplevel configure option ? Unsupported options are ignored, yet still passed to sub-configure scripts from toplevel.

Also, why change MOZ_NATIVE_FFI to MOZ_SYSTEM_FFI ? Most other --enable-system/--with-system options use MOZ_NATIVE_*

Comment 28

[Oleg Romashin (:romaxa)]

Comment on attachment 487146 [details] [diff] [review]
Allow to build against system libffi

ok, will check your comments.
restoring original patch for review now

Comment 29

[Mike Hommey [:glandium]]

Attachment: Allow to build against system libffi

Refreshed for current trunk

Comment 30

[Mike Hommey [:glandium]]

Attachment: Allow to build against system libffi

Let's try someone else than Dan... This version should address Dan's and my concerns appropriately.

Comment 31

[Wesley W. Garland]

Does mainstream libffi have the 3.0.9-moz changes in it now?

If so, does this patch check for them? (i.e. looking for version 3.0.10 or whatever?)  A quick glance suggests that we look for >= 3.0.9 on GNUC and > 3.0.9 elsewhere. Dan, is that sufficient? Are the only 3.0.9-moz changes on MSVC? Is 3.0.10 out yet?

I, for one, would love to see this patch land.   My embedding needs to link libffi for its own use, so I wind up building the Moz version separately *and* having it embedded in libmozjs, which is just totally silly.

Comment 32

[Mike Hommey [:glandium]]

(In reply to comment #31)
> Does mainstream libffi have the 3.0.9-moz changes in it now?
> 
> If so, does this patch check for them? (i.e. looking for version 3.0.10 or
> whatever?)  A quick glance suggests that we look for >= 3.0.9 on GNUC and >
> 3.0.9 elsewhere. Dan, is that sufficient? Are the only 3.0.9-moz changes on
> MSVC?

See comment 11. I don' think there is any way to actually test whether the fixes are in the system library. However, MSVC builds don't use pkg-config, so the version test won't work there. That leaves sun studio...

> Is 3.0.10 out yet?

AFAIK, not yet.

Comment 33

[(not currently active) Ted Mielczarek]

Comment on attachment 544958 [details] [diff] [review]
Allow to build against system libffi

Review of attachment 544958 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +4380,5 @@
>      CFLAGS=$_SAVE_CFLAGS
>  fi
>  
> +dnl system libffi Support
> +dnl ========================================================

Nit: I think you're missing a matching line of == above the comment.

Comment 34

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/aad6deb0dd02

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/aad6deb0dd02