Last Comment Bug 676585 - fix JS compilation for Darwin/ARM
: fix JS compilation for Darwin/ARM
Status: RESOLVED FIXED
[iOS][fixed-in-bs][fixed-in-nanojit]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM iOS 4
: -- normal (vote)
: mozilla9
Assigned To: Ted Mielczarek [:ted.mielczarek]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-04 10:56 PDT by Doug Turner (:dougt)
Modified: 2011-09-08 10:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
work in progress. (4.23 KB, patch)
2011-08-04 10:56 PDT, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
build on ios complication fixes (4.23 KB, patch)
2011-08-04 10:58 PDT, Doug Turner (:dougt)
marty.rosenberg: review+
Details | Diff | Splinter Review
js arm-darwin fixes (5.23 KB, patch)
2011-08-18 10:53 PDT, Ted Mielczarek [:ted.mielczarek]
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2011-08-04 10:56:54 PDT
Created attachment 550749 [details] [diff] [review]
work in progress.

ted: 

-#include <prtypes.h>
+//#include <prtypes.h>

I am not sure why this change.
Comment 1 Doug Turner (:dougt) 2011-08-04 10:58:33 PDT
Created attachment 550750 [details] [diff] [review]
build on ios complication fixes
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2011-08-08 09:57:09 PDT
Comment on attachment 550750 [details] [diff] [review]
build on ios complication fixes

Passing over to Marty, who has the ARM builds going these days.
Comment 3 Marty Rosenberg [:mjrosenb] 2011-08-09 08:25:15 PDT
Comment on attachment 550750 [details] [diff] [review]
build on ios complication fixes

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

I don't see anything wrong with the patch per se.  It doesn't look like it breaks any existing code, and I can only assume that it builds firefox correctly for ios, so lgtm
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-08-18 08:39:44 PDT
I hit a snag actually trying to build with this patch on an updated tree. I'll put a new patch up soon.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2011-08-18 10:53:56 PDT
Created attachment 554141 [details] [diff] [review]
js arm-darwin fixes

Okay, I had to merge in some bits from some other patches I had laying around. I'll comment specifically on some of the bits here, since they're non-obvious.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2011-08-18 10:56:21 PDT
Comment on attachment 554141 [details] [diff] [review]
js arm-darwin fixes

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

::: js/src/assembler/jit/ExecutableAllocator.h
@@ +374,4 @@
>          _flush_cache(reinterpret_cast<char*>(code), size, BCACHE);
>  #endif
>      }
> +#elif WTF_CPU_ARM && WTF_OS_IOS

We never set WTF_CPU_ARM_THUMB2 (different from upstream webkit), so this code wouldn't get built previously.

::: js/src/assembler/wtf/Platform.h
@@ +329,4 @@
>  /* WTF_CPU_ARMV5_OR_LOWER - ARM instruction set v5 or earlier */
>  /* On ARMv5 and below the natural alignment is required. 
>     And there are some other differences for v5 or earlier. */
> +#if !defined(ARMV5_OR_LOWER) && WTF_CPU_ARM && !(WTF_ARM_ARCH_VERSION >= 6)

This is just wrong, I don't know if it's necessary, but it doesn't match upstream certainly.

@@ +1115,5 @@
>  /* Setting this flag prevents the assembler from using RWX memory; this may improve
>     security but currectly comes at a significant performance cost. */
>  #if WTF_PLATFORM_IOS
> +//XXX: this doesn't currently compile in the spidermonkey build
> +#define ENABLE_ASSEMBLER_WX_EXCLUSIVE 0

The build breaks with this defined, maybe an artifact of our build system, but it's easier to just turn this off for now.

::: js/src/configure.in
@@ -2006,4 @@
>      STRIP="$STRIP -x -S"
>      _PLATFORM_DEFAULT_TOOLKIT='cairo-cocoa'
>      TARGET_NSPR_MDCPUCFG='\"md/_darwin.cfg\"'
> -    LDFLAGS="$LDFLAGS -framework Cocoa -lobjc"

