2007081 - PGO crash on try when adding JetStream 3 to our PGO training set

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Yannis Juglaret [:yannis]]

Dedicated bug for bug 1962418 comment 17. When we train PGO on wasm code execution, the inlining decisions seem to lead to a compiler bug that makes Firefox crash during wasm execution. Link [2] in the comment provides a detailed call stack with inlining decisions.

Relevant bits of C++:

// js/src/jit/x86-shared/Encoding-x86-shared.h
  OP_SUB_GvEv = 0x2B,

// js/src/jit/x86-shared/BaseAssembler-x86-shared.h
  void subl_rr(RegisterID src, RegisterID dst) {
    spew("subl       %s, %s", GPReg32Name(src), GPReg32Name(dst));
    m_formatter.oneByteOp(OP_SUB_GvEv, src, dst);
  }

    void oneByteOp(OneByteOpcodeID opcode, RegisterID rm, int reg) {
      m_buffer.ensureSpace(MaxInstructionSize);
      emitRexIfNeeded(reg, 0, rm);
      m_buffer.putByteUnchecked(opcode);
      registerModRM(rm, reg);
    }

In today's Nightly this code looks as follows at the call site of interest:

// ...
xul!js::jit::X86Encoding::BaseAssembler::subl_rr [/builds/worker/checkouts/gecko/js/src/jit/x86-shared/BaseAssembler-x86-shared.h @ 1265]
[inlined in xul!js::wasm::BaseCompiler::emitRemainderI32+0x1e4 [/builds/worker/checkouts/gecko/js/src/wasm/WasmBaselineCompile.cpp @ 3318]]:
movzx r9d,dil
mov   rcx,r15
mov   edx,2Bh
mov   r8d,ebp
call  xul!js::jit::X86Encoding::BaseAssembler::X86InstructionFormatter::oneByteOp
// ...

xul!js::jit::X86Encoding::BaseAssembler::X86InstructionFormatter::oneByteOp [/builds/worker/checkouts/gecko/js/src/jit/x86-shared/BaseAssembler-x86-shared.h @ 5571]:
push  r14
push  rsi
push  rdi
push  rbx
sub   rsp,28h
mov   rax,qword ptr [rcx+8]
lea   r10,[rax+10h]
cmp   r10,qword ptr [rcx+10h]
ja    xul!js::jit::X86Encoding::BaseAssembler::X86InstructionFormatter::oneByteOp+0x88
movzx r10d,r8b
mov   r11d,r9d
or    r11d,r10d
cmp   r11d,8
jae   xul!js::jit::X86Encoding::BaseAssembler::X86InstructionFormatter::oneByteOp+0x5f
mov   r10,qword ptr [rcx]
mov   byte ptr [r10+rax],dl
mov   rax,qword ptr [rcx]
mov   rdx,qword ptr [rcx+8]
lea   r10,[rdx+1]
mov   qword ptr [rcx+8],r10
and   r8b,7
shl   r9b,3
or    r9b,r8b
or    r9b,0C0h
mov   byte ptr [rax+rdx+1],r9b
inc   qword ptr [rcx+8]
add   rsp,28h
pop   rbx
pop   rdi
pop   rsi
pop   r14
ret

Compared to Nightly, in the try push we took the extra decision to inline xul!js::jit::X86Encoding::BaseAssembler::X86InstructionFormatter::oneByteOp at this call site. Somehow the corresponding code looks like this:

mov  rbx, qword ptr [r13]
mov  rax, qword ptr [rbx+560h]
lea  rcx, [rax+10h]
cmp  rcx, qword ptr [rbx+568h]
ja   xul!js::wasm::BaseCompiler::emitBody+0x6f730
mov  ecx, dword ptr [rsp+90h]
or   ecx, dword ptr [rsp+0C0h]
cmp  ecx, 8
jae  xul!js::wasm::BaseCompiler::emitBody+0x5e79a
mov  rcx, qword ptr [rsp+260h]
mov  rcx, qword ptr [rcx] <---- crash
mov  byte ptr [rcx+rax], 2Bh
mov  rax, qword ptr [rbx+558h]
mov  rcx, qword ptr [rbx+560h]
lea  rdx, [rcx+1]
mov  qword ptr [rbx+560h], rdx
lea  edx, [rdi*8]
or   sil, dl
or   sil, 0C0h
mov  byte ptr [rax+rcx+1], sil
inc  qword ptr [rbx+560h]
mov  rax, qword ptr [rsp+78h]
mov  ecx, dword ptr [rsp+0F8h]
or   word ptr [rax], cx
add  rbx, 558h
mov  qword ptr [rsp+260h], rbx
jmp  xul!js::wasm::BaseCompiler::emitBody+0x99d3

I reproduced the crash in a WinDbg time-travel trace by loading the JetStream 3 website in the Shippable build from the try push. Nothing within xul!js::wasm::BaseCompiler::emitBody writes to the address behind rsp+260h before we reach the crash site, hence why we crash when we dereference the value found there. However, our snippet of code itself does write a value there at the end, so I wonder if this could be some bad reordering of instructions where what we want to dereference is the value that will be stored later, especially since I don't see other uses of the value stored at rsp+260h in the function body as far as I can tell.

Comment 1

[Yannis Juglaret [:yannis]]

Below is a manual annotation of the generated assembly code, showing the relation to the source code and emphasizing that we could be trying to use a temporary stack variable before it is set:

// m_buffer.ensureSpace(MaxInstructionSize);
mov rbx, qword ptr [r13]      // rbx = (MacroAssembler*) &this->masm
mov rax, qword ptr [rbx+560h] // rax = (uint64_t) rbx->masm.m_formatter.m_buffer.m_buffer.mLength
lea rcx, [rax+10h]            // rcx = rax + 10
cmp rcx, qword ptr [rbx+568h] // jump if rcx > rbx->masm.m_formatter.m_buffer.m_buffer.mTail.mCapacity
ja  xul!js::wasm::BaseCompiler::emitBody+0x6f730

