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

[Micah Gersten]

Debian has the fix, so I'll add it to the branch and it should work with the next upload.

[Landry-openbsd (landry-openbsd)]

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.

[Christopher-leary (christopher-leary)]

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.

[Dmandelin (dmandelin)]

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

[Christopher-leary (christopher-leary)]

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

[Spectre (spectre)]

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

[Spectre (spectre)]

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?

[Jndiogo (jndiogo)]

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?

[Spectre (spectre)]

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?

[Mh+mozilla (mh+mozilla)]

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

[Mh+mozilla (mh+mozilla)]

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

[Spectre (spectre)]

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

[Mh+mozilla (mh+mozilla)]

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

This patch is incomplete.

[Micah Gersten (micahg)]

Setting back to triaged, Debian's patch is incomplete for the moment

[Mh+mozilla (mh+mozilla)]

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.

[Mh+mozilla (mh+mozilla)]

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

[Landry-openbsd (landry-openbsd)]

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.

[Mh+mozilla (mh+mozilla)]

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

[Landry-openbsd (landry-openbsd)]

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.

[Ben Collins (ben-collins)]

I've tested the patch on #691898. Built and ran firefox on my powerpc and all worked as expected. Can we please get an upload on this soonish? I could do it, but don't want to step on any toes.

[Chris Coulson (chrisccoulson)]

We might be able to get the patch in for the next beta release (towards the end of the week), depending on how much non-PPC specific code it touches. In any case, we don't normally do out-of-band uploads, especially ones which have only PPC fixes, as they're basically a no-change rebuild for all of the other architectures (which means an armel builder tied up for an entire day for no reason, plus retriggering a reupload of our build symbols).

This is becoming a pain though, as powerpc is breaking for every 6-week Firefox release, and we're having to land patches late in the beta cycle to fix it (we're only 2 weeks away from the Firefox 1

[Chris Coulson (chrisccoulson)]

We might be able to get the patch in for the next beta release (towards the end of the week), depending on how much non-PPC specific code it touches. In any case, we don't normally do out-of-band uploads, especially ones which have only PPC fixes, as they're basically a no-change rebuild for all of the other architectures (which means an armel builder tied up for an entire day for no reason, plus retriggering a reupload of our build symbols).

This is becoming a pain though, as powerpc is breaking for every 6-week Firefox release and we're having to land patches late in the beta cycle to fix it (we're only 2 weeks away from the Firefox 10 release, which isn't a great time to be landing any code changes at all). IMO we're seriously going to have to consider skipping powerpc with some Firefox releases, rather than compromising the stability and quality of our other architectures.

[Chris Coulson (chrisccoulson)]

Sorry, I submitted to early

[Ben Collins (ben-collins)]

I'm willing to do pre-builds before uploads in order to validate powerpc if that will help. Takes about 6 hours to do a build+test. I'd rather not kill powerpc sync on a main package.

[Micah Gersten (micahg)]

I haven't committed the fix for this yet as I was waiting on upstream trying to get a solution that worked on all branches.

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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.

[Christopher-leary (christopher-leary)]

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

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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

[Mh+mozilla (mh+mozilla)]

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

[Spectre (spectre)]

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.

[Spectre (spectre)]

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

[Landry-openbsd (landry-openbsd)]

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

[Mh+mozilla (mh+mozilla)]

That's why ENABLE_ASSEMBLER needs to be off.

[Spectre (spectre)]

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.

[Launchpad Janitor (janitor)]

This bug was fixed in the package firefox - 10.0~b5+build1-0ubuntu1

