670719 - javascript engine doesnt build on powerpc (cacheflush support missing on this platform)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Landry Breuil (:gaston)]

Since a while (sorry, cant exactly bisect where, maybe last yarr update in https://bugzilla.mozilla.org/show_bug.cgi?id=625600 ?), mozilla-central doesn't build on OpenBSD/powerpc (and maybe other oses..), because jit is now required and no jit exists on ppc. https://bugzilla.mozilla.org/show_bug.cgi?id=638056 is back... Firefox 5 works.

First, i need a patch for js/src/ctypes/libffi/configure, but that one could be pushed upstream first...

Without any configure option or --disable-tracejit --disable-methodjit, the build fails with :

error: #error "The cacheFlush support is missing on this platform."

If i provide an empty cacheFlush() implem for WTF_CPU_PPC, it fails with :

error: #error "The MacroAssembler is not supported on this platform."

probably because ENABLE_JIT=1 is inconditionally added to CPPFLAGS in js/src/Makefile.in, and it's now a requirement. Nanojit has no support for ppc.

And finally, if i make the ENABLE_JIT=1 conditional to ENABLE_TRACEJIT, the build fails at :

/home/landry/src/mozilla-central/build/js/src/jsregexpinlines.h:494:
error: 'codeBlock' was not declared in this scope

because codeBlock is used outside ENABLE_YARR_JIT.

If i patch all that, the build finally painfully succeeds, but make package fails, and the binary produced doesnt work. I've yet to debug it...

Comment 1

[Landry Breuil (:gaston)]

Attachment: recognize powerpc-*-openbsd* as powerpc-*-freebsd* in libffi

Workaround - should be pushed upstream.

Comment 2

[Andreas Gal :gal]

Comment on attachment 545222 [details] [diff] [review]
recognize powerpc-*-openbsd* as powerpc-*-freebsd* in libffi

Stealing. Can you push this yourself?

Comment 3

[Landry Breuil (:gaston)]

Attachment: provide an empty implem for cacheFlush on ppc

Probably wrong patch, but fixes the build. I know upstream doesnt have that..

Comment 4

[Landry Breuil (:gaston)]

Attachment: only add ENABLE_JIT=1 if ENABLE_TRACEJIT is set

Sidenote : this is also needed on sparc64, see attachement of bug https://bugzilla.mozilla.org/show_bug.cgi?id=653551

Comment 5

[Landry Breuil (:gaston)]

Attachment: only use codeBlock within ENABLE_YARR_JIT

Build fix, dunno if it makes sense or it should go into the #if block just before..

Comment 6

[Landry Breuil (:gaston)]

As a sidenote, all those issues are probably present in mozilla-aurora, since i was already having similar issues before the last branch move... and i've yet to check mozilla-beta too.

Things are not in good shape for powerpc :(

Comment 7

[Landry Breuil (:gaston)]

(In reply to comment #2)
> Comment on attachment 545222 [details] [diff] [review] [review]
> recognize powerpc-*-openbsd* as powerpc-*-freebsd* in libffi
> 
> Stealing. Can you push this yourself?

Nope, i only have a l1 account.. and maybe it should go into configure.ac too, no ? Unless we don't plan to regenerate configure with autoconf...

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 545224 [details] [diff] [review]
provide an empty implem for cacheFlush on ppc

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

I'm not convinced this is the right thing to do.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 545225 [details] [diff] [review]
only add ENABLE_JIT=1 if ENABLE_TRACEJIT is set

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

::: js/src/Makefile.in
@@ +1052,5 @@
>  
>  # Needed to "configure" it correctly.  Unfortunately these
>  # flags wind up being applied to all code in js/src, not just
>  # the code in js/src/assembler.
> +ifdef ENABLE_TRACEJIT

This might not be the right test, since afaik, methodjit also uses js/src/assembler.

Note that considering the comment, it might be worth adding the flags to js/src/assembler only. I can help with that if necessary.

Comment 10

[Landry Breuil (:gaston)]

Oh, and fwiw make package fails (when generating cache ?) at :
 
adding: defaults/autoconfig/ (stored 0%)
adding: defaults/autoconfig/platform.js (deflated 5%)
adding: defaults/autoconfig/prefcalls.js (deflated 72%)
adding: greprefs.js (deflated 73%)
uncaught exception: unknown (can't convert to string)
/home/landry/src/mozilla-central/build/browser/installer/precompile_cache.js:73: TypeError: Cc is undefined
gmake[2]: *** [make-package] Error 3   

I can run the js shell, but running firefox immediately returns 0. and running it in gdb is sooooo painful..

Comment 11

[David Mandelin [:dmandelin]]

(In reply to comment #9)
> Comment on attachment 545225 [details] [diff] [review] [review]
> only add ENABLE_JIT=1 if ENABLE_TRACEJIT is set
> 
> Review of attachment 545225 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/Makefile.in
> @@ +1052,5 @@
> >  
> >  # Needed to "configure" it correctly.  Unfortunately these
> >  # flags wind up being applied to all code in js/src, not just
> >  # the code in js/src/assembler.
> > +ifdef ENABLE_TRACEJIT
> 
> This might not be the right test, since afaik, methodjit also uses
> js/src/assembler.

Yes, I think this should go by platform, or by having any of the 3 JITs (trace, method, regex) enabled.

> Note that considering the comment, it might be worth adding the flags to
> js/src/assembler only. I can help with that if necessary.

Comment 12

[David Mandelin [:dmandelin]]

Comment on attachment 545227 [details] [diff] [review]
only use codeBlock within ENABLE_YARR_JIT

Bug 665819 takes care of this. I just landed it to mozilla-inbound, so it should be on m-c by tomorrow.

Comment 13

[Landry Breuil (:gaston)]

(In reply to comment #11)
> (In reply to comment #9)
> > Comment on attachment 545225 [details] [diff] [review] [review] [review]
> > only add ENABLE_JIT=1 if ENABLE_TRACEJIT is set
> > 
> > Review of attachment 545225 [details] [diff] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/Makefile.in
> > @@ +1052,5 @@
> > >  
> > >  # Needed to "configure" it correctly.  Unfortunately these
> > >  # flags wind up being applied to all code in js/src, not just
> > >  # the code in js/src/assembler.
> > > +ifdef ENABLE_TRACEJIT
> > 
> > This might not be the right test, since afaik, methodjit also uses
> > js/src/assembler.
> 
> Yes, I think this should go by platform, or by having any of the 3 JITs
> (trace, method, regex) enabled.

You mean ... smth like this ?

CXXFLAGS += -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1
ifdef ENABLE_TRACEJIT
CXXFLAGS += -DENABLE_JIT=1
else
ifdef ENABLE_METHODJIT
CXXFLAGS += -DENABLE_JIT=1
else
ifeq (,$(filter arm% sparc %86 x86_64,$(TARGET_CPU))) # regex jit ?
CXXFLAGS += -DENABLE_JIT=1
endif
endif
endif

Comment 14

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

Comment on attachment 545224 [details] [diff] [review]
provide an empty implem for cacheFlush on ppc

Actually, I probably can't give an r+ on this without knowing something about PPC -- are you sure that you don't have to do anything to maintain coherence? I always thought x86 was fairly "special" in that way.

Comment 15

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

Comment on attachment 545227 [details] [diff] [review]
only use codeBlock within ENABLE_YARR_JIT

dmandelin said there's an alternate fix for this in the works.

Comment 16

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

Comment on attachment 545225 [details] [diff] [review]
only add ENABLE_JIT=1 if ENABLE_TRACEJIT is set

glandium already provided review comments on this patch.

Comment 17

[Landry Breuil (:gaston)]

Another data point .. mozilla-beta is fine on ppc without those patches (only the libffi one is needed there)
That's probably broken in mozilla-aurora too given that the migration was recent.. at least that narrows the window.

Comment 18

[Landry Breuil (:gaston)]

Hm, spoke too fast - it builds fine without patches in js, but make package fails :
uncaught exception: unknown (can't convert to string)
/home/landry/src/mozilla-beta/browser/installer/precompile_cache.js:73: TypeError: Cc is undefined
gmake[2]: *** [make-package] Error 3

And running ./firefox returns 0 without doing nothing.
$./firefox -no-remote
$echo $?              
0
$./firefox -v         
Mozilla Firefox 6.0
$./firefox -no-remote -P
Segmentation fault (core dumped)

Comment 19

[Landry Breuil (:gaston)]

And fwiw, i'm getting the same issue during make package in mozilla-central rev 86b0d5ce1a6d

  adding: greprefs.js (deflated 73%)
uncaught exception: unknown (can't convert to string)
/var/buildslave/mozilla-central-macppc/build/toolkit/mozapps/installer/precompile_cache.js:73: TypeError: Cc is undefined

That's with patches from bugs 648735, 651444, 669050, 670719 & 676924

Comment 20

[Landry Breuil (:gaston)]

As i finally found out, m-c packages and runs fine on openbsd/macppc with the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=650749, and the attached patches here : the libffi fix, which gal already reviewed (but i cant commit it myself), and the #ifdef ENABLE_TRACEJIT one, for which i'm awaiting feedback on the alternative approach i proposed in comment #13. It finally seems the empty cacheFlush() implem is not needed.

I'd be really happy if all those pieces can get commited before the next m-c->m-a shift, that'll allow me to concentrate on other openbsd patches.

Mandatory screenshot : http://rhaalovely.net/~landry/stuff/ffx-8.0a1-openbsd-macppc.png

Comment 21

[David Mandelin [:dmandelin]]

(In reply to Landry Breuil from comment #13)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > Comment on attachment 545225 [details] [diff] [review]
> > > only add ENABLE_JIT=1 if ENABLE_TRACEJIT is set
> > > 
> > > Review of attachment 545225 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: js/src/Makefile.in
> > > @@ +1052,5 @@
> > > >  
> > > >  # Needed to "configure" it correctly.  Unfortunately these
> > > >  # flags wind up being applied to all code in js/src, not just
> > > >  # the code in js/src/assembler.
> > > > +ifdef ENABLE_TRACEJIT
> > > 
> > > This might not be the right test, since afaik, methodjit also uses
> > > js/src/assembler.
> > 
> > Yes, I think this should go by platform, or by having any of the 3 JITs
> > (trace, method, regex) enabled.
> 
> You mean ... smth like this ?
> 
> CXXFLAGS += -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1
> ifdef ENABLE_TRACEJIT
> CXXFLAGS += -DENABLE_JIT=1
> else
> ifdef ENABLE_METHODJIT
> CXXFLAGS += -DENABLE_JIT=1
> else
> ifeq (,$(filter arm% sparc %86 x86_64,$(TARGET_CPU))) # regex jit ?
> CXXFLAGS += -DENABLE_JIT=1
> endif
> endif
> endif

Looks sensible to me.

Comment 22

[David Mandelin [:dmandelin]]

(In reply to Landry Breuil from comment #20)
> As i finally found out, m-c packages and runs fine on openbsd/macppc with
> the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=650749, and the
> attached patches here : the libffi fix, which gal already reviewed (but i
> cant commit it myself),

For this kind of thing, you can add the checkin-needed keyword. Don't worry about it on this one, I'll just land it.

Comment 23

[Landry Breuil (:gaston)]

Attachment: Only add -DENABLE_JIT=1 when a jit is effectively enabled

I ended up only doing a test on ENABLE_TRACEJIT/ENABLE_METHODJIT, and moved the regex jit part to where the arch test is already done. This is untested, but you get the idea. If needed, i can push it to try-server, and i'll give it a roll on my sparc64/macppc buildslaves.

Comment 24

[Landry Breuil (:gaston)]

Comment on attachment 545224 [details] [diff] [review]
provide an empty implem for cacheFlush on ppc

That patch seems now unneeded, no cacheFlush implem is needed on ppc with the other patches. I'll deobsolete if needed..

Comment 25

[Landry Breuil (:gaston)]

Comment on attachment 545227 [details] [diff] [review]
only use codeBlock within ENABLE_YARR_JIT

Obsolete, fixed in https://bugzilla.mozilla.org/show_bug.cgi?id=665819

Comment 26

[Landry Breuil (:gaston)]

Just to be on the safe side, pushed to try before setting checkin-needed :

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=73d3334653b9

Comment 27

[Landry Breuil (:gaston)]

Thinking a bit more about this... wouldn't it be better/clearer if CXXFLAGS+= -DENABLE_JIT=1 was moved to the existing ifdef ENABLE_TRACEJIT/ENABLE_METHODJIT blocks like it's done for regex jit, instead of doing a specific ifdef block for that ? The end result will be the same... what do you thing about it ?

As for the -DENABLE_ASSEMBLER=1, i still don't understand why it's needed inconditionally.

Comment 28

[Mike Hommey [:glandium]]

Note that s390 is also affected, and probably bug 680642 has the same root as well.

Comment 29

[Landry Breuil (:gaston)]

All green on http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=73d3334653b9

I'll be happy too if it fixes s390 & mips btw :)

Comment 30

[Landry Breuil (:gaston)]

Attachment: Only add -DENABLE_JIT=1 to CXXFLAGS if a jit is enabled

Same patch in the concept, moving the CXXFLAGS addition to existing ifdef blocks for the sake of clarity/simplicity.

Dunno if i can carry the r+...

Comment 31

[Mike Hommey [:glandium]]

(In reply to Landry Breuil from comment #30)
> Created attachment 554619 [details] [diff] [review]
> Only add -DENABLE_JIT=1 to CXXFLAGS if a jit is enabled
> 
> Same patch in the concept, moving the CXXFLAGS addition to existing ifdef
> blocks for the sake of clarity/simplicity.
> 
> Dunno if i can carry the r+...

With this one you're effectively adding -DENABLE_JIT=1 twice for the tier-1 architectures...
I think I prefer the old one, though it could be simpler with something like
ifneq (,$(ENABLE_TRACEJIT)$(ENABLE_METHODJIT))
CXXFLAGS +=  -DENABLE_JIT=1
endif

though that'd still add it twice for other architectures, since there's also the yarr one. BTW, why do you need that one?

Comment 32

[Landry Breuil (:gaston)]

Comment on attachment 553404 [details] [diff] [review]
Only add -DENABLE_JIT=1 when a jit is effectively enabled

Doh, unset obsolete on the previous patch

Comment 33

[Landry Breuil (:gaston)]

Comment on attachment 554619 [details] [diff] [review]
Only add -DENABLE_JIT=1 to CXXFLAGS if a jit is enabled

and remove review request

Comment 34

[Landry Breuil (:gaston)]

(In reply to Mike Hommey [:glandium] from comment #31)
> (In reply to Landry Breuil from comment #30)
> > Created attachment 554619 [details] [diff] [review]
> > Only add -DENABLE_JIT=1 to CXXFLAGS if a jit is enabled
> > 
> > Same patch in the concept, moving the CXXFLAGS addition to existing ifdef
> > blocks for the sake of clarity/simplicity.
> > 
> > Dunno if i can carry the r+...
> 
> With this one you're effectively adding -DENABLE_JIT=1 twice for the tier-1
> architectures...

Indeed, that should be avoided

> I think I prefer the old one, though it could be simpler with something like
> ifneq (,$(ENABLE_TRACEJIT)$(ENABLE_METHODJIT))
> CXXFLAGS +=  -DENABLE_JIT=1
> endif
> 
> though that'd still add it twice for other architectures, since there's also
> the yarr one. BTW, why do you need that one?

I have no idea how they are intermixed, i've just done it because i was told to :)

Setting checkin-needed for the previous r+'ed patch..

Comment 35

[Mike Hommey [:glandium]]

(In reply to Landry Breuil from comment #34)
> I have no idea how they are intermixed, i've just done it because i was told
> to :)

I would understand if it was set in the else part of the if/else, but at that position, it doesn't make much sense to me. You're effectively adding ENABLE_JIT=1 to !arm, !sparc, !x86.

Comment 36

[Mike Hommey [:glandium]]

Removing checkin-needed until things are sorted out (comment 35)

Comment 37

[Mike Hommey [:glandium]]

I confirm that the attachment 553404 [details] [diff] [review] doesn't solve the problem. (on Linux PPC)

Comment 38

[Mike Hommey [:glandium]]

Attachment: Only add -DENABLE_JIT=1 to CXXFLAGS if any of trace/method/yarr jit is enabled.

Triggerring on the right end of the if/else works better :)

Comment 39

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5847fed34267

Comment 40

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/5847fed34267

Comment 41

[Landry Breuil (:gaston)]

Attachment: properly land as a separate patch + configure.ac fix

Never too late to properly fix that... cf bug 778414 comment 39

Comment 42

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 649781 [details] [diff] [review]
properly land as a separate patch + configure.ac fix

Not sure I can review this (I think khuey is you best bet). Can you also regenerate configure? When I do I see lots of dummy changes like:

-#line 10756 "configure"
+#line 10757 "configure"

Comment 43

[Landry Breuil (:gaston)]

Attachment: properly land as a separate patch + configure.ac fix + regen configure

Sure, here's the version after autoconf-2.65 configure.ac vomit..

Comment 44

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 649790 [details] [diff] [review]
properly land as a separate patch + configure.ac fix + regen configure

I think you forgot to update js/src/ctypes/patches-libffi/05-bug-670719. BTW, I wonder if the best thing to is to just not include configure in it as it is a generated file.

Comment 45

[Landry Breuil (:gaston)]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #44)
> Comment on attachment 649790 [details] [diff] [review]
> properly land as a separate patch + configure.ac fix + regen configure
> 
> I think you forgot to update js/src/ctypes/patches-libffi/05-bug-670719.
> BTW, I wonder if the best thing to is to just not include configure in it as
> it is a generated file.

Right.. that made me realize patches-libffi/01-bug-670719.patch was already here, but hg log shows it came from bug 682180. Oh my.. note that all other patches touch configure _and_ configure.ac, but patches 00, 02 and 04 didnt seem to have regen'ed configure, so we'd need a more general stance on it.

Comment 46

[Mike Hommey [:glandium]]

Comment on attachment 649790 [details] [diff] [review]
properly land as a separate patch + configure.ac fix + regen configure

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

Please avoid to change the configure executable bit. Also, considering the configure.ac changes, there should be no line changes in configure, so that must come from another patch. Better find what.

Comment 47

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Please avoid to change the configure executable bit. Also, considering the
> configure.ac changes, there should be no line changes in configure, so that
> must come from another patch. Better find what.

I think someone just patched configured instead of running autoconf, which is why you get all the line number changes.

Comment 48

[Landry Breuil (:gaston)]

I'm a bit lost here.. what would be acceptable ? A cset only changing configure/configure.ac powerpc-*-freebsd*) line (without the +x mode change) + adding 05-bug-670719 ? would that work out ? Or should i finally rerun autoconf and keep the line changes in configure ?

Comment 49

[Mike Hommey [:glandium]]

You need to figure out what else is making autoconf change line numbers, considering your patch doesn't add or remove a line. It must be some other patch. The simplest might just be to start afresh (revert all patches and refresh autoconf) and for each patch, apply, run autoconf, and refresh the patch with the associated configure changes. IOW, each patch should contain the configure changes that correspond to their own configure.ac changes.

Comment 50

[Landry Breuil (:gaston)]

Reverting all patches is not that easy :

00-base.patch doesnt unapply,

Patching file js/src/ctypes/libffi/src/arm/sysv.S using Plan A...
Hunk #1 succeeded at 137.
Hunk #2 failed at 225.
Hunk #3 succeeded at 301 (offset 4 lines).
1 out of 3 hunks failed--saving rejects to js/src/ctypes/libffi/src/arm/sysv.S.rej

configure.ac changes 3 lines, configure changes the same but adds
-      fix_srcfile_path='`cygpath -w "$srcfile"`'
+      fix_srcfile_path=''

dunno if that comes from an autoconf run

01 is broken as it only changes configure. This one is my fault, it needs to change configure.ac accordingly instead of adding another 05-patch for it. It shouldnt cause line changes.

02 seems to be the guilty one : it removes a line and adds two lines to configure.ac, but configure only has one line changed

03 doesnt unapply, all the line changes dont match:


Patching file configure using Plan A...
Hunk #1 succeeded at 752 (offset -43 lines).
Hunk #2 failed at 4739.
Hunk #3 failed at 5951.
Hunk #4 failed at 7804.
Hunk #5 failed at 8143.
Hunk #6 failed at 8248.
Hunk #7 failed at 8303.
Hunk #8 failed at 11106.
Hunk #9 failed at 11202.
Hunk #10 succeeded at 12404 with fuzz 2 (offset -2243 lines).
8 out of 10 hunks failed--saving rejects to configure.rej

I think the linechanges from 03 should move to 02.

04 only changes one line in both configure and configure.ac.

Comment 51

[Landry Breuil (:gaston)]

Oh and 03-bug-712594.patch is relative to js/src/ctypes/libffi while all others are relative to m-c root..

Comment 52

[Landry Breuil (:gaston)]

After a bit more reflection ... patch 03 is indeed valid, since it adds the 'sys_symbol_underscore' line to configure, and thus shifts all configure lines. But the configure change in http://hg.mozilla.org/mozilla-central/rev/90a869f23e11 doesnt contain the line changes. So they're only in 03-bug-712594.patch but not in the configure file in hg, and thus reappears when one runs autoconf.

I can provide 3 patches :
- one only containing the line changes from 03-bug-712594.patch
- one fixing 02-bug-682180.patch to also add the comment line to configure
- one fixing 01-bug-670719.patch to also change configure.ac

would that work ?

Comment 53

[Landry Breuil (:gaston)]

Attachment: patch configure.ac, update 01-bug-670719.patch accordingly

Comment 54

[Mike Hommey [:glandium]]

Comment on attachment 653178 [details] [diff] [review]
patch configure.ac, update 01-bug-670719.patch accordingly

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

Let's do that in a separate bug

Comment 55

[Landry Breuil (:gaston)]

Comment on attachment 653178 [details] [diff] [review]
patch configure.ac, update 01-bug-670719.patch accordingly

Discard, superseded by bug 783950