// emitRexIfNeeded(reg, 0, base);
mov ecx, dword ptr [rsp+90h]
or  ecx, dword ptr [rsp+0C0h]
cmp ecx, 8
jae xul!js::wasm::BaseCompiler::emitBody+0x5e79a

// m_buffer.putByteUnchecked(opcode);
mov rcx, qword ptr [rsp+260h] // rcx = (mozilla::Vector<unsigned char, 256, AssemblerBufferAllocPolicy>*) uninitialized_stack_var
mov rcx, qword ptr [rcx]      // rcx = (unsigned char*) rcx->mBegin
mov byte ptr [rcx+rax], 2Bh   // rcx[rax] = OP_SUB_GvEv
mov rax, qword ptr [rbx+558h] // rax = (unsigned char*) rbx->masm.m_formatter.m_buffer.m_buffer.mBegin
mov rcx, qword ptr [rbx+560h] // rcx = (uint64_t) rbx->masm.m_formatter.m_buffer.m_buffer.mLength
lea rdx, [rcx+1]              // rdx = rcx + 1
mov qword ptr [rbx+560h], rdx // rbx->masm.m_formatter.m_buffer.m_buffer.mLength = rdx

// memoryModRM(offset, base, reg);
lea edx, [rdi*8]
or  sil, dl
or  sil, 0C0h
mov byte ptr [rax+rcx+1], sil // rcx[rax+1] = ...
inc qword ptr [rbx+560h]      // rbx->masm.m_formatter.m_buffer.m_buffer.mLength++

// ?
mov rax, qword ptr [rsp+78h]  
mov ecx, dword ptr [rsp+0F8h]
or  word ptr [rax], cx

// ?
add rbx, 558h                 // rbx = (mozilla::Vector<unsigned char, 256, AssemblerBufferAllocPolicy>*) &rbx->masm.masm.m_formatter.m_buffer.m_buffer
mov qword ptr [rsp+260h], rbx // uninitialized_stack_var = rbx

Comment 2

[Yannis Juglaret [:yannis]]

The inlined code for subq_rr in bug 1962418 comment 19 is very similar, also using a non-initialized stack variable (though not setting it at the end):

// m_buffer.ensureSpace(MaxInstructionSize);
mov  rsi, qword ptr [rsi]
mov  rax, qword ptr [rsi+560h]
lea  rcx, [rax+10h]
cmp  rcx, qword ptr [rsi+568h]
ja   xul!js::wasm::BaseCompiler::emitBody+0x50a2c

// emitRexW(reg, 0, rm);
mov  ecx, edi
shr  cl, 1
and  cl, 34h
or   r13b, cl
or   r13b, 48h
mov  rcx, qword ptr [rsp+1E8h]
mov  rcx, qword ptr [rcx] <-- crash
mov  byte ptr [rcx+rax], r13b

// m_buffer.putByteUnchecked(opcode);
mov  rax, qword ptr [rsi+558h]
mov  rcx, qword ptr [rsi+560h]
lea  rdx, [rcx+1]
mov  qword ptr [rsi+560h], rdx
mov  byte ptr [rax+rcx+1], 2Bh

// registerModRM(rm, reg);
mov  rax, qword ptr [rsi+558h]
mov  rcx, qword ptr [rsi+560h]
lea  rdx, [rcx+1]
mov  qword ptr [rsi+560h], rdx
lea  edx, [rdi*8]
or   bpl, dl
or   bpl, 0C0h
mov  byte ptr [rax+rcx+1], bpl
inc  qword ptr [rsi+560h]

// ?
mov  rax, qword ptr [rsp+70h]
or   word ptr [rax], r12w

// ?
lea  rax, [rsi+558h]
jmp  xul!js::wasm::BaseCompiler::emitBody+0x17c23

Comment 3

[[:sergesanspaille]]

I'm able to reproduce on my Linux workstation. The following snippet

$ llvm-objdump --disassemble-symbols=_ZN2js4wasm12BaseCompiler8emitBodyEv -S obj-x86_64-pc-linux-gnu/dist/bin/libxul.so | grep -B5  \$0x2b, | head -n6

is an easy reproducer, if the last lines are similar to

 3ce2ba0: 48 8b 0b                     	movq	(%rbx), %rcx
 3ce2ba3: c6 04 01 2b                  	movb	$0x2b, (%rcx,%rax)

Then running https://webkit-jetstream-preview.netlify.app/ consistently crashes.

Using this as a reproducer, and using a locally-compiled version of clang, I've been able to confirm that 20.1.8 reproduces the crash and mainbranch doesn't. Let's bisect.

Comment 4

[[:sergesanspaille]]

The fix upstream seems to be https://github.com/llvm/llvm-project/commit/59c6d70ed8120b8864e5f796e2bf3de5518a0ef0, confirmed on Linux.

Comment 5

[[:sergesanspaille]]

Attachment: Bug 2007081 - Backport upstream patch that fixes wrong instruction scheduling r=glandium!

This fixes a codegen bug triggered by JetBrain3 benchmark, see
https://bugzilla.mozilla.org/show_bug.cgi?id=1962418#c17

Comment 6

[Yannis Juglaret [:yannis]]

Asking for landing on behalf of Serge due to PTO.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7b810d534e1d
https://hg.mozilla.org/integration/autoland/rev/e6b5e33d057a
Backport upstream patch that fixes wrong instruction scheduling r=glandium,yjuglaret

Comment 8

[amarc]

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