Last Comment Bug 691898 - Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported, since PCRE doesnt build
: Use YARR interpreter instead of PCRE on platforms where YARR JIT is not suppo...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: PowerPC OpenBSD
: -- normal (vote)
: mozilla15
Assigned To: Landry Breuil (:gaston)
:
Mentors:
Depends on: 661974
Blocks: 684039 684559 729447 751845 761077
  Show dependency treegraph
 
Reported: 2011-10-04 13:39 PDT by Landry Breuil (:gaston)
Modified: 2012-06-04 00:40 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (1.45 KB, patch)
2011-12-24 00:58 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (9.00 KB, patch)
2012-01-13 05:37 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch) (11.02 KB, patch)
2012-01-19 12:36 PST, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch) (14.71 KB, patch)
2012-01-24 23:51 PST, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch) (16.09 KB, patch)
2012-02-06 13:53 PST, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch) (15.81 KB, patch)
2012-02-13 12:29 PST, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch) (15.81 KB, patch)
2012-02-13 13:15 PST, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch) (15.95 KB, patch)
2012-02-24 13:38 PST, Landry Breuil (:gaston)
dmandelin: review+
Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (16.09 KB, patch)
2012-03-06 00:24 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (16.05 KB, patch)
2012-03-06 08:11 PST, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (11.43 KB, patch)
2012-05-01 02:34 PDT, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (11.40 KB, patch)
2012-05-02 02:10 PDT, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (11.33 KB, patch)
2012-05-02 06:05 PDT, Landry Breuil (:gaston)
dmandelin: review+
Details | Diff | Splinter Review
the patch backported to xulrunner 12, no warranty (9.68 KB, patch)
2012-05-04 03:22 PDT, Dan Horák
no flags Details | Diff | Splinter Review

Description Landry Breuil (:gaston) 2011-10-04 13:39:11 PDT
Since landing of bug #684039, js/src/yarr/pcre fails to build on sparc64 (non-methodjit, non-tracejit) with the following errors :
js/src/yarr/pcre/pcre_exec.cpp:49:21: error: jsarena.h: No such file or directory
js/src/yarr/pcre/pcre_exec.cpp:96: error: ISO C++ forbids declaration of 'JSArenaPool' with no type
js/src/yarr/pcre/pcre_exec.cpp:96: error: expected ';' before '*' token
js/src/yarr/pcre/pcre_exec.cpp:99: error: 'JSArenaPool' has not been declared