---------------
firefox (10.0~b5+build1-0ubuntu1) precise; urgency=low

  * New upstream release from the beta channel (FIREFOX_10_0b5_BUILD1)

  [ Micah Gersten <email address hidden> ]
  * Fix LP: #917529 - Make sure new transitional packages have a versioned
    dependency on Firefox so as to not break Firefox during partial upgrades
    - update debian/control{,.in}

  [ Chris Coulson <email address hidden> ]
  * Make it possible to have per-arch patches
    - update debian/build/enable-dist-patches.pl
    - update debian/rules
    - update debian/patches/series
  * Drop the solid white separators from the addressbar autocomplete dropdown,
    and increase padding so that it doesn't look so bad with dark themes
    - add debian/patches/autocomplete-theme-tweak.patch
    - update debian/patches/series
  * Fix LP: #908508 - Add patch from upstream to fix powerpc build failure.
    Only apply this patch on powerpc to avoid compromising the quality of
    the architectures that we care about
    - add debian/patches/fix-build-failure-without-yarr-jit2.patch
    - update debian/patches/series
  * Also make the previous powerpc build fix apply on ppc only
    - update debian/patches/series
 -- Chris Coulson <email address hidden> Thu, 19 Jan 2012 21:33:22 +0000

[Launchpad Janitor (janitor)]

This bug was fixed in the package thunderbird - 10.0~b4+build1-0ubuntu1

---------------
thunderbird (10.0~b4+build1-0ubuntu1) precise; urgency=low

  * New upstream release from the beta channel (THUNDERBIRD_10_0b4_BUILD1)

  * Update messagingmenu to 0.9.1
    - Fixes LP: #917920 - Messaging menu entries untranslated for Slovak
    - Fixes LP: #917154 - Add Japanese translations
    - Add Spanish translations
    - Don't spawn a new Thunderbird instance when being activated from the
      messaging menu (thus relying on the remoting logic). Instead, use
      the nsIWindowMediator and nsIWindowWatcher interfaces directly
    - Don't keep the main window in the scope of our shared JS module, as
      we're probably going to leak it. Instead, use nsIWindowMediator to
      find the main window when we need it (eg, to switch tabs). Also, use
      nsITimer rather than window.setTimeout().
  * Make it possible to do per-arch patches too
    - update debian/build/enable-dist-patches.pl
    - update debian/rules
    - update debian/patches/series
  * Fix LP: #908508 - Add patch from upstream to fix powerpc build failure.
    Only apply this patch on powerpc to avoid compromising the quality of
    the architectures that we care about
    - add debian/patches/fix-build-failure-without-yarr-jit2.patch
    - update debian/patches/series
  * Also make the previous powerpc build fix apply on ppc only
    - update debian/patches/series
 -- Chris Coulson <email address hidden> Thu, 19 Jan 2012 22:42:09 +0000

[Landry-openbsd (landry-openbsd)]

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

[Mh+mozilla (mh+mozilla)]

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

[Christopher-leary (christopher-leary)]

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

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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.

[Landry-openbsd (landry-openbsd)]

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

[Acatel (acatel)]

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?

[Landry-openbsd (landry-openbsd)]

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

[Acatel (acatel)]

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.

[Landry-openbsd (landry-openbsd)]

Comment on attachment 594798
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 ?

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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 ?

[Landry-openbsd (landry-openbsd)]

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 ?

[Christopher-leary (christopher-leary)]

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

[Landry-openbsd (landry-openbsd)]

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

[Christopher-leary (christopher-leary)]

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

[Emmanuel Ch. (eruyome)]

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.

[Mh+mozilla (mh+mozilla)]

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.

[Landry-openbsd (landry-openbsd)]

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

[Mh+mozilla (mh+mozilla)]

> 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

[Emmanuel Ch. (eruyome)]

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?

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

Most likely fallout of #717540, maybe because of the support (supposedly, yes) of __attribute__((weak)) in our gcc..

Emmanueel, what's your compiler ?

[Mh+mozilla (mh+mozilla)]

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.

[Landry-openbsd (landry-openbsd)]

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.

[Landry-openbsd (landry-openbsd)]

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

[Emmanuel Ch. (eruyome)]

Sorry for the delay,

I have gcc 4.5.3 on a gentoo ppc64 system.

Patch on Comment 55 worked for me. Built successfully.

