1021754 - OdinMonkey: ARM Simulator crashes running the BananaBench demo.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

The ARM simulator crashes with a SIGSEGV around 400M instructions into the demo.

Bisected it down to the range 29ca8bc78484 (good) to 06428c4e45c0 (bad). If it's JIT related then this leaves bug 941805, bug 1015205, or bug 1016519 as regressing. Shall continue narrowing it down.

Comment 1

[Douglas Crosher [:dougc]]

A bisect shows this regression is caused or tickled by bug 941805, changeset https://hg.mozilla.org/mozilla-central/rev/37a64fc4edb3

Comment 2

[Brian Hackett [Laid off!]]

That rev only changes the behavior of web workers; are there web workers involved here?  One possibility is that bug 941805 removed limits on the scripts we will ion compile when running in workers, so could hit an existing issue, but having a stack for the crash would help.

Comment 3

[Douglas Crosher [:dougc]]

Attachment: Revert bug 941805.

Here's a patch to revert the regressing changeset from 941805. This fixes the demo shortly after bug 941805 landed, but the demo was again broken by bug 860736. The problem needs to be analysed, but this patch is at least useful for working on bug 860736.

Comment 4

[Douglas Crosher [:dougc]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> That rev only changes the behavior of web workers; are there web workers
> involved here?  One possibility is that bug 941805 removed limits on the
> scripts we will ion compile when running in workers, so could hit an
> existing issue, but having a stack for the crash would help.

Yes, I think it does use web workers. Applying just the removal of the limits on the scripts, the change in Ion.cpp, does not break the demo. A backtrace is below. Is it possible split up the changeset further to try and get some more clues?

#0  0xf7702430 in __kernel_vsyscall ()
#1  0xf7457056 in nanosleep () from /lib/libc.so.6
#2  0xf7456e2d in sleep () from /lib/libc.so.6
#3  0xf4206d43 in ah_crap_handler (signum=11) at toolkit/xre/nsSigHandlers.cpp:88
#4  0xf4212424 in nsProfileLock::FatalSignalHandler (signo=11, info=0xffdfee4c, context=0xffdfeecc) at profile/dirserviceprovider/src/nsProfileLock.cpp:185
#5  0xf48b4392 in AsmJSFaultHandler (signum=11, info=0xffdfee4c, context=0xffdfeecc) at js/src/jit/AsmJSSignalHandlers.cpp:966
#6  <signal handler called>
#7  0xf4ae13b8 in js::jit::Simulator::decodeSpecialCondition (this=this@entry=0xed897000, instr=instr@entry=0xcd9f756c) at js/src/jit/arm/Simulator-arm.cpp:3887
#8  0xf4b26397 in js::jit::Simulator::instructionDecode (this=this@entry=0xed897000, instr=instr@entry=0xcd9f756c) at js/src/jit/arm/Simulator-arm.cpp:4014
#9  0xf4b29634 in execute<false> (this=0xed897000) at js/src/jit/arm/Simulator-arm.cpp:4069
#10 js::jit::Simulator::callInternal (this=this@entry=0xed897000, entry=entry@entry=0xf2347870 "\360O-\351\r\200\240\341\214\317\005\343\207\315", <incomplete sequence \343>)
    at js/src/jit/arm/Simulator-arm.cpp:4151
#11 0xf4b29914 in js::jit::Simulator::call (this=<optimized out>, entry=entry@entry=0xf2347870 "\360O-\351\r\200\240\341\214\317\005\343\207\315", <incomplete sequence \343>, argument_count=<optimized out>, 
    argument_count@entry=8) at js/src/jit/arm/Simulator-arm.cpp:4232