full trace in http://buildbot.rhaalovely.net/builders/comm-central-sparc64/builds/158/steps/build/logs/stdio .. i'm surprised this hasnt been catched by try/tbpl. I don't know that code but it seems pcre_exec.cpp needs a bit of rewrite since that commit. I'm open to any suggestions, or cluebats explaining what i've done wrong.
Comment 1 Chris Leary [:cdleary] (not checking bugmail) 2011-10-04 14:53:12 PDT
PCRE isn't used in any of the supported builds (we only compile for "tier 1" platforms). We actually removed it a while back, but it seems to have been re-added in bug 684559.
Comment 2 David Mandelin [:dmandelin] 2011-10-10 17:53:24 PDT
Chris, can you provide any hints for how to replace the JSArenaPool usage? Did it just move to a different file?
Comment 3 Chris Leary [:cdleary] (not checking bugmail) 2011-10-11 09:58:44 PDT
(In reply to David Mandelin from comment #2)
> Did it just move to a different file?

No, it was rewritten as js::LifoAlloc.

The interface changes are straightforward, JS_ARENA_MARK => js::LifoAlloc::mark. For the JS_ARENA_ALLOCATE_CAST there's are ::new_ and ::alloc_ functions on the LifoAlloc.
Comment 4 Cameron Kaiser [:spectre] 2011-10-14 07:43:19 PDT
I think I can generate a patch (unless Landry does it first).
Comment 5 Cameron Kaiser [:spectre] 2011-10-14 07:49:50 PDT
I probably should try to fix bustage from bug 673188 at the same time. The options are temporarily commit a change to js/src/Makefile.in to allow builds to continue using the slower YARR interpreter, in the hopes that lazy regexps will mitigate the hit going from PCRE. The other option is to try to deal with both at the same time. The final option is to do both: commit the bandaid, then try to commit a better fix. Which would be okay with you, Chris/David?
Comment 6 Jorge Diogo 2011-11-11 05:16:56 PST
At jsRegExpExecute(), PCRE is trying to use the JSContext->regExpPool (which no longer exists) in the call to match(). 
Do you see any problem in using JSContext::tempLifoAlloc() and using that LifoAlloc instead?
Comment 7 Cameron Kaiser [:spectre] 2011-11-11 08:57:28 PST
No, that's the easy part. The hard part is bug 673188 because PCRE doesn't really have a syntax checking step.

I need a Mozilla call: fix, or issue an intermediate patch to build with YARR interpreter until a final fix?
Comment 8 Mike Hommey [:glandium] 2011-12-24 00:50:50 PST
I would say build with the YARR interpreter. There are chances PCRE breaks again in the future as the JS engine evolves. (And as a matter of fact, in beta, it fails on jstl.h missing before failing on jarena.h...)
Comment 9 Mike Hommey [:glandium] 2011-12-24 00:58:12 PST
Created attachment 584166 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported
Comment 10 Cameron Kaiser [:spectre] 2011-12-24 09:50:55 PST
I'm good with that. We're using YARR JIT in PowerPC now anyway.
Comment 11 Mike Hommey [:glandium] 2011-12-25 23:34:38 PST
Comment on attachment 584166 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

This patch is incomplete.
Comment 12 Mike Hommey [:glandium] 2012-01-13 05:37:00 PST
Created attachment 588391 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

This is the bug I have for beta in Debian. Unfortunately, it doesn't apply to aurora and m-c.
For me, this seriously raises the question as to whether we (mozilla) should be adding js-only builds with JIT disabled at compile time to avoid this in the future, because this is exhausting, especially at the pace of changes in the JS engine.
Comment 13 Mike Hommey [:glandium] 2012-01-13 05:40:17 PST
(In reply to Mike Hommey [:glandium] from comment #12)
> Created attachment 588391 [details] [diff] [review]
> Use YARR interpreter instead of PCRE on platforms where YARR JIT is not
> supported
> 
> This is the bug

Err. the patch, not the bug.
Comment 14 Landry Breuil (:gaston) 2012-01-15 06:18:45 PST
Just to clarify.. is your patch in debian a build fix for fx 10.0beta on ppc & sparc, or only sparc ? currently 10.0b4 as is fails to build for me on ppc, since it doesn't support yarr jit and  !ENABLE_YARR_JIT is not really maintained. I have a "simple" build fix, but i don't know yet if it allows fx to run fine.
Comment 15 Mike Hommey [:glandium] 2012-01-15 22:57:25 PST
(In reply to Landry Breuil from comment #14)
> Just to clarify.. is your patch in debian a build fix for fx 10.0beta on ppc
> & sparc, or only sparc ?

"on platforms where YARR JIT is not supported"
Comment 16 Landry Breuil (:gaston) 2012-01-16 13:14:00 PST
Comment on attachment 588391 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

I can confirm that this patch allows me to build & start firefox 10.0b4 on OpenBSD/powerpc... now, to see what to do for m-c.
Comment 17 Landry Breuil (:gaston) 2012-01-17 23:50:14 PST
For the record, the build failure on sparc64/ppc/!YARR_JIT is now on m-c :

c++ -o jsanalyze.o -c  -fvisibility=hidden -DOSTYPE=\"OpenBSD5\" -DOSARCH=OpenBSD -DEXPORT_JS_API -DIMPL_MFBT -DJS_HAS_CTYPES -DDLL_PREFIX=\"lib\" -DDLL_SUFFIX=\".so.1.0\" -DNO_NSPR_10_SUPPORT -Ictypes/libffi/include -I.  -I/var/buildslave/mozilla-central-sparc64/build/js/src -I. -I./../../dist/include -I./../../dist/include/nsprpub  -I/var/buildslave/mozilla-central-sparc64/build/obj-sparc64-unknown-openbsd5.1/dist/include/nspr  -I/var/buildslave/mozilla-central-sparc64/build/js/src -I/var/buildslave/mozilla-central-sparc64/build/js/src/assembler -I/var/buildslave/mozilla-central-sparc64/build/js/src/yarr  -fPIC  -fno-rtti -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -pthread -pipe  -DNDEBUG -DTRIMMED -g -O -fomit-frame-pointer -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1   -DMOZILLA_CLIENT -include ./js-confdefs.h -MD -MF .deps/jsanalyze.pp /var/buildslave/mozilla-central-sparc64/build/js/src/jsanalyze.cpp
In file included from /var/buildslave/mozilla-central-sparc64/build/js/src/vm/RegExpObject.h:56,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/jsscriptinlines.h:53,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/vm/Stack-inl.h:50,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/jsinferinlines.h:50,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/jsanalyze.cpp:45:
/var/buildslave/mozilla-central-sparc64/build/js/src/yarr/pcre/pcre.h:49:18: error: jstl.h: No such file or directory
In file included from /var/buildslave/mozilla-central-sparc64/build/js/src/jsscriptinlines.h:53,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/vm/Stack-inl.h:50,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/jsinferinlines.h:50,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/jsanalyze.cpp:45:
/var/buildslave/mozilla-central-sparc64/build/js/src/vm/RegExpObject.h:290:3: error: #error "Syntax checking not implemented for !ENABLE_YARR_JIT"
In file included from /var/buildslave/mozilla-central-sparc64/build/js/src/jsscriptinlines.h:53,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/vm/Stack-inl.h:50,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/jsinferinlines.h:50,
                 from /var/buildslave/mozilla-central-sparc64/build/js/src/jsanalyze.cpp:45:
/var/buildslave/mozilla-central-sparc64/build/js/src/vm/RegExpObject.h: In static member function 'static bool js::detail::RegExpPrivateCode::checkSyntax(JSContext*, js::TokenStream*, JSLinearString*)':
/var/buildslave/mozilla-central-sparc64/build/js/src/vm/RegExpObject.h:292: error: no return statement in function returning non-void
Comment 18 Landry Breuil (:gaston) 2012-01-18 12:46:18 PST
I started updating the patch from comment #13 to hg tip, but so far on sparc64 it fails with :

js/src/jscntxt.cpp: In member function 'void js::ThreadData::sizeOfExcludingThis(size_t (*)(const void*, size_t), size_t*, size_t*, size_t*, size_t*)':
js/src/jscntxt.cpp:163: error: invalid use of incomplete type 'struct JSC::ExecutableAllocator'
js/src/jsprvtd.h:335: error: forward declaration of 'struct JSC::ExecutableAllocator'
js/src/jscntxt.h: In member function 'void JSRuntime::delete_(T*) [with T = JSC::ExecutableAllocator]':
js/src/jscntxt.cpp:125:   instantiated from here
js/src/jscntxt.h:712: error: invalid use of incomplete type 'struct JSC::ExecutableAllocator'
js/src/jsprvtd.h:335: error: forward declaration of 'struct JSC::ExecutableAllocator'
js/src/jscntxt.h: In member function 'T* JSRuntime::new_() [with T = JSC::ExecutableAllocator]':
js/src/jscntxt.cpp:201:   instantiated from here
js/src/jscntxt.h:711: error: invalid application of 'sizeof' to incomplete type 'JSC::ExecutableAllocator' 
js/src/jscntxt.h:711: error: invalid use of incomplete type 'struct JSC::ExecutableAllocator'
js/src/jsprvtd.h:335: error: forward declaration of 'struct JSC::ExecutableAllocator'

Of course, it makes sense to me, since JSC::ExecutableAllocator is defined only if ENABLE_ASSEMBLER is set, and the chunk in js/src/Makefile.in makes it unset if ENABLE_YARR_JIT is not set. I'm now trying with that chunk out on sparc64 and will try various hacks.
if i get something that builds i'll post the patch for actual testing on ppc. I really wonder how m-c builds on ppc right now, what changed recently in that regard, and how TenFourFox does to build....

Another annoyance was the landing of bug #673188, which right now #errors out if ENABLE_YARR_JIT is unset (see RegExpObject.cpp, function checkSyntax()), pretty unfriendly. So far i've replaced it by a return true, but it's of course ugly.
Comment 19 Chris Leary [:cdleary] (not checking bugmail) 2012-01-18 15:10:07 PST
(In reply to Landry Breuil from comment #18)
> Another annoyance was the landing of bug #673188, which right now #errors
> out if ENABLE_YARR_JIT is unset (see RegExpObject.cpp, function
> checkSyntax()), pretty unfriendly. So far i've replaced it by a return true,
> but it's of course ugly.

It's required for lazy regular expression compilation -- you have to detect errors in the syntax immediately for error reporting purposes. Continuing to use the YARR parser to report errors when you're using the PCRE backend could result in a mismatch where the PCRE parser believes there to be an error in the pattern that YARR did not detect, which would result in incorrect semantics (because you have no way of reporting the error when you're doing the lazy compilation).

Sorry, I should have put a comment in if I didn't!
Comment 20 Landry Breuil (:gaston) 2012-01-18 23:37:58 PST
(In reply to Chris Leary [:cdleary] from comment #19)
> (In reply to Landry Breuil from comment #18)
> > Another annoyance was the landing of bug #673188, which right now #errors
> > out if ENABLE_YARR_JIT is unset (see RegExpObject.cpp, function
> > checkSyntax()), pretty unfriendly. So far i've replaced it by a return true,
> > but it's of course ugly.
> 
> It's required for lazy regular expression compilation -- you have to detect
> errors in the syntax immediately for error reporting purposes. Continuing to
> use the YARR parser to report errors when you're using the PCRE backend
> could result in a mismatch where the PCRE parser believes there to be an
> error in the pattern that YARR did not detect, which would result in
> incorrect semantics (because you have no way of reporting the error when
> you're doing the lazy compilation).
> 
> Sorry, I should have put a comment in if I didn't!

The patch we're working on aims to replace PCRE interpreter by YARR interpreter.. if i get it right, we wouldn't use the PCRE parser anymore, so there would be no mismatch ? Of course, i don't see how to do syntax checking with YARR interpreter without YARR JIT, as JSC::Yarr::checkSyntax() isn't defined when !ENABLE_YARR_JIT. Help appreciated...
Comment 21 Landry Breuil (:gaston) 2012-01-18 23:40:44 PST
(In reply to Cameron Kaiser from comment #10)
> I'm good with that. We're using YARR JIT in PowerPC now anyway.

Can you detail how you achieved that (in TenFourFox i suppose), how related is it to nanojit on ppc (#624164), and if the corresponding code is commited to m-c, or candidate to be ? I'm trying to understand all the inter-relations...
Comment 22 Mike Hommey [:glandium] 2012-01-19 00:14:44 PST
Hint: a lot of the !ENABLE_YARR_JIT uses really mean USE_PCRE.
Comment 23 Cameron Kaiser [:spectre] 2012-01-19 06:27:55 PST
We're now using a PPC macroassembler in TenFourFox (in our Google Code, this is issue 96). The reason I haven't pushed this upstream yet is because one of our contributors has an idea about reworking branching for better performance, and we are also implementing a software square root for PPCs that only have reciprocal square root estimates. Once this work is done, I'll put it up for review.

That isn't going to be all you would need to do for OpenBSD/powerpc, however. We also have some integration pieces to connect it to YarrJIT which are specific to the OS X ABI (I'm not going to bother with those since Mozilla doesn't support OS X PPC anymore) and in fact our prologue, epilogue and certain modifications to the methodjit assume OS X ABI and OpenBSD (IIRC) is SysV. Some Linux users who were looking at importing our nanojit are running into the same problem. Feel free to contact me privately if you want more information on this.

The long and short of it is, you should wait for the macroassembler bug to be posted by me (ETA a month or two), and then make whatever changes need to be made to YarrJIT. For now you're stuck with the YARR interpreter, sucky as it is.
Comment 24 Cameron Kaiser [:spectre] 2012-01-19 06:29:41 PST
(If you really want this now, it's in our changesets, btw. See http://code.google.com/p/tenfourfox/wiki/HowToBuildNow and on the ABI issue, http://code.google.com/p/tenfourfox/wiki/WhichABI .
Comment 25 Landry Breuil (:gaston) 2012-01-19 12:36:53 PST
Created attachment 589953 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

Here's the patch updated to m-c, with the following changes wrt previous patch against beta :
- return true instead of #error'ing in syntaxCheck. (no idea what to do for that)
- assembler/jit added to VPATH in js/src/Makefile.in (for !yarr_jit)
- ExecutableAllocator.cpp/ExecutableAllocatorPosix.cpp added to CPPSRCS (for !yarr_jit)

This allows me to build m-c on sparc64. I'm now trying to build on ppc to see if it actually runs, but it seems bug #703534 is back, as of now i'm getting again the #error about missing cacheFlush (which for now i'll define as empty..). Not setting any review, but i'd like feedback...
Comment 26 Mike Hommey [:glandium] 2012-01-19 13:00:22 PST
That's why ENABLE_ASSEMBLER needs to be off.
Comment 27 Cameron Kaiser [:spectre] 2012-01-19 13:28:09 PST
And a drive-by nit: you should also pull the comment out of the Makefile at the same time, since this effectively undoes bug 684559. For that matter, we should probably remove PCRE altogether again since there will be no more consumers after this.
Comment 28 Landry Breuil (:gaston) 2012-01-19 23:23:05 PST
(In reply to Mike Hommey [:glandium] from comment #26)
> That's why ENABLE_ASSEMBLER needs to be off.

But if ENABLE_ASSEMBLER is conditional to ENABLE_YARR_JIT|ENABLE_METHODJIT in Makefile.in, build fails in jscntxt.{cpp,h}/jsprvtd.h due to missing def of struct JSC::ExecutableAllocator (see comment #18). All those uses are inconditional right now, so do you mean they should be made conditional to ENABLE_ASSEMBLER (or ENABLE_YARR_JIT) too ?
Comment 29 Mike Hommey [:glandium] 2012-01-19 23:32:31 PST
Yes, but for sparc, it's more subtle: ENABLE_ASSEMBLER needs to be set for the normal non YARR JIT. Man, what a mess.
Comment 30 Chris Leary [:cdleary] (not checking bugmail) 2012-01-22 11:39:25 PST
(In reply to Mike Hommey [:glandium] from comment #29)
> Yes, but for sparc, it's more subtle: ENABLE_ASSEMBLER needs to be set for
> the normal non YARR JIT. Man, what a mess.

This is what happens when you don't test non-standard configurations. I had a patch that dropped off my radar in bug 661974 to clean this all up -- I could make an effort to push through if it would be helpful to you guys.
Comment 31 Landry Breuil (:gaston) 2012-01-22 13:18:09 PST
(In reply to Mike Hommey [:glandium] from comment #29)
> Yes, but for sparc, it's more subtle: ENABLE_ASSEMBLER needs to be set for
> the normal non YARR JIT. Man, what a mess.

How is it a problem, since sparc is on the ENABLE_YARR_JIT side ? And even if it wasn't,
that wouldnt be an issue if we use your part of the js/src/Makefile.in patch, it sets ENABLE_ASSEMBLER for sparc too, since it has ENABLE_METHODJIT set to 1 in js/src/configure.in.
Comment 32 Landry Breuil (:gaston) 2012-01-24 23:51:09 PST
Created attachment 591376 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

And here's the corresponding patch, ie porting mike's patch to -current, adds #if ENABLE_ASSEMBLER around ExecutableAllocator definitions/uses (instead of pulling assembler/ExecutableAllocator.cpp in the build), and still ugly return true in checkSyntax, for which i don't know what to do.. m-c builds and starts on ppc with that patch, but i've not really tested it (only ssh -X..).
Would really appreciate things going forward.
Comment 33 Landry Breuil (:gaston) 2012-02-04 01:53:25 PST
Since landing of bug #688069, previous patch (#591376) doesn't apply anymore (regExpPrivateCode disappeared/was renamed ?). I have a bad feeling about fighting for a lost cause, and chasing a moving target.... i don't know the code nor how it works, and blindly adding #ifdefs while trying to 'backport' the patch depresses me.
Comment 34 Landry Breuil (:gaston) 2012-02-06 13:53:01 PST
Created attachment 594798 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

And here's a new updated patch after last commits to that area, which allows me to build m-c on sparc64 and ppc, i've run dromaeo & kraken tests on ppc for a bunch of hours without issues (can post results if needed), and i'm posting the patch from 'Mozilla/5.0 (X11; OpenBSD macppc; rv:13.0a1) Gecko/20120206 Firefox/13.0a1'.
It adds #if ENABLE_ASSEMBLER around execAlloc_ uses, which seems enough for it to run.
Of course this stills unconditionally returns true in checkSyntax(), which i suppose is deeply wrong, but at least it fixes the build for those platforms.
Comment 35 Anthony Catel [:paraboul] 2012-02-08 21:21:17 PST
Any update on this?

I'm trying to build a JIT-free version a SM for armv7.
The last failing piece is related to YARR-JIT.

Landry, I can build without error with your patch but the link fail with :

Undefined symbols for architecture armv7:
  "__ZN3JSC14ExecutablePoolD1Ev", referenced from:
      __ZN3JSC19ExecutableAllocatorD2Ev in jsapi.o
      __ZN2js12RegExpShared6decrefEP9JSContext in jsstr.o
      __ZN2js12RegExpShared14createUncachedEP9JSContextP14JSLinearStringNS_10RegExpFlagEPNS_11TokenStreamE in RegExpObject.o
  "__ZN3JSC4Yarr7executeERNS0_13YarrCodeBlockEPKtjjPi", referenced from:
      __ZN2js12RegExpShared7executeEP9JSContextPKtmPmRNS_14LifoAllocScopeEPPNS_10MatchPairsE in RegExpObject.o
ld: symbol(s) not found for architecture armv7


Any idea?
Comment 36 Landry Breuil (:gaston) 2012-02-09 13:59:58 PST
how are you trying to build it jit-free ? those codepaths are not really tested.. and armv7 falls in the case where ENABLE_ASSEMBLER should be set, which builds ExecutablePool/Allocator code..
Comment 37 Anthony Catel [:paraboul] 2012-02-09 15:03:09 PST
I don't care if the JIT compiler is built. I'm just trying to escape de ENABLE_YARR_JIT path but I'm failing.
Comment 38 Landry Breuil (:gaston) 2012-02-13 09:43:41 PST
Comment on attachment 594798 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

Doesn't apply anymore since landing of bug #724748, since code was shuffled a bit. I'll try to come up with a new patch. Sigh. Can someone care a bit about that issue so that we get something commited, instead of chasing a moving target ?
Comment 39 Landry Breuil (:gaston) 2012-02-13 12:29:15 PST
Created attachment 596748 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

Same patch, updated. Build results in a few hours (if anything else didn't broke on exotic archs..) in http://buildbot.rhaalovely.net/builders/mozilla-central-macppc/builds/54
Comment 40 Landry Breuil (:gaston) 2012-02-13 13:15:41 PST
Created attachment 596771 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

Grr mishmashed patch.. here's a correct one that at least should build js/.
Results in http://buildbot.rhaalovely.net/builders/mozilla-central-macppc/builds/55
Comment 41 Landry Breuil (:gaston) 2012-02-14 01:53:58 PST
And the build was successful, browsing maps.google.fr from my iBook G4 with  http://buildbot.rhaalovely.net/builds/firefox-13.0a1.en-US.openbsd5.1-powerpc.tar.bz2. Apart checkSyntax, it should be okay. Chris, any idea what to do about that, and would it be so wrong to return true, given that it worksforme ?
Comment 42 Landry Breuil (:gaston) 2012-02-14 02:25:22 PST
in bug #684559 comment 34, cameron kaiser talks about the possibility of 'unless simply trying to build it (+/- throwing away the result) is acceptable.' to check syntax on !yarr_jit platforms, how would it be achievable ?
Comment 43 Chris Leary [:cdleary] (not checking bugmail) 2012-02-14 11:00:53 PST
(In reply to Landry Breuil from comment #41)
> Apart checkSyntax, it should be okay. Chris, any idea what
> to do about that, and would it be so wrong to return true, given that it
> worksforme ?

Hey Landry, sorry for the delay on this. We have to do something about checkSyntax. I need to create a patch that disables YARR-JIT for testing anyway, so I'll try to get you a patch that rips out PCRE (leaving it in obviously hasn't been working out) and adds a preprocessor option to disable YARR-JIT entirely and rely so PPC can rely on YARR-interp. I started doing this fallback as a runtime option in bug 661974, but adding a preprocessor seam as well shouldn't be tough.
Comment 44 Landry Breuil (:gaston) 2012-02-16 01:28:18 PST
Will that mean that !yarr-jit will become part of the 'regular testsuite', so that build breakage are detected on tbpl ? Ill monitor #661974, when you have something ready i'll try-compile it on ppc..
Comment 45 Chris Leary [:cdleary] (not checking bugmail) 2012-02-16 14:30:35 PST
(In reply to Landry Breuil from comment #44)
> Will that mean that !yarr-jit will become part of the 'regular testsuite',
> so that build breakage are detected on tbpl ?

Not build breakage, no, since we won't build with !YARR_JIT, but correctness testing (since we can disable the JIT at runtime and test the equivalence of YARR-JIT and YARR-Interp results).
Comment 46 Emmanuel Chourdakis 2012-02-20 08:04:47 PST
Hello, 

I had the same problem on Linux ppc64 with a recent hg spidermonkey checkout.

To this matter, on a recent hg checkout, I tried the patch mentioned in Comment 40, but failed
on jscntxt.cpp. I applied the changes needed by hand and created the following patch in order to make it able to build (did some extra changes  to jsapi.cpp and jscntxt.cpp) 

http://slexy.org/view/s2s8MTv0XP

I tried to build it, but I failed with the following:

http://slexy.org/view/s21lmXnyA1

I don't know if it is relevant to this bug, my mistake or something else. Please excuse me if it's irrelevant.
Comment 47 Mike Hommey [:glandium] 2012-02-24 03:31:55 PST
For checkSyntax, nothing needs to be done, really. Essentially, the #ifdef is wrong, the code is not tied to the YARR JIT, it's tied to YARR. So removing the #ifdef works.
Comment 48 Landry Breuil (:gaston) 2012-02-24 09:27:17 PST
Yes, except that YarrSyntaxChecker.cpp is in CPPSRCS only in ENABLE_YARR_JIT case.. i'll wrap up a patch removing that #ifdef, and adding that cpp file to CPPSRCS in the !ENABLE_YARR_JIT case to see if it helps. Patch has to be updated anyway since landing of #700822
Comment 49 Mike Hommey [:glandium] 2012-02-24 09:44:20 PST
> Yes, except that YarrSyntaxChecker.cpp is in CPPSRCS only in ENABLE_YARR_JIT case.. 

Both your patch and my older patch add YarrSyntaxChecker.cpp to CPPSRCS in the non-ENABLE_YARR_JIT case
Comment 50 Emmanuel Chourdakis 2012-02-24 10:49:27 PST
Just to clarify, I had included the patch in Comment 40 so I have not really knowledge of what happens there. 

Any idea why it complains about MozAssert and MozCrash?
Comment 51 Landry Breuil (:gaston) 2012-02-24 12:03:38 PST
(In reply to Mike Hommey [:glandium] from comment #49)
> > Yes, except that YarrSyntaxChecker.cpp is in CPPSRCS only in ENABLE_YARR_JIT case.. 
> 
> Both your patch and my older patch add YarrSyntaxChecker.cpp to CPPSRCS in
> the non-ENABLE_YARR_JIT case

Yep, but YarrSyntaxChecker.h is only included within #ifdef ENABLE_YARR_JIT.. and even with that 'fixed', i'm now seeing the same error as Emmanuel Chourdakis.

See bottom of http://buildbot.rhaalovely.net/builders/mozilla-central-macppc/builds/60/steps/build/logs/stdio for the full log error.
# nm objdir/dist/lib/libjs_static.a | grep MOZ_                                                                               
00000028 T MOZ_Assert
00000000 T MOZ_Crash

# nm objdir/dist/lib/libmozglue.a 

Assertions.o:
00000000 F Assertions.cpp
00000004 ? MOZ_Assert
00000000 ? MOZ_Crash

I sense something related to your last blog post and related to something which landed recently... any clue ?
Comment 52 Landry Breuil (:gaston) 2012-02-24 12:09:48 PST
Most likely fallout of #717540, maybe because of the support (supposedly, yes) of __attribute__((weak)) in our gcc..

Emmanueel, what's your compiler ?
Comment 53 Mike Hommey [:glandium] 2012-02-24 12:41:15 PST
These symbols are supposed to be in one OR the other, not in both.

The relevant snippet in js/src/Makefile.in is:

ifeq (,$(MOZ_GLUE_PROGRAM_LDFLAGS))
# When building standalone, we need to include mfbt sources, and to declare
# "exported" mfbt symbols on its behalf when we use its headers.
include $(srcdir)/../../mfbt/sources.mk
DEFINES += -DIMPL_MFBT
endif

when building the js engine standalone, MOZ_GLUE_PROGRAM_LDFLAGS is empty, but not when building Gecko.
Comment 54 Landry Breuil (:gaston) 2012-02-24 12:46:32 PST
ok i think i figured it out.. the patch readded Assertions.cpp to CPPSRCS, which was gone from the ENABLE_YARR_JIT case in https://hg.mozilla.org/mozilla-central/diff/71d144fbd53e/js/src/Makefile.in. The breakage might be because mfbt/ is in the search path, and it's that mfbt/Assertions.cpp that was included..

Testing a fix.
Comment 55 Landry Breuil (:gaston) 2012-02-24 13:38:34 PST
Created attachment 600506 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

That was it, with this new patch the build goes past js/. Changes from last patch :
- update after landing of #700822, ie enclose use of execAlloc_ withing #if ENABLE_ASSEMBLER in jscntxt.cpp/setJitHardening()
- remove #if ENABLE_YARR_JIT in checkSyntax
- don't readd Assertions.cpp to CPPSRCS in !ENABLE_YARR_JIT case
- include YarrSyntaxChecker.h inconditionally in js/src/vm/RegExpObject.h

Results of the build in ~10h at http://buildbot.rhaalovely.net/builders/mozilla-central-macppc/builds/61

So, now that the patch looks clean, can it be considered for inclusion ? bug 661974 can still be built on top of that..
Comment 56 Emmanuel Chourdakis 2012-02-25 05:09:25 PST
Sorry for the delay,

I have gcc 4.5.3 on a gentoo ppc64 system.

Patch on Comment 55 worked for me. Built successfully.
Comment 57 Landry Breuil (:gaston) 2012-02-26 15:14:40 PST
(In reply to Landry Breuil from comment #55)
> Created attachment 600506 [details] [diff] [review]
> Use YARR interpreter instead of PCRE on platforms where YARR JIT is not
> supported (m-c patch)
> 
> That was it, with this new patch the build goes past js/. Changes from last
> patch :
> - update after landing of #700822, ie enclose use of execAlloc_ withing #if
> ENABLE_ASSEMBLER in jscntxt.cpp/setJitHardening()
> - remove #if ENABLE_YARR_JIT in checkSyntax
> - don't readd Assertions.cpp to CPPSRCS in !ENABLE_YARR_JIT case
> - include YarrSyntaxChecker.h inconditionally in js/src/vm/RegExpObject.h
> 
> Results of the build in ~10h at
> http://buildbot.rhaalovely.net/builders/mozilla-central-macppc/builds/61

That particular build failed for other reasons (repository in a mixed state) but i've built successfully with that patch, and m-c works fine on OpenBSD/ppc. Chris, can you review it ? i see no reason to wait for 661974, and it would fix the build for exotic architectures.
Comment 58 Chris Leary [:cdleary] (not checking bugmail) 2012-02-26 23:59:00 PST
(In reply to Landry Breuil from comment #57)
> Chris, can you review it ? i see no reason to wait for 661974,
> and it would fix the build for exotic architectures.

Sure, will try to get it done tomorrow!
Comment 59 Landry Breuil (:gaston) 2012-03-01 09:49:17 PST
Review ping ? :)
Comment 60 David Mandelin [:dmandelin] 2012-03-05 17:13:15 PST
Comment on attachment 600506 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

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

Stealing the review because Chris doesn't have time. Looks fine--just one little note about this and bug 731110 but you probably already know anyway.

::: js/src/Makefile.in
@@ +351,5 @@
> +                OSAllocatorWin.cpp \
> +                PageBlock.cpp \
> +                YarrInterpreter.cpp \
> +                YarrPattern.cpp \
> +                YarrSyntaxChecker.cpp \

This probably conflicts with patch 3 in bug 731110. Not sure exactly how you guys want to deal with that, just pointing it out.
Comment 61 Cameron Kaiser [:spectre] 2012-03-05 17:21:51 PST
Let's land this first, since it fixes arches that are actually broken. I'll rebase bug 731110 on top.
Comment 62 Mike Hommey [:glandium] 2012-03-06 00:24:01 PST
Created attachment 603183 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

I refreshed the patch against current mozilla-inbound. Landry, can you check I didn't break anything?
Comment 63 Landry Breuil (:gaston) 2012-03-06 02:18:48 PST
patch #603183 reads good, applies fine on m-c. Doing a testbuild before setting checkin-needed, results in ~6h.
Comment 64 Landry Breuil (:gaston) 2012-03-06 08:11:31 PST
Created attachment 603278 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

And here's a proper hg changeset, applied on top of m-c, running fine as "Mozilla/5.0 (X11; OpenBSD macppc; rv:13.0) Gecko/20120306 Firefox/13.0a1"

I took the liberty of removing the now useless comment in js/src/Makefile.in as Cameron said in comment #27.

-# For architectures without YARR JIT, PCRE is faster than the YARR
-# interpreter (bug 684559).

I suppose we should let the dust settle before reopening the bug removing pcre...
Comment 65 Landry Breuil (:gaston) 2012-03-06 08:13:17 PST
Set checkin-needed, carrying forward dmandelin's r+ on attachement 600506.
Comment 67 Marco Bonardo [::mak] 2012-03-07 05:09:07 PST
backed out as suspected for a bunch of jit tests failures in linux debug builds
see
https://tbpl.mozilla.org/php/getParsedLog.php?id=9878244&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=9878570&tree=Mozilla-Inbound

while this looks like for other platforms, it was the most likely candidate
Comment 68 Mike Hommey [:glandium] 2012-03-07 05:27:35 PST
(In reply to Marco Bonardo [:mak] from comment #67)
> backed out as suspected for a bunch of jit tests failures in linux debug
> builds
> see
> https://tbpl.mozilla.org/php/getParsedLog.php?id=9878244&tree=Mozilla-Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=9878570&tree=Mozilla-Inbound
> 
> while this looks like for other platforms, it was the most likely candidate

Note that theoretically, this patch doesn't change anything on tier-1 platforms.
Comment 69 Marco Bonardo [::mak] 2012-03-07 06:12:08 PST
(In reply to Mike Hommey [:glandium] from comment #68)
> Note that theoretically, this patch doesn't change anything on tier-1
> platforms.

that was my assumption as well, but the backout fixed the problem.
Comment 70 Landry Breuil (:gaston) 2012-03-07 10:18:53 PST
So, what's needed to fix this ? Sorry, i have no idea why it changes behaviour for linux debug builds, since it's only shuffling ifdefs around... and the testsuite log isnt really verbose, besides saying there's a failure.
Comment 71 Mike Hommey [:glandium] 2012-03-07 10:32:06 PST
Well, if it's breaking linux debug builds, it means it's *not* only shuffling ifdefs around. So you should start by checking that.
Comment 72 David Mandelin [:dmandelin] 2012-03-07 10:36:36 PST
(In reply to Landry Breuil from comment #70)
> So, what's needed to fix this ? Sorry, i have no idea why it changes
> behaviour for linux debug builds, since it's only shuffling ifdefs around...
> and the testsuite log isnt really verbose, besides saying there's a failure.

You could try running one of the failing tests, say bug706316.js, with the -so option in the harness, which will show the command line and output.
Comment 73 Landry Breuil (:gaston) 2012-03-07 12:01:24 PST
(In reply to Mike Hommey [:glandium] from comment #71)
> Well, if it's breaking linux debug builds, it means it's *not* only
> shuffling ifdefs around. So you should start by checking that.

It shuffles ENABLER_YARR_JIT, adds #if ENABLE_ASSEMBLER around execAlloc, ENABLE_ASSEMBLER is set on tier1 archs, as is ENABLE_YARR_JIT. And i don't have a linux box anyway to try this on...
Comment 74 Landry Breuil (:gaston) 2012-03-07 13:16:24 PST
Just to make sure, i've pushed the offending diff to try with -b do -p linux,linux64 : 
https://tbpl.mozilla.org/?tree=Try&rev=d77a3095728e
Comment 75 David Anderson [:dvander] 2012-03-07 17:01:06 PST
The failure in comment #72:

Program received signal SIGSEGV, Segmentation fault.
0x081107fa in js::gc::CheckMarkedThing<js::Shape> (trc=0x8577ca0, thing=0xf750bd78 "i") at /home/dvander/mozilla/mozilla-inbound/js/src/jsgcmark.cpp:81
81	    JS_ASSERT_IF(trc->runtime->gcCurrentCompartment, IS_GC_MARKING_TRACER(trc));
(gdb) bt 10
#0  0x081107fa in js::gc::CheckMarkedThing<js::Shape> (trc=0x8577ca0, thing=0xf750bd78 "i") at /home/dvander/mozilla/mozilla-inbound/js/src/jsgcmark.cpp:81
#1  0x08112fbe in js::gc::MarkInternal<js::Shape> (trc=0x8577ca0, thing=0xf750bd78 "i") at /home/dvander/mozilla/mozilla-inbound/js/src/jsgcmark.cpp:93
#2  0x0810ee00 in js::gc::MarkUnbarriered<js::Shape> (trc=0x8577ca0, thingp=0xffffc5b8, name=0x83c6a56 "write barrier") at /home/dvander/mozilla/mozilla-inbound/js/src/jsgcmark.cpp:132
#3  0x0810b5ee in js::gc::MarkShapeUnbarriered (trc=0x8577ca0, thingp=0xffffc5b8, name=0x83c6a56 "write barrier") at /home/dvander/mozilla/mozilla-inbound/js/src/jsgcmark.cpp:211
#4  0x0808c88b in js::Shape::writeBarrierPre (shape=0xf750bd78 "i") at ../jsscopeinlines.h:399
#5  0x0808fc19 in js::HeapPtr<js::Shape, unsigned int>::pre (this=0xf7506464) at /home/dvander/mozilla/mozilla-inbound/js/src/gc/Barrier.h:243
#6  0x080a83f5 in js::HeapPtr<js::Shape, unsigned int>::operator= (this=0xf7506464, v=0xf750be20 "i") at /home/dvander/mozilla/mozilla-inbound/js/src/gc/Barrier.h:215
#7  0x081ce226 in js::Bindings::setParent (this=0xf7506464, cx=0x859dc98, obj=0xf7503040 [Object global]) at /home/dvander/mozilla/mozilla-inbound/js/src/jsscope.cpp:1397
#8  0x081226fc in js::types::TypeScript::SetScope (cx=0x859dc98, script=0xf7506420, scope=0xf7503040 [Object global]) at /home/dvander/mozilla/mozilla-inbound/js/src/jsinfer.cpp:5197
#9  0x08128ef8 in JSScript::ensureRanAnalysis (this=0xf7506420, cx=0x859dc98, scope=0xf7503040 [Object global]) at ../jsinferinlines.h:1391

(gdb) f 0
#0  0x081107fa in js::gc::CheckMarkedThing<js::Shape> (trc=0x8577ca0, thing=0xf750bd78 "i") at /home/dvander/mozilla/mozilla-inbound/js/src/jsgcmark.cpp:81
81	    JS_ASSERT_IF(trc->runtime->gcCurrentCompartment, IS_GC_MARKING_TRACER(trc));
(gdb) p trc.runtime.gcCurrentCompartment 
Cannot access memory at address 0x2951
(gdb) p trc.runtime
$1 = (JSRuntime *) 0x1d5
Comment 76 Landry Breuil (:gaston) 2012-03-08 01:04:16 PST
The try builds on linux64 failed due to hg errors, i've asked the rebuild but the linux debug build shows the same failure.

- what is the difference between debug and non debug builds in terms of JIT ? ENABLE_ASSEMBLER and ENABLE_YARR_JIT are set in both on linux, from what i can tell.
- the patch doesnt touch anything under jsgcmark.cpp, and gCurrentCompartment is still initialized to null in jsapi.cpp:JSRuntime::JSRuntime(). The next (and only) initialization of cCurrentCompartment i could find is in jsgc.cpp:AutoGCSession::AutoGCSession(), called from GCCycle() in the same file. Anyway, from your debug session it even seems JSRuntime::JSRuntime() isnt called, since it's not defined to a viable value.
- Any idea why those codepaths seems not used only in debug builds, and only with my changes ?
Comment 77 Landry Breuil (:gaston) 2012-03-12 14:10:47 PDT
Re-read the diff, and a bit out of ideas i've pushed a similar changeset with s/#if ENABLE_ASSEMBLER/#ifdef ENABLE_ASSEMBLER/, but i doubt it'll change anything.

https://tbpl.mozilla.org/?tree=Try&rev=ff2bd33b8476

To me, JS_METHODJIT includes ENABLE_YARR_JIT, so that chunk in js/src/vm/RegExpObject.cpp could go away.. :

-#ifdef JS_METHODJIT
+#if ENABLE_YARR_JIT && defined(JS_METHODJIT)
if (isJITRuntimeEnabled(cx) && !yarrPattern.m_containsBackreferences) {
Comment 78 Landry Breuil (:gaston) 2012-03-13 00:56:29 PDT
Try build still fails the same on debug builds only.
Comment 79 Landry Breuil (:gaston) 2012-03-13 12:36:20 PDT
Trying to find a common denominator in the failing 9 tests :

some tests fail for all 9 combinaisons of jit_test.py arguments : tests/basic/bug708805.js, bug713226.js, bug716013.js, bug728609.js, bug729364.js

some fail for only 7 combinaisons out of 9, and goes into 2 classes :
 the ones that fail in all combinaisons but the ones where '-a -m -n' is used : bug722028.js, bug706316.js
 the ones that fail in all combinaisons but the ones where '-a -m' (but NOT -n!) is used : bug708228.js, bug707750.js 

Don't really see what the arguments mean.. -m = methodjit, but -a is also methodjit through 'mjitalways' ? -n is nitro ?

A common denominator : all those tests (but bug729364.js) use 'gczeal(4)', but some not failing-tests use it too : bug734196.js, bug704795.js, bug710947.js, bug734196.js

Other than that, i have no idea what can be the common thing between those tests, and why do they fail only on linux debug builds.
Comment 80 Cameron Kaiser [:spectre] 2012-03-13 13:35:23 PDT
-m = Methodjit
-n = type infereNce
-a = Always run in jit
Comment 81 Cameron Kaiser [:spectre] 2012-03-13 13:54:01 PDT
I'm no expert in JS, but I think the problem you're having is that one of these ifdefs is causing an object to be marked wrong for garbage collection and it's violating an invariant. The GC depends on the property that there can be no black to grey edges (i.e., reachable to unknown), or the cycle collector may mistakenly free things that are still reachable. I suspect the problem is not that you did something wrong in your patch per se, but that not all the bases are covered. But I don't use Linux, so I'm not able to help you here much more other than to consider breaking up the assert and seeing which part it is actually failing (that assert is a macro with two conditions; see also jsgc.h).

Do you mind if I get bug 731110 landed in the meantime?
Comment 82 David Mandelin [:dmandelin] 2012-03-13 18:38:24 PDT
Landry, following up on a hint from billm, I got your patch to work on Linux with this addition:

diff -r e7bbcbb6c24a js/src/jscntxt.h
--- a/js/src/jscntxt.h	Tue Mar 13 17:42:33 2012 -0700
+++ b/js/src/jscntxt.h	Tue Mar 13 18:32:46 2012 -0700
@@ -64,6 +64,7 @@
 #include "js/HashTable.h"
 #include "js/Vector.h"
 #include "vm/Stack.h"
+#include "assembler/jit/ExecutableAllocator.h"
 
 #ifdef _MSC_VER
 #pragma warning(push)


I'm not quite sure what's going on, but it kind of seems like ENABLE_ASSEMBLER=1 is not being set for all the files in the build, and so different object files get different definitions of JSRuntime, which causes the trc field to get overwritten with junk, and then you crash. Your patch does modify how ENABLE_ASSEMBLER is set, so it seems vaguely plausible that could be the problem.

The strange thing is that it seems like js.cpp is the file that doesn't get ENABLE_ASSEMBLER=1, but I do see it being set on the command line to build js.o. And it also doesn't make much sense that the patch above would solve the problem.

Bug 731110 might just make things easy: if you don't have to make execAlloc_ conditional on ENABLE_ASSEMBLER (because you'll have the assembler), then you won't have to modify jscntxt.h and the Makefile.in, and it should just work.
Comment 83 Cameron Kaiser [:spectre] 2012-03-13 18:51:49 PDT
He'll still need it for sparc64, though (and the PPC assembler is only for OS X ABI, and *BSD/ppc is SysV ABI).
Comment 84 Cameron Kaiser [:spectre] 2012-03-13 18:53:05 PDT
(er, unless someone(tm) adds the necessary ABI sauce to it, which is just a matter of doing it(tm))
Comment 85 Landry Breuil (:gaston) 2012-03-14 02:57:04 PDT
(In reply to David Mandelin from comment #82)
> Landry, following up on a hint from billm, I got your patch to work on Linux
> with this addition:
> 
> diff -r e7bbcbb6c24a js/src/jscntxt.h
> --- a/js/src/jscntxt.h	Tue Mar 13 17:42:33 2012 -0700
> +++ b/js/src/jscntxt.h	Tue Mar 13 18:32:46 2012 -0700
> @@ -64,6 +64,7 @@
>  #include "js/HashTable.h"
>  #include "js/Vector.h"
>  #include "vm/Stack.h"
> +#include "assembler/jit/ExecutableAllocator.h"
>  
>  #ifdef _MSC_VER
>  #pragma warning(push)
> 
> 
> I'm not quite sure what's going on, but it kind of seems like
> ENABLE_ASSEMBLER=1 is not being set for all the files in the build, and so
> different object files get different definitions of JSRuntime, which causes
> the trc field to get overwritten with junk, and then you crash. Your patch
> does modify how ENABLE_ASSEMBLER is set, so it seems vaguely plausible that
> could be the problem.

Aha. Very interesting... a shroedingbug.
 
> The strange thing is that it seems like js.cpp is the file that doesn't get
> ENABLE_ASSEMBLER=1, but I do see it being set on the command line to build
> js.o. And it also doesn't make much sense that the patch above would solve
> the problem.

If i look at old build logs on amd64, i don't see ENABLE_ASSEMBLER=1 in the js.o build
line, maybe because it's set in js/src/Makefile.in and not in js/src/shell/Makefile.in ?

I'll recheck with a clean tip and only that patch. 
I've also pushed the corresponding patchset to https://tbpl.mozilla.org/?tree=Try&rev=88122a478851

For me that change make sense, since that brings ExecutableAllocator definition
to jscntxt.h where as of now, it's used without knowing how it's defined.

> 
> Bug 731110 might just make things easy: if you don't have to make execAlloc_
> conditional on ENABLE_ASSEMBLER (because you'll have the assembler), then
> you won't have to modify jscntxt.h and the Makefile.in, and it should just
> work.

Yes, as cameron points out it's also needed for other archs (sparc64, broken anyway,
but i try to keep it in a compilable state with that patch, ia64, see #729447, and maybe others..)

Unfortunately i'm not in a position to write the ppc assembler for sysV ABI.
Comment 86 Mike Hommey [:glandium] 2012-03-14 03:06:39 PDT
(In reply to Landry Breuil from comment #85)
> (In reply to David Mandelin from comment #82)
> > Landry, following up on a hint from billm, I got your patch to work on Linux
> > with this addition:
> > 
> > diff -r e7bbcbb6c24a js/src/jscntxt.h
> > --- a/js/src/jscntxt.h	Tue Mar 13 17:42:33 2012 -0700
> > +++ b/js/src/jscntxt.h	Tue Mar 13 18:32:46 2012 -0700
> > @@ -64,6 +64,7 @@
> >  #include "js/HashTable.h"
> >  #include "js/Vector.h"
> >  #include "vm/Stack.h"
> > +#include "assembler/jit/ExecutableAllocator.h"
> >  
> >  #ifdef _MSC_VER
> >  #pragma warning(push)
> > 
> > 
> > I'm not quite sure what's going on, but it kind of seems like
> > ENABLE_ASSEMBLER=1 is not being set for all the files in the build, and so
> > different object files get different definitions of JSRuntime, which causes
> > the trc field to get overwritten with junk, and then you crash. Your patch
> > does modify how ENABLE_ASSEMBLER is set, so it seems vaguely plausible that
> > could be the problem.
> 
> Aha. Very interesting... a shroedingbug.
>  
> > The strange thing is that it seems like js.cpp is the file that doesn't get
> > ENABLE_ASSEMBLER=1, but I do see it being set on the command line to build
> > js.o. And it also doesn't make much sense that the patch above would solve
> > the problem.
> 
> If i look at old build logs on amd64, i don't see ENABLE_ASSEMBLER=1 in the
> js.o build
> line, maybe because it's set in js/src/Makefile.in and not in
> js/src/shell/Makefile.in ?
> 
> I'll recheck with a clean tip and only that patch. 
> I've also pushed the corresponding patchset to
> https://tbpl.mozilla.org/?tree=Try&rev=88122a478851
> 
> For me that change make sense, since that brings ExecutableAllocator
> definition
> to jscntxt.h where as of now, it's used without knowing how it's defined.

Except iirc, including that header leads to a failure to build on various architectures. So you'd fix what the patch here broke, but you break what the patch was supposed to fix.

The right solution is probably to define ENABLE_ASSEMBLER elsewhere than js/src/Makefile.in.
Comment 87 Mike Hommey [:glandium] 2012-03-14 03:08:18 PDT
The best place is probably js-config.h.in, so that external apps building against the headers don't end up with the same problem.
Comment 88 Landry Breuil (:gaston) 2012-03-14 03:26:00 PDT
(In reply to Mike Hommey [:glandium] from comment #86)
> (In reply to Landry Breuil from comment #85)
> > (In reply to David Mandelin from comment #82)
> > > Landry, following up on a hint from billm, I got your patch to work on Linux
> > > with this addition:
> > > 
> > > diff -r e7bbcbb6c24a js/src/jscntxt.h
> > > --- a/js/src/jscntxt.h	Tue Mar 13 17:42:33 2012 -0700
> > > +++ b/js/src/jscntxt.h	Tue Mar 13 18:32:46 2012 -0700
> > > @@ -64,6 +64,7 @@
> > >  #include "js/HashTable.h"
> > >  #include "js/Vector.h"
> > >  #include "vm/Stack.h"
> > > +#include "assembler/jit/ExecutableAllocator.h"
> > >  
> > >  #ifdef _MSC_VER
> > >  #pragma warning(push)
> > > 
> > > 
> > > I'm not quite sure what's going on, but it kind of seems like
> > > ENABLE_ASSEMBLER=1 is not being set for all the files in the build, and so
> > > different object files get different definitions of JSRuntime, which causes
> > > the trc field to get overwritten with junk, and then you crash. Your patch
> > > does modify how ENABLE_ASSEMBLER is set, so it seems vaguely plausible that
> > > could be the problem.
> > 
> > Aha. Very interesting... a shroedingbug.
> >  
> > > The strange thing is that it seems like js.cpp is the file that doesn't get
> > > ENABLE_ASSEMBLER=1, but I do see it being set on the command line to build
> > > js.o. And it also doesn't make much sense that the patch above would solve
> > > the problem.
> > 
> > If i look at old build logs on amd64, i don't see ENABLE_ASSEMBLER=1 in the
> > js.o build
> > line, maybe because it's set in js/src/Makefile.in and not in
> > js/src/shell/Makefile.in ?
> > 
> > I'll recheck with a clean tip and only that patch. 
> > I've also pushed the corresponding patchset to
> > https://tbpl.mozilla.org/?tree=Try&rev=88122a478851
> > 
> > For me that change make sense, since that brings ExecutableAllocator
> > definition
> > to jscntxt.h where as of now, it's used without knowing how it's defined.
> 
> Except iirc, including that header leads to a failure to build on various
> architectures. So you'd fix what the patch here broke, but you break what
> the patch was supposed to fix.

Well, not sure, since the patch removes #if ENABLE_YARR_JIT around the inclusion of assembler/jit/ExecutableAllocator.h and it doesnt seem to break that much things. But i agree it should be included _only_ if ENABLE_ASSEMBLER is set. It makes more sense, and the rest of the patch makes the presence of this header unneeded if ENABLE_ASSEMBLER is not set.

> 
> The right solution is probably to define ENABLE_ASSEMBLER elsewhere than
> js/src/Makefile.in.

Righto.. but ENABLE_YARR_JIT is not known when you're creating js-config.h, since it's in Makefile.in. and ENABLE_YARR_JIT is needed to know if we need to define ENABLE_ASSEMBLER. So ENABLE_YARR_JIT definition would have to go to configure too ?
Comment 89 Landry Breuil (:gaston) 2012-03-14 03:31:21 PDT
I've looked at try logs, and yes ENABLE_ASSEMBLER=1 is not set on the commandline when building under shell/ with the patch.
Comment 90 Mike Hommey [:glandium] 2012-03-14 03:34:21 PDT
(In reply to Landry Breuil from comment #88)
> Righto.. but ENABLE_YARR_JIT is not known when you're creating js-config.h,
> since it's in Makefile.in. and ENABLE_YARR_JIT is needed to know if we need
> to define ENABLE_ASSEMBLER. So ENABLE_YARR_JIT definition would have to go
> to configure too ?

Yes. That these things are in Makefile.in is a mistake anyways.
Comment 91 David Mandelin [:dmandelin] 2012-03-14 09:53:37 PDT
(In reply to Landry Breuil from comment #89)
> I've looked at try logs, and yes ENABLE_ASSEMBLER=1 is not set on the
> commandline when building under shell/ with the patch.

Ah, OK. I looked at it in my Linux VM, and I really thought I saw ENABLE_ASSEMBLER in the command line for js.o, but given that that makes no sense, it makes sense that you found it's the opposite. :-)

(In reply to Mike Hommey [:glandium] from comment #90)
> Yes. That these things are in Makefile.in is a mistake anyways.

JSC sets those flags in what we imported as assembler/wtf/Platform.h, which seems reasonable. In fact, I see they set ENABLE_ASSEMBLER in there depending on various conditions, which is maybe why the patch works on most platforms already.

js-config.h also seems like a reasonable place, although since there is already ENABLE_ASSEMBLER and ENABLE_JIT stuff in Platform.h, maybe it will be easier to keep consistent if we do it there.
Comment 92 Landry Breuil (:gaston) 2012-03-14 10:04:40 PDT
Sidenote : https://tbpl.mozilla.org/?tree=Try&rev=88122a478851 with inclusion of jit/ExecutableAllocator.h in jscntxt.h is all green, so we're definitely on the right track.
Comment 93 Landry Breuil (:gaston) 2012-03-14 10:14:59 PDT
(In reply to David Mandelin from comment #91)
> (In reply to Mike Hommey [:glandium] from comment #90)
> > Yes. That these things are in Makefile.in is a mistake anyways.
> 
> JSC sets those flags in what we imported as assembler/wtf/Platform.h, which
> seems reasonable. In fact, I see they set ENABLE_ASSEMBLER in there
> depending on various conditions, which is maybe why the patch works on most
> platforms already.

Erm... looking at Platform.h, ENABLE_ASSEMBLER is set :
- #if WTF_PLATFORM_WX -> wxWidgets ? Seriously ?
- #if WTF_CPU_SH4 && WTF_PLATFORM_QT -> does it concern mozilla ?
- #if ENABLE_JIT || ENABLE_YARR_JIT -> so enabling yarr jit sets ENABLE_ASSEMBLER.

Given that, why bothering setting ENABLE_ASSEMBLER at all in Makefile.in ? Are there cases where METHODJIT is enabled and YARR JIT is not ? What a #define maze..

> js-config.h also seems like a reasonable place, although since there is
> already ENABLE_ASSEMBLER and ENABLE_JIT stuff in Platform.h, maybe it will
> be easier to keep consistent if we do it there.

Wait, so what should move where ? ENABLE_YARR_JIT & ENABLE_ASSEMBLER to js-config.h & configure.in ? ENABLE_YARR_JIT to Platform.h ? Do we keep the list of yarr jit architectures in Makefile.in ?
Comment 94 Mike Hommey [:glandium] 2012-03-14 10:28:28 PDT
(In reply to David Mandelin from comment #91)
> JSC sets those flags in what we imported as assembler/wtf/Platform.h, which
> seems reasonable. In fact, I see they set ENABLE_ASSEMBLER in there
> depending on various conditions, which is maybe why the patch works on most
> platforms already.

Platform.h is not supposed to be a public header; at least it's not exposed as one presently. Relying on it is going to break third parties using public headers.
Comment 95 David Mandelin [:dmandelin] 2012-03-14 10:48:14 PDT
Mike, what exactly do you mean by Platform.h being or not being a public header? Do you mean being installed to an include/ dir, or do you mean getting included into jsapi.h? AFAICT it is neither at the moment, just trying to see what you're getting at.

Landry, it's definitely a maze. Sorting it out proper would be a lot of work so I definitely don't expect you to fix everything as part of this patch. It seems important to make sure ENABLE_ASSEMBLER is consistent, though, so combined with Mike's point about Platform.h not being public, it seems that we shouldn't try to set ENABLE_ASSEMBLER, ENABLE_YARR_JIT or ENABLE_JIT in the Makefile, and we shouldn't use them in public header files. 

One idea is to create a new thing that is set on the same conditions when ENABLE_ASSEMBLER is set and put that in js-config.h. Then you could use that in jscntxt.h without causing any problems. Then we could use macros to check that the two are consistent on one of the other files, like YarrJIT.cpp.
Comment 96 Dan Horák 2012-04-11 00:01:54 PDT
Can we get an update what's the status of this bug, please? Comment #92 looks optimistic. I'm asking whether we should attempt to build Xulrunner on Fedora s390 and ppc.
Comment 97 Landry Breuil (:gaston) 2012-04-12 09:58:50 PDT
(In reply to David Mandelin from comment #95)

> Landry, it's definitely a maze. Sorting it out proper would be a lot of work
> so I definitely don't expect you to fix everything as part of this patch. It
> seems important to make sure ENABLE_ASSEMBLER is consistent, though, so
> combined with Mike's point about Platform.h not being public, it seems that
> we shouldn't try to set ENABLE_ASSEMBLER, ENABLE_YARR_JIT or ENABLE_JIT in
> the Makefile, and we shouldn't use them in public header files. 
> 
> One idea is to create a new thing that is set on the same conditions when
> ENABLE_ASSEMBLER is set and put that in js-config.h. Then you could use that
> in jscntxt.h without causing any problems. Then we could use macros to check
> that the two are consistent on one of the other files, like YarrJIT.cpp.

Getting back to that bug.. I'm a bit lost, since ENABLE_JIT, ENABLE_YARR_JIT and ENABLE_ASSEMBLER are #defined in wtf/Platform.h (depending on tons of conditions), and also set in Makefile.in. Is it wise to keep the same conflicting names ?

If i add a single variable (say JS_HAS_ASSEMBLER or JS_HAS_JIT.. gah, speak about meaningful names..) set arch-dependant (the ones with yarr jit : arm% sparc %86 x86_64 mips%) in configure.in & js-config.h where should that variable be used ? 

- replace all #ifdef ENABLE_ASSEMBLER/#ifdef ENABLE_JIT/#ifdef ENABLE_YARR_JIT, since they seem to overlap ?
- or i should just use it in the chunks i'm #ifdef'ing in my attempts to fix powerpc ?
- in the same patch, should i also remove all ENABLE_ASSEMBLER/ENABLE_JIT/ENABLE_YARR_JIT=1 from Makefile.in ?

Just askin', before wrapping a patch...
Comment 98 Landry Breuil (:gaston) 2012-04-12 10:27:09 PDT
And looking at Makefile.in, the list of archs is really the same as ENABLE_METHODJIT, so i'd be tempted to replace ifeq (,$(filter arm% sparc %86 x86_64 mips%,$(TARGET_CPU))) by ifdef ENABLE_METHODJIT, or even better merge the two blocks with the ENABLE_METHODJIT above. Thoughs ?
Comment 99 David Mandelin [:dmandelin] 2012-04-12 12:32:27 PDT
I would suggest making the minimal change that actually works and doesn't make things more confusing. Exactly what that would be, I don't know without trying things out. My *guess* would be that adding JS_ENABLE_JIT to Makefile.in and using it only in the places you need here would be it. I'm not sure exactly what blocks you are talking about in comment 98, but it sounds like a good idea.
Comment 100 Landry Breuil (:gaston) 2012-04-13 02:10:09 PDT
(In reply to David Mandelin from comment #99)
> I would suggest making the minimal change that actually works and doesn't
> make things more confusing. Exactly what that would be, I don't know without
> trying things out. My *guess* would be that adding JS_ENABLE_JIT to
> Makefile.in and using it only in the places you need here would be it. I'm

Erm... you lost me here. I though we aimed at removing those *ENABLE* being set in Makefile.in and moving them to configure.in/js-config.h. If i add a new JS_ENABLE_JIT only overlapping with the others without removing them from Makefile.in that'll only confuse things.
Remember that the issue we're trying to fix is getting ENABLE_ASSEMBLER "known" under shell/ so that the test failures are fixed.

> not sure exactly what blocks you are talking about in comment 98, but it
> sounds like a good idea.

The list of archs where METHODJIT is enabled is the same as the list of archs where YARR_JIT is enabled, the former being set in configure.in and the latter being set in Makefile.in. I dont know if they will be always the same, and if so we could "merge" the two defines.. 

http://mxr.mozilla.org/mozilla-central/source/js/src/Makefile.in#262 <- ifdef ENABLE_METHODJIT block 
http://mxr.mozilla.org/mozilla-central/source/js/src/Makefile.in#344 <- list of ENABLE_YARR_JIT archs
Comment 101 Landry Breuil (:gaston) 2012-04-16 05:06:42 PDT
I've tried a slightly different approach in https://tbpl.mozilla.org/?tree=Try&rev=49b9616807cc but looking at the results it wasnt working 100%..

The idea (https://hg.mozilla.org/try/rev/96ce117be086) was to set ENABLE_ASSEMBLER=1 through configure.in + config/autoconf.mk so that it got applied all under js/src (including shell/) but it seems i have failed at it. not sure it's the right place to set a -D flag, but i'm not comfortable adding a 'too anonymous and not JS_ prefixed' ENABLE_ASSEMBLER #define to js-config.h.

Thougths ?
Comment 102 David Mandelin [:dmandelin] 2012-04-17 17:40:03 PDT
This is getting really confusing--I feel like I need to back up a few steps.

1. First off, do you still need to leave out the executableAllocators in jscntxt.h? If you have the JITs now, then it seems like you don't even need to patch jscntxt.h. I'm going to assume you do, and continue, but if we don't need to do anything, that would be great.

2. Second, if possible, it'd be nice to do something small and simple that just fixes the immediate problem. The immediate problem, IIUC, is that you can't have the executableAllocators in JSContext, and #ifdef ENABLE_ASSEMBLER in jscntxt.h doesn't work. So, what about just providing a dummy class for ExecutableAllocator on SPARC? Would that work? You could put it in a .cpp file guarded by !ENABLE_ASSEMBLER.
Comment 103 Landry Breuil (:gaston) 2012-04-18 12:43:34 PDT
(In reply to David Mandelin from comment #102)
> This is getting really confusing--I feel like I need to back up a few steps.

Yes, i'm also confused.

> 1. First off, do you still need to leave out the executableAllocators in
> jscntxt.h? If you have the JITs now, then it seems like you don't even need
> to patch jscntxt.h. I'm going to assume you do, and continue, but if we
> don't need to do anything, that would be great.

There are platforms where there's no JIT (and hence no ExecutableAllocator), for example sparc64 & macppc/sysv abi. So we have to #ifdef ENABLE_ASSEMBLER out the execAlloc uses. 

> 2. Second, if possible, it'd be nice to do something small and simple that
> just fixes the immediate problem. The immediate problem, IIUC, is that you
> can't have the executableAllocators in JSContext, and #ifdef
> ENABLE_ASSEMBLER in jscntxt.h doesn't work. So, what about just providing a
> dummy class for ExecutableAllocator on SPARC? Would that work? You could put
> it in a .cpp file guarded by !ENABLE_ASSEMBLER.

To me, the immediate problem is that i've a diff that works, but it breaks some tests in subtle ways just because ENABLE_ASSEMBLER=1 is not propagated anymore under shell/ with my diff.

ExecutableAllocator.h has the arch-dependant machinery, and it #errors out on the archs we care about in this bug report.. because (at least!) there's no implementation for 'cacheFlush', which seems to require writing some low-level asm, and i have no idea if it can be made a nop.

x86/x86_64 provides an empty implementation, but iirc i tried adding ppc to also use that empty implem and it failed to build.
Comment 104 David Mandelin [:dmandelin] 2012-04-18 17:46:13 PDT
(In reply to Landry Breuil from comment #103)
> (In reply to David Mandelin from comment #102)
> > This is getting really confusing--I feel like I need to back up a few steps.
> 
> Yes, i'm also confused.
> 
> > 1. First off, do you still need to leave out the executableAllocators in
> > jscntxt.h? If you have the JITs now, then it seems like you don't even need
> > to patch jscntxt.h. I'm going to assume you do, and continue, but if we
> > don't need to do anything, that would be great.
> 
> There are platforms where there's no JIT (and hence no ExecutableAllocator),
> for example sparc64 & macppc/sysv abi. So we have to #ifdef ENABLE_ASSEMBLER
> out the execAlloc uses. 

OK. Good to know.

> > 2. Second, if possible, it'd be nice to do something small and simple that
> > just fixes the immediate problem. The immediate problem, IIUC, is that you
> > can't have the executableAllocators in JSContext, and #ifdef
> > ENABLE_ASSEMBLER in jscntxt.h doesn't work. So, what about just providing a
> > dummy class for ExecutableAllocator on SPARC? Would that work? You could put
> > it in a .cpp file guarded by !ENABLE_ASSEMBLER.
> 
> To me, the immediate problem is that i've a diff that works, but it breaks
> some tests in subtle ways just because ENABLE_ASSEMBLER=1 is not propagated
> anymore under shell/ with my diff.

Sure, but if we try to get ENABLE_ASSEMBLER=1 propagated, it seems like it's going to involve either a gross hack, or a lot of work to redo the options everywhere. So I was looking for an easier way first.

> ExecutableAllocator.h has the arch-dependant machinery, and it #errors out
> on the archs we care about in this bug report.. because (at least!) there's
> no implementation for 'cacheFlush', which seems to require writing some
> low-level asm, and i have no idea if it can be made a nop.

Which archs do have a problem? It looks like there is an implementation for SPARC. We're still missing PPC, I guess.

> x86/x86_64 provides an empty implementation, but iirc i tried adding ppc to
> also use that empty implem and it failed to build.

I think an empty implementation should build on PPC. Whether it's correct, I don't know, but it only matters if using the JITs.
Comment 105 Cameron Kaiser [:spectre] 2012-04-18 19:48:05 PDT
The cacheFlush implementation I have for OS X/ppc is OS X specific and calls sys_icache_invalidate() which is provided by the kernel. You could fake it up with isync/icbi on other architectures (a blank one wouldn't be correct, PPCs are very cache dependent).
Comment 106 Cameron Kaiser [:spectre] 2012-04-18 19:48:35 PDT
s/architectures/operating systems/
Comment 107 Mike Hommey [:glandium] 2012-04-18 23:09:40 PDT
A safe way to implement cacheFlush that would be to have a dummy implementation that does nothing (cacheFlush is completely unimportant when there is no JIT), with the following preprocessor goop (or something similar):
#ifndef ENABLE_ASSEMBLER
#error cacheFlush unimplemented for this platform
#endif.
Comment 108 ifinisheri 2012-04-19 19:09:17 PDT
I encountered the same question while building firefox11.0 and patches here seem like to be the right answer.
But when I checked the latest patch,which is modified on 2012-03-06 08:11 PST, by Landry Breuil,I found this patch can hardly match souce code in js/src of FF11.0.
I wonder that if the patch only support for FF10.0 or that I have checked out the wrong version patch?
By the way, can anyone tell me how to use the patch here?Each time I used it ,I just copy the code of the patch down and then manually compared it with the js/src. which takes a rather long time and you know,I am just irritated by that.
Any useful suggestions would be appreciated,thx!
Comment 109 Jory A. Pratt 2012-04-20 19:07:58 PDT
(In reply to ifinisheri from comment #108)
> I encountered the same question while building firefox11.0 and patches here
> seem like to be the right answer.
> But when I checked the latest patch,which is modified on 2012-03-06 08:11
> PST, by Landry Breuil,I found this patch can hardly match souce code in
> js/src of FF11.0.
> I wonder that if the patch only support for FF10.0 or that I have checked
> out the wrong version patch?
> By the way, can anyone tell me how to use the patch here?Each time I used it
> ,I just copy the code of the patch down and then manually compared it with
> the js/src. which takes a rather long time and you know,I am just irritated
> by that.
> Any useful suggestions would be appreciated,thx!

'man patch', for starts tho it would be `patch -p1 --dry-run < /path/to/patch` to check it for failure if all went well you could drop the --dry-run. Hope that helps ya a bit.
Comment 110 Cameron Kaiser [:spectre] 2012-04-22 21:17:44 PDT
Landry, how close are you to a patch for this? I'd like to get bug 731110 revised and landed with the next train so that downstreamers can get it and I can stop schlepping it around in our own changesets. It may actually simplify your work. If you have a new patch imminent, I'll wait, but if not I'd like to get the PPC mjit in first.
Comment 111 Landry Breuil (:gaston) 2012-04-30 13:49:22 PDT
(In reply to Cameron Kaiser from comment #110)
> Landry, how close are you to a patch for this? I'd like to get bug 731110
> revised and landed with the next train so that downstreamers can get it and
> I can stop schlepping it around in our own changesets. It may actually
> simplify your work. If you have a new patch imminent, I'll wait, but if not
> I'd like to get the PPC mjit in first.

Please go ahead, that issue depresses me :) I'll see what your patch breaks for me !
Comment 112 Landry Breuil (:gaston) 2012-05-01 02:34:58 PDT
Created attachment 619876 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

Fwiw here's the (simpler) patch i'm working on (no, i didn't gave up..) :
- it doesnt touch the ENABLE_ASSEMBLER hell -> shouldnt affect tier1 archs
- it uses the dummy empty cacheFlush implem for PPC in ExecutableAllocator.h
- adds ExecutableAllocator.cpp/ExecutableAllocatorPosix.cpp to CPPSRCS in !YARR_JIT case (i didnt do the ifdef WIN/OS2 dance, as i doubt win/os2 might fall in the !YARR_JIT case someday.. prove me wrong :)
- inconditionally includes ExecutableAllocator.h in wtfbridge.h so that the definition of ExecutableAllocator class is available everywhere

It currently builds a working shell on ppc (regress tests passes except testConservativeGC but that seems related to #750620), i've yet to confirm it produces a working firefox. I'll also see if it fixes the build on sparc64, and then i'll send it to try to see if it breaks anything else.
Comment 113 Cameron Kaiser [:spectre] 2012-05-01 08:15:30 PDT
Can you change the ifsef WTF_CPU_PPC to defined(WTF_CPU_PPC) && !defined(JS_CPU_PPC_OSX) around cacheFlush?
Comment 114 Landry Breuil (:gaston) 2012-05-01 12:11:07 PDT
(In reply to Cameron Kaiser from comment #113)
> Can you change the ifsef WTF_CPU_PPC to defined(WTF_CPU_PPC) &&
> !defined(JS_CPU_PPC_OSX) around cacheFlush?

Sure, no problem.

I've sent the current patch to try here : https://tbpl.mozilla.org/?tree=Try&rev=ec2fff3441e3
I'm not an expert for deciphering the logs, but it looks most of the failures there are either already known or due to infrastructure issues and unrelated to the diff itself. js shell builds on sparc64 too, firefox is still building on powerpc.... i'll post an updated patch (need to fix indentation in Makefile.in) and ask for review when i've tested firefox on ppc.
Comment 115 Landry Breuil (:gaston) 2012-05-02 02:10:50 PDT
Created attachment 620233 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

So with this smaller patch (and also patches from 750447, 750620 & 750853..) i have a working firefox (Mozilla/5.0 (X11; OpenBSD macppc; rv:15.0) Gecko/15.0 Firefox/15.0a1), browsed hg.m.o/w.m.o for a while, gmail, google maps...

So let's try to get this reviewed and hope this time it sticks. Only change from att 619876 is indentation fixed in Makefile.in (use tabs) and (defined(WTF_CPU_PPC) && !defined(JS_CPU_PPC_OSX)) for using the dummy cacheFlush.
Comment 116 Mike Hommey [:glandium] 2012-05-02 02:32:53 PDT
(In reply to Landry Breuil from comment #115)
> So let's try to get this reviewed and hope this time it sticks. Only change
> from att 619876 is indentation fixed in Makefile.in (use tabs) and
> (defined(WTF_CPU_PPC) && !defined(JS_CPU_PPC_OSX)) for using the dummy
> cacheFlush.

You should use something else than (defined(WTF_CPU_PPC) && !defined(JS_CPU_PPC_OSX)), otherwise, I'll have to file yet another bug for other platforms... I'm not convinced listing architectures is going to be any helpful there. I think what is important is whether there is a JIT or not. If there is no JIT, then the dummy cacheFlush works. If there is, then there should be an actual cacheFlush implementation for the given platform, or an explicit use of the dummy one like for x86/x64, which don't require cache flushing.
Comment 117 Landry Breuil (:gaston) 2012-05-02 03:05:50 PDT
So that'd mean replacing :

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

by

static void cacheFlush(void*, size_t)
{
#warning "Using dummy generic & empty cacheFlush for this platform."
}

Do you think that'll work out ? Since from my understanding of the previous comments cacheFlush() is only called if there's a JIT..
Comment 118 Mike Hommey [:glandium] 2012-05-02 03:09:41 PDT
(In reply to Landry Breuil from comment #117)
> So that'd mean replacing :
> 
> #error "The cacheFlush support is missing on this platform."
> 
> by
> 
> static void cacheFlush(void*, size_t)
> {
> #warning "Using dummy generic & empty cacheFlush for this platform."
> }
> 
> Do you think that'll work out ? Since from my understanding of the previous
> comments cacheFlush() is only called if there's a JIT..

That would work, but that would fail to ensure people adding a JIT for a new platform make sure cacheFlush is implemented correctly for that platform. (a warning is most probably not going to be seen)
Comment 119 Mike Hommey [:glandium] 2012-05-02 03:11:49 PDT
What might work, is to remove the error and put nothing to replace it. Adding a JIT for a new platform will make the function required, and compilation will fail because it is completely missing.
Comment 120 Landry Breuil (:gaston) 2012-05-02 06:05:55 PDT
Created attachment 620274 [details] [diff] [review]
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

That also works. New patch which only removes the #else #error from ExecutableAllocator.h, tested working here on ppc.
Comment 121 Cameron Kaiser [:spectre] 2012-05-02 09:02:18 PDT
I'm fine with comment 119, that shouldn't break me.
Comment 123 Ed Morley [:emorley] 2012-05-04 02:57:37 PDT
\o/

https://hg.mozilla.org/mozilla-central/rev/f5a3a7b9c6b0
Comment 124 Landry Breuil (:gaston) 2012-05-04 03:03:33 PDT
Finally \o/
Comment 125 Dan Horák 2012-05-04 03:22:46 PDT
Created attachment 621000 [details] [diff] [review]
the patch backported to xulrunner 12, no warranty

it could be useful for someone, I'm using it on Fedora/s390 to build the xulrunner version 12 package

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