682180 - libffi doesn't allocate executable trampoline buffers in OSX 10.7

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Bobby Holley (:bholley)]

This is causing crashes on my lion dev machine. Patch coming right up.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Allocate executable trampoline buffers in darwin 10 and beyond. v1

Added a patch for this.

I also fixed up the patch management a little bit, so that patches between our libffi and the vanilla libffi can be serially applied rather than as one megapatch. This should make it easier to upstream changes and prune changes that have been upstreamed. They can still be applied as one by simply cat-ing patches-libffi/* and piping the output to patch.

I also found a sneaky change in our libffi tree that wasn't reflected in libffi.patch. I included it in the new serial patch directory.

Flagging dwitte for review.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 556071 [details] [diff] [review]
Allocate executable trampoline buffers in darwin 10 and beyond. v1

Also flagging khuey for review of the build stuff.

Comment 3

[[e-mail]]

Awesome. I'll look at this next week. If I don't, poke me :)

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What is $target on your machine?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> What is $target on your machine?

x86_64-apple-darwin11.1.0

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 556071 [details] [diff] [review]
Allocate executable trampoline buffers in darwin 10 and beyond. v1

I don't pretend to understand why it's 11, but ok.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> I don't pretend to understand why it's 11, but ok.

The darwin kernel has a major version bump every major release of MacOS:

http://en.wikipedia.org/wiki/Darwin_%28operating_system%29#Release_history

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 556071 [details] [diff] [review]
Allocate executable trampoline buffers in darwin 10 and beyond. v1

Dwitte and I talked about this extensively over gchat some months back, so I think khuey's review should be fine here.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Allocate executable trampoline buffers in darwin 10 and beyond. v2

So I got a couple of scattered oranges when pushing this to try.

It turns out that, depending on the builder, it might be of the form -apple-darwin-10, or of the form -apple-darwin-10.x.x.

Attaching a fix - flagging khuey for re-review. As soon as that happens and my try run completes, this whole stack of things can land.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why don't we just make this '*-apple-darwin*' ?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> Why don't we just make this '*-apple-darwin*' ?

There's clearly some reason why libffi doesn't want to just use the alternate malloc path for everything. Maybe performance (perceived, or otherwise?) Either way, I want to alter behavior as little as possible to increase the chances of this getting accepted upstream.

Comment 12

[Bobby Holley (:bholley)]

Though really, I'm fine with either.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok ... I'll r+ *-apple-darwin1*, even though this seems a bit silly.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Allocate executable trampoline buffers in darwin 10 and beyond. v3

Ok, sure. Attaching a patch and carrying over review.

I've pushed this to try here: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e71f6e3a3771 - once that goes green, the ctype stuff can land.

Comment 15

[Bobby Holley (:bholley)]

Pushed to mozilla-inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/def68a106743

This had a green try run, modulo known oranges: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e71f6e3a3771

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/def68a106743

Comment 17

[Landry Breuil (:gaston)]

Attachment: properly regen configure

Let's properly regen configure to include the comment (and also massage the @@ lines so it applies without offsets), see bug 670719 comment 52

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 653179 [details] [diff] [review]
properly regen configure

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

Let's do that in a separate bug

Comment 19

[Landry Breuil (:gaston)]

Comment on attachment 653179 [details] [diff] [review]
properly regen configure

Discard, superseded by bug #783950