Bug 28317 – Assertion being hit on layout tests in debug build

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

I am hitting an assertion when running some of the tests that are currently failing in the GTK+ bot:

ASSERTION FAILED: differenceBetween(coldPathBegin, call) == patchOffsetGetByIdSlowCaseCall
(../../JavaScriptCore/jit/JITPropertyAccess.cpp:1260 void JSC::JIT::compileGetByIdSlowCase(int, int, JSC::Identifier*, JSC::SlowCaseEntry*&, bool))

One such test:

http/tests/xmlhttprequest/origin-header-cross-origin-get-async.html

[Gustavo Noronha (kov)]

Created attachment 34872 [details]
backtrace

[Gustavo Noronha (kov)]

This actually seems to crash everything, and seems to only be exposed by a clean build of the GTK+ port, so I believe it may happen in other ports after a clean rebuild. I will try the Qt port, though it would be useful if someone with a different environment could try it.

[Priit Laes (IRC: plaes)]

I'm getting it also with debug build (and webkit-gtk)

From my local backtrace:

(gdb) info locals
coldPathBegin = {m_label = {m_offset = 549, m_used = false}}
stubCall = {static stackIndexStep = 1, static stackIndexStart = 1, m_jit = 0x7ffff8654640, m_stub = 0x7fb63115a505, m_returnType = JSC::JITStubCall::VoidPtr, m_stackIndex = 3}
call = {m_jmp = {m_offset = 584}, m_flags = JSC::AbstractMacroAssembler<JSC::X86Assembler>::Call::Linkable}
__PRETTY_FUNCTION__ = "void JSC::JIT::compileGetByIdSlowCase(int, int, JSC::Identifier*, JSC::SlowCaseEntry*&, bool)"
(gdb) p patchOffsetGetByIdSlowCaseCall
$1 = 41
(gdb) p coldPathBegin
$2 = {m_label = {m_offset = 549, m_used = false}}
(gdb) p call
$3 = {m_jmp = {m_offset = 584}, m_flags = JSC::AbstractMacroAssembler<JSC::X86Assembler>::Call::Linkable}

From here (and your backtrace) the correct value for patchOffsetGetByIdSlowCaseCall should be 35 (584 minus 549 equals 35 and your case 1691-1656 is also 35)

Now we only need to figure out what has changed and which patchOffsetGetByIdSlowCaseCall value in JavaScriptCore/jit/JIT.h should be changed ;)

[Priit Laes (IRC: plaes)]

Created attachment 35036 [details]
webkit-bug-28317-debug-fix-js-jit-assertion.patch

Change the size to 35, so it doesn't assert anymore...

[Priit Laes (IRC: plaes)]

