676585 - fix JS compilation for Darwin/ARM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Doug Turner (:dougt)]

Attachment: work in progress.

ted: 

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

I am not sure why this change.

Comment 1

[Doug Turner (:dougt)]

Attachment: build on ios complication fixes

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

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]]

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

[(not currently active) Ted Mielczarek]

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

[(not currently active) Ted Mielczarek]

Attachment: 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

[(not currently active) Ted Mielczarek]

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]]

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

[(not currently active) Ted Mielczarek]

(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

[(not currently active) Ted Mielczarek]

http://hg.mozilla.org/projects/build-system/rev/06defc1b4250

Comment 10

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/06defc1b4250

Comment 11

[Nicholas Nethercote [inactive]]

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]

Informal R+ from me.

Comment 13

[Nicholas Nethercote [inactive]]

(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 [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/d66d2e24ef16

Comment 15

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6974e199548d

Comment 16

[(not currently active) Ted Mielczarek]

Thanks, sorry about that!

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6974e199548d