890143 - Profiler debug assertion: idx >= 0 && uint32_t(idx) < script()->length, at SPSProfiler.cpp:267

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I can reliably reproduce this by starting a debug build with profiling enabled.  The failure is:

F/MOZ_Assert(17795): Assertion failure: idx >= 0 && uint32_t(idx) < script()->length, at /home/jld/src/B2G/gecko/js/src/vm/SPSProfiler.cpp:267

Apparently caused by (in a profiler sample):

(gdb) p aStack->mStack[i].idx
$12 = 1024
(gdb) p aStack->mStack[i].script_->length
$15 = 169

If I understand correctly, this has to be getting written in by jitcode, because the only path to write it from C++ has an equivalent assertion.  We presumably need more assertions to track down the bad index at its source.  (This might be ARM-specific, though I haven't tried it with an x86 build yet, nor do I know if our regular testing would cover this on other platforms.)

In addition to making the Gecko profiler unusable for investigating debug build overhead, which is something that may need to happen on b2g, this also makes it somewhat difficult to do development on the profiling infrastructure.

Comment 1

[Benoit Girard (:BenWa)]

This error can be worked around by disabling the 'js' feature. Of course this isn't something we can do long term. We need someone from js to look at this.

Comment 3

[Julian Seward [:jseward]]

I also saw this, on x86_64-linux.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jed Davis [:jld] from comment #0)
> I can reliably reproduce this by starting a debug build with profiling
> enabled.

Does this happen with the js shell or do we need to build the whole browser? Doesn't seem to reproduce on x86_64-linux with the js shell only (built with --enable-profiling).

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I tried running a simple program in the JS shell, on b2g, on a debug+profiling build, and it didn't fail.  But, doesn't the jitcode profiling instrumentation need a dynamic flag to be set, as well as --enable-profiling?

Also, if it helps, I've pastebinned the stack of a failure: http://pastebin.mozilla.org/2891906 .  These are the lines with the bad bytecode index (accompanied by their indices within the stack array):

10:  {{string = 0x474d7d00 "csp_toString (resource://gre/modules/CSPUtils.jsm:765)", sp = 0x0, script_ = 0x44a4ae00, idx = 1024}},
11:  {{string = 0x4715cba0 "CSPSourceList.prototype.toString (resource://gre/modules/CSPUtils.jsm:1134)", sp = 0x0, script_ = 0x44a4f400, idx = 1024}},
15:  {{string = 0x4715cc40 "CSPSource.prototype.toString (resource://gre/modules/CSPUtils.jsm:1593)", sp = 0x0, script_ = 0x44a4fc00, idx = 1024}},
16:  {{string = 0x4715cc40 "CSPSource.prototype.toString (resource://gre/modules/CSPUtils.jsm:1593)", sp = 0x0, script_ = 0x44a4fc00, idx = 1024}},

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug890143-workaround.diff

This patch prevents the assertion failure for me, by not setting the pseudo-stack index from a jitcode register if it's equal to 1024.  I therefore conclude that the bad values are coming in through MacroAssembler::spsUpdatePCIdx(SPSProfiler*, Register, Register), which seems to be used only in BaselineIC.cpp to copy values from the pcOffset_ fields of the various kinds of baseline frames.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I think I know what's going on here: the register holding the bytecode index is the same as the ScratchRegister that spsProfileEntryAddressSafe clobbers when it does branch32 with an AbsoluteAddress.

So, the mysterious 1024 is the number of elements in the pseudostack.

It makes sense that this would also affect x86_64 (but not 32-bit x86), because that architecture also can't do arithmetic with a memory operand at an arbitrary absolute address without using a temporary.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: BaselineSecondScratchReg is not available in Baseline mode.

This should take care of things for ARM.  It looks like profiling is only relevant in that nothing in BaselineIC happens to get r6 and use it across a conflicting operation otherwise (or maybe it does but we don't notice, which is a scary thought).

I don't have an answer for x86_64 yet.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jed Davis [:jld] from comment #8)
> I don't have an answer for x86_64 yet.

I've reproduced a crash on x86_64 Linux, but it's a different assertion: in SPSInstrumentation::leave, computing the offset for the immediate version of spsUpdatePCIdx, getting a bytecode pointer that's obviously out of range for the script it's supposed to be inside.  I'll file a separate bug.

I'm currently thinking that my previous guess was wrong, and this bug is in fact ARM-backend-specific.

Comment 10

[Kannan Vijayan [:djvj]]

Comment on attachment 793796 [details] [diff] [review]
BaselineSecondScratchReg is not available in Baseline mode.

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

Nice find.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Also, for whatever it's worth at this point, I did some try builds last week:
https://tbpl.mozilla.org/?tree=Try&rev=2eb3fba9017a
https://tbpl.mozilla.org/?tree=Try&rev=9783bb53b9d2

(Built on all platforms, ran tests on ARM.)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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