(In reply to comment #4)
> Created an attachment (id=35036) [details]
> webkit-bug-28317-debug-fix-js-jit-assertion.patch
> 
> Change the size to 35, so it doesn't assert anymore...

Seems that it causes crash on regular builds :S

[Priit Laes (IRC: plaes)]

Well, I have currently no better idea than this...

diff --git a/JavaScriptCore/jit/JIT.h b/JavaScriptCore/jit/JIT.h
index 5c6607c..9abdafc 100644
--- a/JavaScriptCore/jit/JIT.h
+++ b/JavaScriptCore/jit/JIT.h
@@ -537,8 +537,12 @@ namespace JSC {
 #if ENABLE(OPCODE_SAMPLING)
         static const int patchOffsetGetByIdSlowCaseCall = 63;
 #else
+#ifndef NDEBUG
+        static const int patchOffsetGetByIdSlowCaseCall = 35;
+#else
         static const int patchOffsetGetByIdSlowCaseCall = 41;
-#endif
+#endif // NDEBUG
+#endif // ENABLE(OPCODE_SAMPLING)
         static const int patchOffsetOpCallCompareToJump = 9;
 
         static const int patchOffsetMethodCheckProtoObj = 20;

[Priit Laes (IRC: plaes)]

It seems that differenceBetween(coldPathBegin, call) varies between 35 and 41 :S

if isMethodCheck == True
  patchOffsetGetByIdSlowCaseCall == 41
else
  patchOffsetGetByIdSlowCaseCall == 35

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fb11e7cd770 (LWP 4654)]
0x00007fb11b82fa70 in JSC::JIT::compileGetByIdSlowCase (this=0x7fff6eb29180, resultVReg=1, baseVReg=2, ident=0x7fb0fc087970, iter=@0x7fff6eb28f00, isMethodCheck=true)
    at JavaScriptCore/jit/JITPropertyAccess.cpp:1260
1260	    ASSERT(differenceBetween(coldPathBegin, call) == patchOffsetGetByIdSlowCaseCall);
Current language:  auto; currently c++
(gdb) p coldPathBegin
$1 = {m_label = {m_offset = 880, m_used = false}}
(gdb) p call
$2 = {m_jmp = {m_offset = 921}, m_flags = JSC::AbstractMacroAssembler<JSC::X86Assembler>::Call::Linkable}
(gdb) p patchOffsetGetByIdSlowCaseCall
$3 = 35
(gdb)

[Eric Seidel (no email)]

Comment on attachment 35036 [details]
webkit-bug-28317-debug-fix-js-jit-assertion.patch

Tabs in your ChangeLog will cause the commit to fail.

[Gavin Barraclough]

Comment on attachment 35036 [details]
webkit-bug-28317-debug-fix-js-jit-assertion.patch

(In reply to comment #7)
> It seems that differenceBetween(coldPathBegin, call) varies between 35 and 41
> :S
> 
> if isMethodCheck == True
>   patchOffsetGetByIdSlowCaseCall == 41
> else
>   patchOffsetGetByIdSlowCaseCall == 35

Hi Priit,

This value shouldn't be varying, the fix here will not be to just change the constant, since this will then fail if isMethodCheck is true.  This patch will break x86-64 on OS X, so please not not land this change.


The code generated should look something like this:

// stubCall.addArgument(regT0);
0x5548490019a9:	mov    %rax,0x8(%rsp)
    ^ 5 bytes

// stubCall.addArgument(ImmPtr(ident));
0x5548490019ae:	mov    $0x100627c90,%r11
0x5548490019b8:	mov    %r11,0x10(%rsp)
    ^ 20 bytes (5 + 15)

// Call call = stubCall.call(resultVReg);
0x5548490019bd:	mov    %rsp,%rdi
0x5548490019c0:	mov    %r13,0x58(%rsp)
0x5548490019c5:	mov    $0x10010c7c4,%r11
0x5548490019cf:	callq  *%r11
    ^ 41 bytes (5 + 15 + 21)
0x5548490019d2:	mov    %rax,0x0(%r13)
    ^ 45 bytes (5 + 15 + 21 + 4)

The offset of 41 is the offset to the end of the call instruction planted by the call to 'stubCall.call'.

I'd suggest you try replacing:
    ASSERT(differenceBetween(coldPathBegin, call) == patchOffsetGetByIdSlowCaseCall);
with:
    if (differenceBetween(coldPathBegin, call) != patchOffsetGetByIdSlowCaseCall) breakpoint();
on line 1260 of JITPropertyAccess.cpp, then run a failing test under gdb.

You should hit the breakpoint, and then be able to grab from memory the instructions that are actually being generated - then we can hopefully work out what what the underlying problem is.

cheers,
G.

[Gustavo Noronha (kov)]

Created attachment 38459 [details]
dump of breakpoint

Here's a dump I got following gbarra's instructions =).

[Priit Laes (IRC: plaes)]

Created attachment 38606 [details]
jit_bactrace.txt

Backtrace I'm getting when starting epiphany.

[Gavin Barraclough]

Created attachment 38646 [details]
Speculative fix

// stubCall.addArgument(ImmPtr(ident));
0x7fffe66e34d1: movq   $0xd0c308,0x10(%rsp)

Hmmm, okay, this is the problem, we're relying on a slightly OS X specific behaviour here (this accounts for the 6 byte difference - this instuction is 9 bytes & we expect 15).

64-bit Mac OS applications have a 4Gb zero page, so pointers are never representable as a 32-bit integer, and always have to be represented by a separate immediate load instruction, rather than within the immediate field of an arithmetic or memory operation.  The problem is that you're hitting a pointer low in memory, and the MacroAssembler is finding a tighter encoding.

We explicitly check for a couple of cases where a value might be representable in 32-bit, but these probably never kick in on Mac OS, and only kick in on GTK to screw you over and break you here (sorry!). :-)  For now, I think we can just cheerfully remove these, and hopefully this will fix the problem for you (rather than coming up with any more complex solution  - since we don't expect these to really be hit right now, we don't really expect these to be benefitting us right now, either performance or memory wise).  On x86-64 we probably just want to stick to a general policy of never trying to compress pointers (on Arm plarforms we have to do something move complex, since ImmPtrs are always converted to Imm32s, so we have to track within the Imm32 whether the value is allowed to be compressed in a tighter encoding).

