Last Comment Bug 682180 - libffi doesn't allocate executable trampoline buffers in OSX 10.7
: libffi doesn't allocate executable trampoline buffers in OSX 10.7
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks: 682504
  Show dependency treegraph
 
Reported: 2011-08-25 18:48 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-08-28 14:10 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allocate executable trampoline buffers in darwin 10 and beyond. v1 (4.99 KB, patch)
2011-08-26 10:48 PDT, Bobby Holley (:bholley) (busy with Stylo)
khuey: review+
Details | Diff | Splinter Review
Allocate executable trampoline buffers in darwin 10 and beyond. v2 (5.13 KB, patch)
2011-10-06 12:09 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
Allocate executable trampoline buffers in darwin 10 and beyond. v3 (4.47 KB, patch)
2011-10-06 12:45 PDT, Bobby Holley (:bholley) (busy with Stylo)
bobbyholley: review+
Details | Diff | Splinter Review
properly regen configure (2.36 KB, patch)
2012-08-19 06:49 PDT, Landry Breuil (:gaston)
mh+mozilla: review-
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2011-08-25 18:48:04 PDT
This is causing crashes on my lion dev machine. Patch coming right up.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2011-08-26 10:48:10 PDT
Created attachment 556071 [details] [diff] [review]
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) (busy with Stylo) 2011-08-26 10:49:36 PDT
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 dwitte@gmail.com 2011-08-26 13:03:08 PDT
Awesome. I'll look at this next week. If I don't, poke me :)
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-26 18:48:14 PDT
What is $target on your machine?
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2011-08-26 19:42:01 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> What is $target on your machine?

x86_64-apple-darwin11.1.0
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-27 03:09:40 PDT
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) (busy with Stylo) 2011-08-27 09:32:46 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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) (busy with Stylo) 2011-10-05 10:00:24 PDT
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) (busy with Stylo) 2011-10-06 12:09:32 PDT
Created attachment 565304 [details] [diff] [review]
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-06 12:10:36 PDT
Why don't we just make this '*-apple-darwin*' ?
Comment 11 Bobby Holley (:bholley) (busy with Stylo) 2011-10-06 12:12:38 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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) (busy with Stylo) 2011-10-06 12:13:51 PDT
Though really, I'm fine with either.
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-06 12:17:18 PDT
Ok ... I'll r+ *-apple-darwin1*, even though this seems a bit silly.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2011-10-06 12:45:41 PDT
Created attachment 565318 [details] [diff] [review]
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) (busy with Stylo) 2011-10-07 10:54:44 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-09 07:25:04 PDT
https://hg.mozilla.org/mozilla-central/rev/def68a106743
Comment 17 Landry Breuil (:gaston) 2012-08-19 06:49:13 PDT
Created attachment 653179 [details] [diff] [review]
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] 2012-08-19 23:18:11 PDT
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) 2012-08-28 14:10:39 PDT
Comment on attachment 653179 [details] [diff] [review]
properly regen configure

Discard, superseded by bug #783950

Note You need to log in before you can comment on or make changes to this bug.