Firefox/Thunderbird 10 FTBFS on powerpc in js/src/yarr/pcre

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.

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.

Chris, can you provide any hints for how to replace the JSArenaPool usage? Did it just move to a different file?

(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.

I think I can generate a patch (unless Landry does it first).

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?

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?

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?

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...)

Created attachment 584166
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

I'm good with that. We're using YARR JIT in PowerPC now anyway.

Comment on attachment 584166
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported

This patch is incomplete.

Created attachment 588391
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.

(In reply to Mike Hommey [:glandium] from comment #12)
> Created attachment 588391
> Use YARR interpreter instead of PCRE on platforms where YARR JIT is not
> supported
>
> This is the bug

Err. the patch, not the bug.

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.

(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 on attachment 588391
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.

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

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.

(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!

(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...

(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...

Hint: a lot of the !ENABLE_YARR_JIT uses really mean USE_PCRE.

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.

(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 .

Created attachment 589953
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...

That's why ENABLE_ASSEMBLER needs to be off.

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.

(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 ?

Yes, but for sparc, it's more subtle: ENABLE_ASSEMBLER needs to be set for the normal non YARR JIT. Man, what a mess.

(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.

(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.

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.

(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 ?

I've looked at try logs, and yes ENABLE_ASSEMBLER=1 is not set on the commandline when building under shell/ with the patch.

(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.

(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.

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.

(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 ?

(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.

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.

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.

(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...

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 ?

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.

(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

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 ?

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.

(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.

(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.

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).

s/architectures/operating systems/

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.

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!

(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.

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.

(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 !

Created attachment 619876
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.

Can you change the ifsef WTF_CPU_PPC to defined(WTF_CPU_PPC) && !defined(JS_CPU_PPC_OSX) around cacheFlush?

(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.

Created attachment 620233
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.

(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.

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

(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)

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.

Created attachment 620274
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.

I'm fine with comment 119, that shouldn't break me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a3a7b9c6b0

\o/

https://hg.mozilla.org/mozilla-central/rev/f5a3a7b9c6b0

Finally \o/

Created attachment 621000
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