Turns out this is totally unnecessary. Probably just copy/pasted from the top-level configure.
Comment 7 Marty Rosenberg [:mjrosenb] 2011-08-19 14:48:19 PDT
Comment on attachment 554141 [details] [diff] [review]
js arm-darwin fixes


>-#if !defined(ARMV5_OR_LOWER) && WTF_CPU_ARM && WTF_ARM_ARCH_VERSION >= 6
>+#if !defined(ARMV5_OR_LOWER) && WTF_CPU_ARM && !(WTF_ARM_ARCH_VERSION >= 6)
This seems rather bad.

>-#define ENABLE_ASSEMBLER_WX_EXCLUSIVE 1
>+//XXX: this doesn't currently compile in the spidermonkey build
>+#define ENABLE_ASSEMBLER_WX_EXCLUSIVE 0
I assume this is done through a system call we haven't bothered to research/use
Comment 8 Ted Mielczarek [:ted.mielczarek] 2011-08-19 15:28:20 PDT
(In reply to Marty Rosenberg [:Marty] from comment #7)
> Comment on attachment 554141 [details] [diff] [review]
> js arm-darwin fixes
> 
> 
> >-#if !defined(ARMV5_OR_LOWER) && WTF_CPU_ARM && WTF_ARM_ARCH_VERSION >= 6
> >+#if !defined(ARMV5_OR_LOWER) && WTF_CPU_ARM && !(WTF_ARM_ARCH_VERSION >= 6)
> This seems rather bad.

I don't know how that got flipped from upstream. It probably hasn't been hurting anything because we force the use of the standard ARM backend and not the thumb backend.

> >-#define ENABLE_ASSEMBLER_WX_EXCLUSIVE 1
> >+//XXX: this doesn't currently compile in the spidermonkey build
> >+#define ENABLE_ASSEMBLER_WX_EXCLUSIVE 0
> I assume this is done through a system call we haven't bothered to
> research/use

No, I think it's just a build configuration that we haven't tested, so it doesn't quite build properly. We can probably make it work, it just seemed simpler to disable it for the moment.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-08-22 05:18:20 PDT
http://hg.mozilla.org/projects/build-system/rev/06defc1b4250
Comment 10 Mike Hommey [:glandium] 2011-08-25 01:53:47 PDT
http://hg.mozilla.org/mozilla-central/rev/06defc1b4250
Comment 11 Nicholas Nethercote [:njn] 2011-09-07 18:58:23 PDT
Ok, bad news.  Nanojit changes have to be landed on nanojit-central and then merged to mozilla-central.  See https://developer.mozilla.org/en/NanojitMerge for details.  The important thing is that if you don't do that, any Nanojit changes that land on mozilla-central get clobbered when the next Nanojit merge happens.  I just did one, and the changes from this bug got clobbered.

I can reland the Nanojit part of this patch in nanojit-central and do a merge.  Is it urgent?  (Nanojit changes are supposed to get a review from an Adobe person too, but these ones are innocuous enough that we can probably let it slide, though I've CC'd Edwin Smith just in case.)
Comment 12 Edwin Smith 2011-09-07 19:12:52 PDT
Informal R+ from me.
Comment 13 Nicholas Nethercote [:njn] 2011-09-07 19:34:27 PDT
(In reply to Edwin Smith from comment #12)
> Informal R+ from me.

Ok, thanks, I'll land and merge this later today.
Comment 14 Nicholas Nethercote [:njn] 2011-09-07 21:36:34 PDT
http://hg.mozilla.org/projects/nanojit-central/rev/d66d2e24ef16
Comment 15 Nicholas Nethercote [:njn] 2011-09-07 21:39:51 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/6974e199548d
Comment 16 Ted Mielczarek [:ted.mielczarek] 2011-09-08 05:24:08 PDT
Thanks, sorry about that!

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