602333 - Crash in [@ JSC::X86Assembler::setRel32]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Recently we've seen a number of crashes in the media mochitests in content/media/test which have been in JSC::X86Assembler::setRel32.

Examples:

Bug 597050, crash in content/media/test/test_new_audio.html
Rev3 Fedora 12x64 mozilla-central debug test mochitests-1/5 on 2010/09/18 14:15:30
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284844530.1284846996.15464.gz#err5

ASSERTION FAILED: offset == static_cast<int32_t>(offset)
(/builds/slave/mozilla-central-linux64-debug/build/js/src/assembler/assembler/X86Assembler.h:2251 static void JSC::X86Assembler::setRel32(void*, void*))


Thread 0 (crashed)
 0  libxul.so!JSC::X86Assembler::setRel32 [X86Assembler.h:6066d938a0dd : 2251 + 0x30]
    rbx = 0x00000028   r12 = 0x0438adb0   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0xf0119b60   rip = 0x1de416f8   rsp = 0xdc7a1e30   rbp = 0xdc7a1e50
 1  libxul.so!JSC::X86Assembler::linkJump [X86Assembler.h:6066d938a0dd : 2116 + 0x15]
    rbx = 0x00000028   r12 = 0x0438adb0   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0xf0119b60   rip = 0x1de613f9   rsp = 0xdc7a1e60   rbp = 0xdc7a1e80
 2  libxul.so!JSC::AbstractMacroAssembler<JSC::X86Assembler>::linkJump [AbstractMacroAssembler.h:6066d938a0dd : 518 + 0x17]
    rbx = 0x00000028   r12 = 0x0438adb0   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0xf0119b60   rip = 0x1de61426   rsp = 0xdc7a1e90   rbp = 0xdc7a1eb0
 3  libxul.so!JSC::LinkBuffer::link [LinkBuffer.h:6066d938a0dd : 103 + 0x17]
    rbx = 0x00000028   r12 = 0x0438adb0   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0xf0119b60   rip = 0x1de61453   rsp = 0xdc7a1ec0   rbp = 0xdc7a1ee0
 4  libxul.so!GetPropCompiler::generateStub [PolyIC.cpp:6066d938a0dd : 1169 + 0x20]
    rbx = 0x00000028   r12 = 0x0438adb0   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0xf0119b60   rip = 0x1de6a365   rsp = 0xdc7a1ef0   rbp = 0xdc7a25f0
 5  libxul.so!GetPropCompiler::update [PolyIC.cpp:6066d938a0dd : 1248 + 0x10]
    rbx = 0x031ff508   r12 = 0xf0132620   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0xf0119b60   rip = 0x1de6c55d   rsp = 0xdc7a2600   rbp = 0xdc7a2650
 6  libxul.so!js::mjit::ic::CallProp [PolyIC.cpp:6066d938a0dd : 2306 + 0xb]
    rbx = 0x031ff508   r12 = 0xf0132620   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0xf0119b60   rip = 0x1de65689   rsp = 0xdc7a2660   rbp = 0xdc7a27c0
 7  0x7f4b1f7e8722
    rbx = 0x031ff508   r12 = 0xf0132620   r13 = 0x00000000   r14 = 0xffffffff
    r15 = 0xf0119b60   rip = 0x1f7e8723   rsp = 0xdc7a27d0   rbp = 0xdc7a2850

Examples from bug 599647:
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1286389121.1286391362.22197.gz
Rev3 Fedora 12x64 tracemonkey debug test mochitests-1/5 on 2010/10/06 11:18:41

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1285874389.1285876673.27153.gz
Rev3 Fedora 12x64 tracemonkey debug test mochitests-1/5 on 2010/09/30 12:19:49

I've only seen this on x64 Linux so far.

Comment 1

[Sean Stangl [:sstangl]]

A relative jump from the inline buffer low in the virtual address space to a PIC high in the address space is not possible on x64. However, we don't currently detect this occurrence. This is the most likely cause.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