#12 0xf4925116 in EnterBaseline (cx=cx@entry=0xde56de80, data=...) at js/src/jit/BaselineJIT.cpp:123
#13 0xf49b7ac5 in js::jit::EnterBaselineMethod (cx=cx@entry=0xde56de80, state=...) at js/src/jit/BaselineJIT.cpp:155
#14 0xf4cafd0b in js::RunScript (cx=cx@entry=0xde56de80, state=...) at js/src/vm/Interpreter.cpp:391
#15 0xf4cb010d in js::Invoke (cx=cx@entry=0xde56de80, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:474
#16 0xf4cb2a0d in js::Invoke (cx=cx@entry=0xde56de80, thisv=..., fval=..., argc=argc@entry=3, argv=argv@entry=0xebcff4cc, rval=rval@entry=...) at js/src/vm/Interpreter.cpp:511
#17 0xf48bfdd1 in js::InvokeFromAsmJS_ToInt32 (cx=0xde56de80, exitIndex=exitIndex@entry=69, argc=argc@entry=3, argv=argv@entry=0xebcff4cc) at js/src/jit/AsmJS.cpp:6246
#18 0xf4b27e4a in js::jit::Simulator::softwareInterrupt (this=0xed897000, instr=0xcb8556b4) at js/src/jit/arm/Simulator-arm.cpp:2164
#19 0xf4b28056 in js::jit::Simulator::decodeType7 (this=this@entry=0xed897000, instr=instr@entry=0xcb8556b4) at js/src/jit/arm/Simulator-arm.cpp:3180
#20 0xf4b26557 in js::jit::Simulator::instructionDecode (this=this@entry=0xed897000, instr=instr@entry=0xcb8556b4) at js/src/jit/arm/Simulator-arm.cpp:4037
#21 0xf4b29634 in execute<false> (this=0xed897000) at js/src/jit/arm/Simulator-arm.cpp:4069
#22 js::jit::Simulator::callInternal (this=this@entry=0xed897000, entry=entry@entry=0xc33a8920 "\360_-\351\020\213", <incomplete sequence \355>) at js/src/jit/arm/Simulator-arm.cpp:4151
#23 0xf4b29914 in js::jit::Simulator::call (this=<optimized out>, entry=entry@entry=0xc33a8920 "\360_-\351\020\213", <incomplete sequence \355>, argument_count=<optimized out>, argument_count@entry=2)
    at js/src/jit/arm/Simulator-arm.cpp:4232
#24 0xf48e3f79 in CallAsmJS (cx=0xde56de80, argc=2, vp=0xffe00248) at js/src/jit/AsmJSLink.cpp:515
#25 0xf4cb6feb in js::CallJSNative (cx=0xde56de80, native=0xf48e3b30 <CallAsmJS(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:239
#26 0xf4caff03 in js::Invoke (cx=0xde56de80, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:455
#27 0xf4b9307f in js_fun_apply (cx=0xde56de80, argc=2, vp=0xeb89c160) at js/src/jsfun.cpp:1139
#28 0xf4cb6feb in js::CallJSNative (cx=0xde56de80, native=0xf4b92c10 <js_fun_apply(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:239
#29 0xf4caff03 in js::Invoke (cx=0xde56de80, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:455
#30 0xf4ca9986 in Interpret (cx=cx@entry=0xde56de80, state=...) at js/src/vm/Interpreter.cpp:2566
#31 0xf4cafc72 in js::RunScript (cx=cx@entry=0xde56de80, state=...) at js/src/vm/Interpreter.cpp:402
#32 0xf4cb010d in js::Invoke (cx=cx@entry=0xde56de80, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:474
#33 0xf4cb2a0d in js::Invoke (cx=cx@entry=0xde56de80, thisv=..., fval=..., argc=0, argv=0xffe01408, rval=rval@entry=...) at js/src/vm/Interpreter.cpp:511
#34 0xf4af73d9 in JS::Call (cx=0xde56de80, thisv=..., fval=..., args=..., rval=...) at js/src/jsapi.cpp:5212
#35 0xf316767f in mozilla::dom::Function::Call (this=0xcb751180, cx=0xde56de80, aThisVal=..., arguments=..., aRv=...) at /home/src/b2g/build-firefox-i686-arm-sim/dom/bindings/FunctionBinding.cpp:35
#36 0xf36466bf in mozilla::dom::Function::Call<nsCOMPtr<nsISupports> > (this=0xcb751180, thisObjPtr=..., arguments=..., aRv=..., aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions)
    at ../../dist/include/mozilla/dom/FunctionBinding.h:58