I don't have a linux box to test on, but I think the attacked patch should fix things for you.

[Oliver Hunt]

Comment on attachment 38646 [details]
Speculative fix

r=me, assuming the fix works.

[Gavin Barraclough]

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/assembler/MacroAssemblerX86_64.h
Transmitting file data ..
Committed revision 47802.


Please could a GTKer confirm whether this fixes & let me know, so I can close the bug if so?

cheers,
G.

[Priit Laes (IRC: plaes)]

Created attachment 38656 [details]
jit_bactrace_with_speculative_patch.txt

Still running into Assertions :S

Is there anything else we could try to see what is causing this?

[Gavin Barraclough]

Created attachment 38681 [details]
Fix pt II.

[Oliver Hunt]

Comment on attachment 38681 [details]
Fix pt II.

r=me

[Gavin Barraclough]

New fix (I'd missed a case that handled unsigned extension, which would probably have been kicking in here - my first patch would have made GTK start emitting two instructions instead of one, as expected, but one of those instructions would contain a compressed immediate).

Give this a try, hopefully this one will nail it.

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/assembler/MacroAssemblerX86Common.h
Sending        JavaScriptCore/assembler/X86Assembler.h
Transmitting file data ...
Committed revision 47834.



If not ...

Another backtrack would help, we need to understand what is getting generated & why.  There is something I think I failed to explain properly previously.  The directions I gave above plant a breakpoint at the *end* of the failing instruction sequence, not at the start (since only at the end do we know there is a problem).  It looks like you're backtrace is for the instructions following the breakpoint, which are not the ones I need to see.

Because of the variable width instruction encoding on x86, reading previous instructions invloves a bit of guesswork, I'm afraid.  Since we know the instructions we are looking for *should* be around 41 bytes before the breakpoint (but aren't exactly - or we wouldn't have a bug here!), then I'd suggest subtracting 41 (decimal) from the RIP you break at, and dumping the instructions.  The trouble it, this may not get all the instructions we need, if the dump starts at the wrong offset (you'll see some instructions disassembled as "(bad)", as in kov's trace).  To try to fix this you can just take a number of dumps at different starting points (+/- 1 to 4 bytes), and try to work out which ones look sensible – or just attach them all.

cheers,
G.

[Priit Laes (IRC: plaes)]

(In reply to comment #18)
> New fix (I'd missed a case that handled unsigned extension, which would
> probably have been kicking in here - my first patch would have made GTK start
> emitting two instructions instead of one, as expected, but one of those
> instructions would contain a compressed immediate).
> 
> Give this a try, hopefully this one will nail it.
> 
> Sending        JavaScriptCore/ChangeLog
> Sending        JavaScriptCore/assembler/MacroAssemblerX86Common.h
> Sending        JavaScriptCore/assembler/X86Assembler.h
> Transmitting file data ...
> Committed revision 47834.

Woohoo :D

JIT seems to be working now :D

Now we only need a 64-bit Linux buildbot :P

[Gavin Barraclough]

I believe kov has confirmed this, so marking fixed.  Please reopen if this is not the case.