[Landry-openbsd (landry-openbsd)]

(In reply to Landry Breuil from comment #55)
> Created attachment 600506
> 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.

[Christopher-leary (christopher-leary)]

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

[Landry-openbsd (landry-openbsd)]

Review ping ? :)

[Dmandelin (dmandelin)]

Comment on attachment 600506
Use YARR interpreter instead of PCRE on platforms where YARR JIT is not supported (m-c patch)

Review of attachment 600506:
-----------------------------------------------------------------

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.

[Spectre (spectre)]

Let's land this first, since it fixes arches that are actually broken. I'll rebase bug 731110 on top.

[Mh+mozilla (mh+mozilla)]

Created attachment 603183
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?

[Landry-openbsd (landry-openbsd)]

patch #603183 reads good, applies fine on m-c. Doing a testbuild before setting checkin-needed, results in ~6h.

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

Set checkin-needed, carrying forward dmandelin's r+ on attachement 600506.

[Dao (dao)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d68420c97e0e

[Mak77 (mak77)]

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

[Mh+mozilla (mh+mozilla)]

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

[Mak77 (mak77)]

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

[Landry-openbsd (landry-openbsd)]

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.

[Mh+mozilla (mh+mozilla)]

Well, if it's breaking linux debug builds, it means it's *not* only shuffling ifdefs around. So you should start by checking that.

[Dmandelin (dmandelin)]

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

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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

[Dvander (dvander)]

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

[Landry-openbsd (landry-openbsd)]

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 ?

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

Try build still fails the same on debug builds only.

[Landry-openbsd (landry-openbsd)]

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.

[Spectre (spectre)]

-m = Methodjit
-n = type infereNce
-a = Always run in jit

[Spectre (spectre)]

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?

[Dmandelin (dmandelin)]

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.

[Spectre (spectre)]

He'll still need it for sparc64, though (and the PPC assembler is only for OS X ABI, and *BSD/ppc is SysV ABI).

[Spectre (spectre)]

(er, unless someone(tm) adds the necessary ABI sauce to it, which is just a matter of doing it(tm))

[Landry-openbsd (landry-openbsd)]

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

[Mh+mozilla (mh+mozilla)]

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

[Mh+mozilla (mh+mozilla)]

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.

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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

[Mh+mozilla (mh+mozilla)]

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

[Dmandelin (dmandelin)]

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

[Landry-openbsd (landry-openbsd)]

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.

[Landry-openbsd (landry-openbsd)]

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

[Mh+mozilla (mh+mozilla)]

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

[Dmandelin (dmandelin)]

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.

[SharkCZ (dan-danny)]

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.

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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 ?

[Dmandelin (dmandelin)]

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.

[Landry-openbsd (landry-openbsd)]

(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

[Landry-openbsd (landry-openbsd)]

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 ?

[Dmandelin (dmandelin)]

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.

[Landry-openbsd (landry-openbsd)]

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

[Dmandelin (dmandelin)]

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

[Spectre (spectre)]

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

[Spectre (spectre)]

s/architectures/operating systems/

[Mh+mozilla (mh+mozilla)]

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.

[Ifinisheri (ifinisheri)]

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!

[Anarchy (anarchy)]

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

[Spectre (spectre)]

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.

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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.

[Spectre (spectre)]

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

[Landry-openbsd (landry-openbsd)]

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

[Landry-openbsd (landry-openbsd)]

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.

[Mh+mozilla (mh+mozilla)]

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

[Landry-openbsd (landry-openbsd)]

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

[Mh+mozilla (mh+mozilla)]

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

[Mh+mozilla (mh+mozilla)]

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.

[Landry-openbsd (landry-openbsd)]

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.

[Spectre (spectre)]

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

[Mh+mozilla (mh+mozilla)]

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

[Bmo-edmorley (bmo-edmorley)]

\o/

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

[Landry-openbsd (landry-openbsd)]

Finally \o/

[SharkCZ (dan-danny)]

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