#37 0xf3638df0 in nsGlobalWindow::RunTimeoutHandler (this=0xdfdc5e90, aTimeout=0xde54e6f0, aScx=0xdf74ebb0) at dom/base/nsGlobalWindow.cpp:12099
#38 0xf3639432 in nsGlobalWindow::RunTimeout (this=0xdfdc5e90, aTimeout=0xde54e6f0) at dom/base/nsGlobalWindow.cpp:12323
#39 0xf36397b5 in nsGlobalWindow::TimerCallback (aTimer=0xde54e740, aClosure=0xde54e6f0) at dom/base/nsGlobalWindow.cpp:12569
#40 0xf2825641 in nsTimerImpl::Fire (this=0xde54e740) at xpcom/threads/nsTimerImpl.cpp:609
#41 0xf2825805 in nsTimerEvent::Run (this=0xe9fde0f0) at xpcom/threads/nsTimerImpl.cpp:702
#42 0xf2822c2c in nsThread::ProcessNextEvent (this=0xeec06600, aMayWait=false, aResult=0xffe0190f) at xpcom/threads/nsThread.cpp:766
#43 0xf27b4e3b in NS_ProcessNextEvent (thread=<optimized out>, mayWait=false) at xpcom/glue/nsThreadUtils.cpp:263
#44 0xf2ac1fdf in mozilla::ipc::MessagePump::Run (this=0xf71b60a0, aDelegate=0xf71237c0) at ipc/glue/MessagePump.cpp:95
#45 0xf2a9ff70 in MessageLoop::RunInternal (this=0xf71237c0) at ipc/chromium/src/base/message_loop.cc:229
#46 0xf2a9ffa0 in RunHandler (this=0xf71237c0) at ipc/chromium/src/base/message_loop.cc:222
#47 MessageLoop::Run (this=0xf71237c0) at ipc/chromium/src/base/message_loop.cc:196

Comment 5

[Brian Hackett [Laid off!]]