(Weird, why doesn't it print 64-bit values for those registers?)

Would be nice to figure out how to stress test and reproduce this.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch to stress test media mochitest

(In reply to comment #2)
> Would be nice to figure out how to stress test and reproduce this.

Here's how I do it.

If you apply this patch, you'll change most of the media mochitests so that they individually loop forever. If you then run the media mochitest which this crash shows up on (content/media/test/test_new_audio.html), the test will loop until you see the crash. You'll probably want to run the test concurrently in multiple tabs, and you'll want to change the mochitest --timeout command line option, else it will be killed after 3 minutes or so. The command line I use for mochitest is this:

(from $objdir/_tests/testing/)
python mochitest/runtests.py --appname=$objdir/dist/bin/firefox --test-path=$srcdir/content/media/test/test_new_audio.html --timeout=100000 --autorun --console-level=DEBUG --log-file=$srcdir/mochitest.log --file-level=INFO

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #3)
> Created attachment 481398 [details] [diff] [review]
> Patch to stress test media mochitest
> 
> (In reply to comment #2)
> > Would be nice to figure out how to stress test and reproduce this.
> 
> Here's how I do it.

In fact I just updated my tree, and I can now reproduce the crash this way. I opened test_new_audio in about 25 tabs, and I hit this crash in under 2 minutes.

Comment 5

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1286421348.1286423056.11488.gz#err1
Rev3 Fedora 12x64 mozilla-central debug test mochitests-1/5 on 2010/10/06 20:15:48
s: talos-r3-fed64-048

Comment 6

[Daniel Holbert [:dholbert]]

Bug 599645 is tracking this assertion-failure during test_media_selection.html (and now test_seek_out_of_range.html, when I noted the log from this bug's comment 5 there just now).

It looks like this is all the same underlying bug -- I'm just mentioning it here since that one has a number of logs already posted.

Comment 7

[Jeff Walden [:Waldo]]

O frabjous day!

For you, at least, if not for me.  My month-plus neglected Firefox tab set reliably triggers this with the mozilla-central 4788083ce564 revision (not *just* that revision, just being precise), and I have it in a debugger.

#5  0x00007f3c17360937 in JSC::X86Assembler::setRel32 (from=0x7f3b9744a475, to=0x7f3c187f90af)
    at /home/jwalden/moz/2/js/src/assembler/assembler/X86Assembler.h:2253
2250	    static void setRel32(void* from, void* to)
2251	    {
2252	        intptr_t offset = reinterpret_cast<intptr_t>(to) - reinterpret_cast<intptr_t>(from);
2253	        ASSERT(offset == static_cast<int32_t>(offset));
2254	
2255	        js::JaegerSpew(js::JSpew_Insns,
2256	                       ISPFX "##setRel32 ((from=%p)) ((to=%p))\n", from, to);
2257	        setInt32(from, offset);

(gdb) p/x offset
$3 = 0x813aec3a
(gdb) bt
#4  <signal handler called>
#5  0x00007f3c17360937 in JSC::X86Assembler::setRel32 (from=0x7f3b9744a475, to=0x7f3c187f90af)
    at /home/jwalden/moz/2/js/src/assembler/assembler/X86Assembler.h:2253
#6  0x00007f3c17388b4b in JSC::X86Assembler::linkJump (code=0x7f3b9744a468, from=..., to=0x7f3c187f90af)
    at /home/jwalden/moz/2/js/src/assembler/assembler/X86Assembler.h:2117
#7  0x00007f3c17388c33 in JSC::AbstractMacroAssembler<JSC::X86Assembler>::linkJump (code=0x7f3b9744a468, jump=..., target=...)
    at /home/jwalden/moz/2/js/src/assembler/assembler/AbstractMacroAssembler.h:523
#8  0x00007f3c17388b7e in JSC::LinkBuffer::link (this=0x7fffa422a1a0, jump=..., label=...)
    at /home/jwalden/moz/2/js/src/assembler/assembler/LinkBuffer.h:113
#9  0x00007f3c17396715 in GetPropCompiler::generateStub (this=0x7fffa422a3e0, holder=0x7f3c04b69e38, shape=0x7f3c022b8660)
    at /home/jwalden/moz/2/js/src/methodjit/PolyIC.cpp:1120
#10 0x00007f3c17396c5e in GetPropCompiler::update (this=0x7fffa422a3e0) at /home/jwalden/moz/2/js/src/methodjit/PolyIC.cpp:1201
#11 0x00007f3c17390d0e in js::mjit::ic::CallProp (f=..., pic=0x7f3bfb635a40) at /home/jwalden/moz/2/js/src/methodjit/PolyIC.cpp:2263

The only bad news is that tab set is a (pretty-printed) 4.1MB sessionstore.js file, and it's fragile in the face of efforts to reduce it.  But reduction is definitely possible if truly necessary: when I first started reducing (before pulling out a debugger) the file was 6.9MB.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

There's a testcase in bug 606288 which reproduces this crash reliably.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

jst hit this on linux x64 today. Sean, potentially easy fix, can we add our own branchWithPatch, that either (a) clobbers one of the unused registers, or (b) loads the offset out of a memory table at the bottom of the script?

Comment 11

[Johnny Stenback (:jst)]

David says we want to fix this for Firefox 4. Marking a blocker.

Comment 12

[Sean Stangl [:sstangl]]

(In reply to comment #10)
> jst hit this on linux x64 today. Sean, potentially easy fix, can we add our own
> branchWithPatch, that either (a) clobbers one of the unused registers, or (b)
> loads the offset out of a memory table at the bottom of the script?

All of our code uses near, relative jumps -- there is no such thing as a 64-bit relative jump, so we would have to extend the Assembler to handle near, absolute indirect jumps (clobbering %r11, the scratchRegister), then always patch that value.

But doing absolute jumps in this manner has a significant performance penalty. To mitigate the damage, we may want different 'flavors' of branchWithPatch code, which know whether the jump is local to the buffer or possibly external.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: diagnostic patch

Yesterday Brendan managed to get a crash in gdb, where an IC jump was completely bogus. It was low enough to make this bug a possible culprit, though it's much less likely to happen on OS X than it is on Linux. Another possibility for that crash is that a GC failed to unpatch an IC, but freed the code pointer.

This patch will crash at 0xC0DE when we're about to patch an IC jump with an invalid offset.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Brendan, this is the candidate for your crash.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Diagnostic patch landed as http://hg.mozilla.org/tracemonkey/rev/6f9c075ee45e

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Taking. We should fix this sooner rather than later, whether or not it's happening on Mac in the wild.

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Verified by one of Brendan's crashes that this is happening on OS X. Yuck. Patch in a day or two, the plan of attack is to extend code buffers with padding at the end, enough padding to add an absolute jump for every patchable jump. Then we'll add FarJump/CodeLocationFarJump structures to the assembler which will describe how to patch both locations.

This way, most of the time when address spaces are in range, there will be no performance hit. Once we start overflowing addresses, we'll start using indirect jumps.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v1

Sean, does this seem okay to you? There's a new assembler function that lets you wrap jumps as extended branches. Right before assembling, the buffer is extended with absolute jumps.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: framework for the fix

A few CallIC spots that need FarJumps are included in this patch. To get the other ICs working needs a lot of refactoring, so that will be a separate patch.

Comment 21

[David Mandelin [:dmandelin]]

Question: What is the "correction" value for?

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

The far jump generates code like:

1.   jne  _end_of_buffer

_end_of_buffer:
2.   mov rax, <absolute address>
3.   jmp rax

The DataLabelPtr is for patching the absolute address, the "correction" (poorly named, sorry) scoots back X bytes to get the start address of the thunk, to patch #1 -> #2.

Comment 23

[David Mandelin [:dmandelin]]

Comment on attachment 490926 [details] [diff] [review]
part 1: framework for the fix

Can we rename farJumpCorrectionDistance to FarJump_StartOffsetFromTargetData or something like that? And add a comment to the value (if comment is good, name can be shorter)? Otherwise looks good.

And can you file a followup on fixing the OOM-handling for vectors in BaseAssembler?

Comment 24

[Sean Stangl [:sstangl]]

Comment on attachment 490926 [details] [diff] [review]
part 1: framework for the fix

Are we sure that FarJumpCorrectionDistance is actually a constant? Can we assert that it is?

> Allocation mem = systemAlloc(size_t(4294967291));

This should be a #define, since it occurs multiple times and demands context to be understood. This value is 0xFFFFFFFB, which seems arbitrary to me. Naming the value would clarify it.

Looks good.

Comment 25

[Jeff Walden [:Waldo]]

You surely meant const size_t, right?  ;-)  More debuggable and all that.

Comment 26

[David Anderson [:dvander] - inactive, e-mail if emergency]

The oversize allocation is just a hack so the problem is really easy to testcase and debug. It'll get removed before checkin so x64 users don't allocate 81234834GB of RAM with the method jit.

Comment 27

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2: refactor PICLabels to work on all platforms

This patch is based off the refactoring that Chris and Jacob did for ARM. The goal is to begin moving all of the gross #ifdefs out of the IC code and compiler, and put it behind an interface.

PICLabels now exist for all platforms, with a distinction: if labels are not needed, a single version with hardcoded offsets is returned instead. When trying to mutate this version, it just asserts that the incoming value matches the hardcoded constant.

Comment 28

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3: fix getprop type guards

Fixes inline type guards on GETPROP

Comment 29

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4: fix rest of call ICs

Comment 30

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 491052 [details] [diff] [review]
part 2: refactor PICLabels to work on all platforms

I'm pretty sure that constant prop won't happen on x86 because of the dynamic-static initialization (though it is a neat trick!) -- usually static constructors run before |_main|. Can we use macro expansion for the private members of the *Labels structs, so that they form bitfield members |#if defined JS_HAS_IC_LABELS| but |static const int|s otherwise (which would require static initializers in the cpp file)? That way, we'd definitely get constant prop and the getters could still access members by the same name. This was the plan I had in mind for our MQ, but didn't try to work through it.

- Looks like some FarJump updates ended up in this patch -- maybe you want these to be part of the former patch on commit?
- Might want to get rid of the offset-getting helpers in the compilers now that it's a no-ifdef-required interface.

-    PICLabels labels;
+#if !defined JS_HAS_IC_LABELS
+    static GetPropLabels getPropLabels_;

Nit: positive reasoning on the "then" branch is easier to read when there's an "else", so I would flip-flop these blocks.

Comment 32

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP

Comment 33

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1: correctly bound code blocks

I'm ditching the FarJump approach. It solved the problem nicely - no perf cost, and the type system prevented invalid links at compile-time. But it was a ton of code motion, probably too risky for b8. It also bloated the IC sizes, and masm.far() made the code look really gross.

Instead, let's just disable ICs that can't be patched. It seems really rare, and if it starts impacting performance, we can either gradually re-introduce the original patch, or work toward something fancier, like relocating code or recompilation. I'll file a follow-up bug.

This first patch reintroduces some of the safer refactorings, and makes it so that whenever we want to patch an IC stub, its size is retained in addition to its address.

Comment 34

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2: the fix. disable unpatchable ICs

Comment 36

[Jacob Bramley [:jbramley]]

For the record, the FarJump/NearJump distinction could be useful on ARM. We don't need it for the next release but it'd be useful to consider in the future.

ARM has relative branches with a range of ±32MB. (That's 24 significant bits in the 'B' instruction, with an alignment restriction of 4 bytes.) Currently, we emit all branches using "LDR pc, =<address>", which loads the address from memory directly into the PC. This will be slower than 'B', and takes twice as much space for each branch. (With PICs, we could have quite a few branches.)

Bug 586297 aims to re-enable 'B' branches by allowing the back-end to handle the patching, but if you're planning to introduct FarJump/NearJump distinctions then ARM could probably just use that mechanism instead.

Comment 37

[Sean Stangl [:sstangl]]

Comment on attachment 491460 [details] [diff] [review]
part 1: correctly bound code blocks

> namespace js { namespace mjit { struct JITScript; } }

Should this only be defined if JS_METHODJIT?

Everything else looks good.

Comment 38

[Sean Stangl [:sstangl]]

Comment on attachment 491461 [details] [diff] [review]
part 2: the fix. disable unpatchable ICs

canRelinkJump() is defined in X86Assembler.h, but it can be specialized for x86 in the same way that it is specialized for ARM, saving some calculations. Specialization between the Intel architectures usually involves #ifdef magic, or defining functions in the MacroAssemblerX86*.h files.

> //#define STRESS_JSC_ALLOCATOR

Can we prefix it with 'DEBUG_' and add a comment?

> if (hasLastStringStub && !buffer.verifyRange(lastStringStub))
>     return disable(cx, "code memory is out of range");
> if ((shouldPatchInlineTypeGuard() || shouldPatchUnconditionalClaspGuard()) &&
>     !buffer.verifyRange(cx->fp()->jit())) {
>          return disable(cx, "code memory is out of range");
> }

No less clear with one if().

Looks good. Excited to end these crashes.

Comment 39

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/54fb9d61096a
http://hg.mozilla.org/tracemonkey/rev/bbc8aeb55bae

I kept the two ifs separate, it's not a very complicated expression but it's verbose, and the formatting gets weird.

Comment 41

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: follow-up fix for more range checks

This crash seems all but gone. There's a few stragglers though.

It looks like the range checks aren't complete, it's necessary to always test the range to the script in addition to the last generated stub.

Comment 43

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/a230f26db792

Comment 44

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bbc8aeb55bae

Comment 45

[David Anderson [:dvander] - inactive, e-mail if emergency]

Brendan got another crash here, the caller was GetElementIC::attachGetProp. It looks like I missed this check when widening the ranges in part 3. Follow-up fix shortly.

Comment 46

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: follow-up 2: another range fix

Comment 47

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/e5c6b0151c7b
http://hg.mozilla.org/mozilla-central/rev/160e6bf9752e

Comment 48

[(not currently active) Ted Mielczarek]

(In reply to comment #2)
> (Weird, why doesn't it print 64-bit values for those registers?)

That's a bug in minidump_stackwalk that I apparently introduced and totally forgot to fix, sorry.

Comment 49

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e5c6b0151c7b