(In reply to Douglas Crosher [:dougc] from comment #4)
> (In reply to Brian Hackett (:bhackett) from comment #2)
> > That rev only changes the behavior of web workers; are there web workers
> > involved here?  One possibility is that bug 941805 removed limits on the
> > scripts we will ion compile when running in workers, so could hit an
> > existing issue, but having a stack for the crash would help.
> 
> Yes, I think it does use web workers. Applying just the removal of the
> limits on the scripts, the change in Ion.cpp, does not break the demo. A
> backtrace is below. Is it possible split up the changeset further to try and
> get some more clues?

Well, you could try turning off the various helper thread features --- off thread Ion compilation, parsing, compression and GC sweeping --- and see if that changes anything.  But that stack is on the main thread, which bug 941805 doesn't affect at all.  And since the crash is while decoding an instruction then I would look into whether the jitcode has been swept or whether something or other on the stack has been corrupted.

Comment 6

[Douglas Crosher [:dougc]]

Attachment: Minimized patch that causes the crash.

This patch alone causes the crash. This is applied after applying the revert patch. The patch appears to just enable parallel compilation for web workers?

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Douglas Crosher [:dougc] from comment #6)
> Created attachment 8437535 [details] [diff] [review]
> Minimized patch that causes the crash.
> 
> This patch alone causes the crash. This is applied after applying the revert
> patch. The patch appears to just enable parallel compilation for web workers?

No, changing JS_NO_HELPER_THREADS to JS_USE_HELPER_THREADS will allow the worker to use helper threads for all tasks.  Try turning off javascript.options.ion.parallel_compilation (which just got renamed on trunk I guess) and javascript.options.parallel_parsing in the config.

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #7)
> (In reply to Douglas Crosher [:dougc] from comment #6)
> > Created attachment 8437535 [details] [diff] [review]
> > Minimized patch that causes the crash.
> > 
> > This patch alone causes the crash. This is applied after applying the revert
> > patch. The patch appears to just enable parallel compilation for web workers?
> 
> No, changing JS_NO_HELPER_THREADS to JS_USE_HELPER_THREADS will allow the
> worker to use helper threads for all tasks.  Try turning off
> javascript.options.ion.parallel_compilation (which just got renamed on trunk
> I guess) and javascript.options.parallel_parsing in the config.

Actually, I'm not sure that the DOM workers pay attention to these prefs.  You can also change the default values of parallelIonCompilationEnabled_ and parallelParsingEnabled_ in js/src/vm/Runtime.cpp

Comment 9

[Douglas Crosher [:dougc]]

Thank you. Caught a different crash with above minimum patch plus the following:
    parallelIonCompilationEnabled_(true)
    parallelParsingEnabled_(false)

* 31   Thread 0xed87eb40 (LWP 20665) "Analysis Helper" 0xf77a3430 in __kernel_vsyscall ()

#0  0xf77a3430 in __kernel_vsyscall ()
#1  0xf74f8056 in nanosleep () from /lib/libc.so.6
#2  0xf74f7e2d in sleep () from /lib/libc.so.6
#3  0xf42bb993 in ah_crap_handler (signum=6) at toolkit/xre/nsSigHandlers.cpp:88
#4  0xf42c7074 in nsProfileLock::FatalSignalHandler (signo=6, info=0xed87db8c, context=0xed87dc0c) at profile/dirserviceprovider/src/nsProfileLock.cpp:185
#5  <signal handler called>
#6  0xf77a3430 in __kernel_vsyscall ()
#7  0xf746d936 in raise () from /lib/libc.so.6
#8  0xf746f173 in abort () from /lib/libc.so.6
#9  0xf71c8b4c in PR_Assert (s=s@entry=0xf71efe5c "lock != NULL", file=file@entry=0xf71efb88 "nsprpub/pr/src/pthreads/ptsynch.c", ln=ln@entry=176)
    at nsprpub/pr/src/io/prlog.c:554
#10 0xf71de208 in PR_Lock (lock=0x0) at nsprpub/pr/src/pthreads/ptsynch.c:176
#11 0xf4be6602 in AutoLockSimulatorRuntime (srt=0xd83d8d00, this=<synthetic pointer>) at js/src/jit/arm/Simulator-arm.cpp:449
#12 js::jit::Redirection::Get (nativeFunction=0xf49c35e0 <AssumeUnreachable_(char const*)>, type=js::jit::Args_General1) at js/src/jit/arm/Simulator-arm.cpp:1242
#13 0xf4bccf07 in RedirectNativeFunction (type=<optimized out>, nativeFunction=0xf49c35e0 <AssumeUnreachable_(char const*)>) at js/src/jit/arm/Simulator-arm.cpp:1278
#14 js::jit::MacroAssemblerARMCompat::callWithABI (this=this@entry=0xb8016030, fun=fun@entry=0xf49c35e0 <AssumeUnreachable_(char const*)>, result=result@entry=js::jit::MoveOp::GENERAL)
    at js/src/jit/arm/MacroAssembler-arm.cpp:4010
#15 0xf4a15529 in callWithABINoProfiling<void*> (fun=<optimized out>, result=js::jit::MoveOp::GENERAL, this=0xb8016030) at js/src/jit/IonMacroAssembler.h:917
#16 js::jit::MacroAssembler::assumeUnreachable (this=0xb8016030, output=output@entry=0xf527f6de "Argument check fail.") at js/src/jit/IonMacroAssembler.cpp:1294
#17 0xf4a41188 in js::jit::CodeGenerator::generateArgumentsChecks (this=this@entry=0xb8016000, bailout=bailout@entry=false) at js/src/jit/CodeGenerator.cpp:2910
#18 0xf4a48553 in js::jit::CodeGenerator::generate (this=this@entry=0xb8016000) at js/src/jit/CodeGenerator.cpp:6658
#19 0xf4a4876d in js::jit::GenerateCode (mir=mir@entry=0xb80070f8, lir=0xbb305728) at js/src/jit/Ion.cpp:1689
#20 0xf4a82f5e in js::jit::CompileBackEnd (mir=0xb80070f8) at js/src/jit/Ion.cpp:1707
#21 0xf4d425dc in js::HelperThread::handleIonWorkload (this=this@entry=0xf7240d78) at js/src/vm/HelperThreads.cpp:844
#22 0xf4d43244 in js::HelperThread::threadLoop (this=0xf7240d78) at js/src/vm/HelperThreads.cpp:1087
#23 0xf71e5e86 in _pt_root (arg=0xeed5e7c0) at nsprpub/pr/src/pthreads/ptthread.c:212
#24 0xf77549da in start_thread () from /lib/libpthread.so.0
#25 0xf7536bfe in clone () from /lib/libc.so.6

It appears that the SimulatorRuntime init() method has not been called. It is called via CreateSimulatorRuntime() which is called from JSRuntime::init().

Not sure if this is a valid issue given all the hacks. Can the compiler be called without a JSRuntime now?

Might a change to the simulator initialization path be needed?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Douglas Crosher [:dougc] from comment #9)
> It appears that the SimulatorRuntime init() method has not been called. It
> is called via CreateSimulatorRuntime() which is called from
> JSRuntime::init().
> 
> Not sure if this is a valid issue given all the hacks. Can the compiler be
> called without a JSRuntime now?
> 
> Might a change to the simulator initialization path be needed?

It appears you have a non-null SimulatorRuntime, but SimulatorRuntime::lock_ is still nullptr, is that right? That makes no sense; we always call init() when we create a SimulatorRuntime. Maybe you could add some logging to trace which SimulatorRuntime* pointers we have for which JSRuntime*, and what their lock value is? Then when we crash you can look at the SimulatorRuntime* and see if it's valid (is in the log).

Comment 11

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> (In reply to Douglas Crosher [:dougc] from comment #9)
> > It appears that the SimulatorRuntime init() method has not been called. It
> > is called via CreateSimulatorRuntime() which is called from
> > JSRuntime::init().
> > 
> > Not sure if this is a valid issue given all the hacks. Can the compiler be
> > called without a JSRuntime now?
> > 
> > Might a change to the simulator initialization path be needed?
> 
> It appears you have a non-null SimulatorRuntime, but SimulatorRuntime::lock_
> is still nullptr, is that right? That makes no sense; we always call init()
> when we create a SimulatorRuntime. Maybe you could add some logging to trace
> which SimulatorRuntime* pointers we have for which JSRuntime*, and what
> their lock value is? Then when we crash you can look at the
> SimulatorRuntime* and see if it's valid (is in the log).

Thank you, that appears correct. Unfortunately it is not reproducible.

Disabling helper threads for the asm.js compilation does not help, it still crashes.

Setting OffThreadCompilationAvailable() to false avoids the crash.

Tried another asm.js demo and it crashes before finishing compilation. The backout above does not resolve it so it might be a different issue. But it does appear to be a crash in a helper thread, and it appears related to the runtime, so might it be a clue?

Assertion failure: CurrentThreadCanAccessRuntime(rt), at js/src/jsgc.cpp:2152

#0  0xf776e430 in __kernel_vsyscall ()
#1  0xf74c3056 in nanosleep () from /lib/libc.so.6
#2  0xf74c2e2d in sleep () from /lib/libc.so.6
#3  0xf427d81b in ah_crap_handler (signum=11) at toolkit/xre/nsSigHandlers.cpp:88
#4  0xf4288efc in nsProfileLock::FatalSignalHandler (signo=11, info=0xedab3d0c, context=0xedab3d8c) at profile/dirserviceprovider/src/nsProfileLock.cpp:185
#5  0xf491d292 in AsmJSFaultHandler (signum=11, info=0xedab3d0c, context=0xedab3d8c) at js/src/jit/AsmJSSignalHandlers.cpp:966
#6  <signal handler called>
#7  0xf4bcc69b in js::gc::GCRuntime::triggerGC (this=0xed9751cc, reason=JS::gcreason::TOO_MUCH_MALLOC) at js/src/jsgc.cpp:2152
#8  0xf4bcc6f4 in js::gc::GCRuntime::onTooMuchMalloc (this=0xed9751cc) at js/src/jsgc.cpp:1525
#9  0xf4bcc754 in js::gc::GCRuntime::updateMallocCounter (this=<optimized out>, zone=0xd9fb1c00, nbytes=147824884) at js/src/jsgc.cpp:1516
#10 0xf4d3c3e4 in JSRuntime::updateMallocCounter (this=<optimized out>, zone=0xd9fb1c00, nbytes=nbytes@entry=147824884) at js/src/vm/Runtime.cpp:709
#11 0xf4c62eb7 in updateMallocCounter (this=0xd879c040, this=0xd879c040, nbytes=147824884) at js/src/jscntxt.h:274
#12 malloc_ (bytes=147824884, this=0xd879c040) at js/src/vm/MallocProvider.h:52
#13 js::ScriptSource::ensureOwnsSource (this=0xd5861f80, cx=0xd879c040) at js/src/jsscript.cpp:1582
#14 0xf4c63025 in js::ScriptSource::setSourceCopy (this=<optimized out>, this@entry=0xd5861f80, cx=<optimized out>, cx@entry=0xd879c040, srcBuf=..., argumentsNotIncluded=argumentsNotIncluded@entry=false, 
    task=task@entry=0xedab41d4) at js/src/jsscript.cpp:1636
#15 0xf489d033 in js::frontend::CompileScript (cx=0xd879c040, alloc=alloc@entry=0xdacec584, scopeChain=..., evalCaller=..., options=..., srcBuf=..., source_=source_@entry=0x0, staticLevel=staticLevel@entry=0, 
    extraSct=extraSct@entry=0x0) at js/src/frontend/BytecodeCompiler.cpp:256
#16 0xf4cec71a in js::HelperThread::handleParseWorkload (this=this@entry=0xf7240c00) at js/src/vm/HelperThreads.cpp:907
#17 0xf4cecb10 in js::HelperThread::threadLoop (this=0xf7240c00) at js/src/vm/HelperThreads.cpp:1089
#18 0xf71e5e86 in _pt_root (arg=0xeed55640) at nsprpub/pr/src/pthreads/ptthread.c:212
#19 0xf771f9da in start_thread () from /lib/libpthread.so.0
#20 0xf7501bfe in clone () from /lib/libc.so.6

Comment 12

[Brian Hackett [Laid off!]]

That assertion is, yeah, a different issue (bug 1023035).

Comment 13

[Douglas Crosher [:dougc]]

Attachment: Avoid creating a Simulator for helper threads and reference the SimulatorRuntime via the PerThreadData.

One issue here is that the simulator creates Redirection trampolines between the simulated code and the native code, and these are specific to the runtime and are destroyed when the runtime is destroyed. The SimulatorRuntime is stored in the JSRuntime, and the simulator stores it's own reference to the SimulatorRuntime and this is getting out of sync for the helper threads. This patch gets the BananaBench running, and I will do some more testing.

Can it be assumed that a helper thread will never need to execute simulated code?  The patch asserts this.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8439026 [details] [diff] [review]
Avoid creating a Simulator for helper threads and reference the SimulatorRuntime via the PerThreadData.

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

Good catch!

(In reply to Douglas Crosher [:dougc] from comment #13)
> Can it be assumed that a helper thread will never need to execute simulated
> code?

Yes that's correct.

::: js/src/jit/arm/Simulator-arm.cpp
@@ +1193,5 @@
>          swiInstruction_(Assembler::AL | (0xf * (1 << 24)) | kCallRtRedirected),
>          type_(type),
>          next_(nullptr)
>      {
> +        SimulatorRuntime *srt = TlsPerThreadData.get()->simulatorRuntime();

Pre-existing, but you could give this Redirection constructor a SimulatorRuntime *srt argument (the caller also does the lookup), so that you don't need this TLS access.

Comment 15

[Douglas Crosher [:dougc]]

Attachment: Avoid creating a Simulator for helper threads and reference the SimulatorRuntime via the PerThreadData.

Thank you for the feedback.

Comment 16

[Douglas Crosher [:dougc]]

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

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8439190 [details] [diff] [review]
Avoid creating a Simulator for helper threads and reference the SimulatorRuntime via the PerThreadData.

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

::: js/src/jit/arm/Simulator-arm.cpp
@@ +1205,5 @@
>      void *nativeFunction() const { return nativeFunction_; }
>      ABIFunctionType type() const { return type_; }
>  
>      static Redirection *Get(void *nativeFunction, ABIFunctionType type) {
> +        SimulatorRuntime *srt = TlsPerThreadData.get()->simulatorRuntime();

Nit: add a PerThreadData *pt = TlsPerThreadData.get(); before this line.

@@ +1211,5 @@
> +
> +#ifdef DEBUG
> +        Simulator *sim = TlsPerThreadData.get()->simulator();
> +        JS_ASSERT_IF(sim, sim->srt_ == srt);
> +#endif

... and replace this #if...#endif with:

JS_ASSERT_IF(pt->simulator(), pt->simulator()->srt_ == srt);

Same for the MIPS version.

::: js/src/vm/Runtime.h
@@ +612,5 @@
>          {
>              JS_ASSERT(!pt->runtime_);
>              pt->runtime_ = rt;
> +#if defined(JS_ARM_SIMULATOR) || defined(JS_MIPS_SIMULATOR)
> +            JS_ASSERT(!pt->simulator_);

Nit: add a brief explanation, something like this:

// The simulator has a pointer to its SimulatorRuntime, but helper
// threads don't have a simulator as they don't run JIT code.
JS_ASSERT(!pt->simulator_);

Comment 18

[Douglas Crosher [:dougc]]

Attachment: Avoid creating a Simulator for helper threads and reference the SimulatorRuntime via the PerThreadData.

Address reviewer feedback, thank you. Carry forward r+.

Comment 19

[Douglas Crosher [:dougc]]

This try run just tests that the ARM builds are not broken by the patch. Not sure how to invoke an ARM-simulator try test. I've tested the ARM simulator locally for the past week and it fixes the reported bug.

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

Comment 20

[Douglas Crosher [:dougc]]

Comment on attachment 8440615 [details] [diff] [review]
Avoid creating a Simulator for helper threads and reference the SimulatorRuntime via the PerThreadData.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 941805.

User impact if declined: None, it only affects the ARM simulator, but this is very useful for testing.

Testing completed (on m-c, etc.): Locally.

Risk to taking this patch (and alternatives if risky): Very low.

String or IDL/UUID changes made by this patch: n/a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5628e4770c96