Summary: | Assertion being hit on layout tests in debug build | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gustavo Noronha (kov) <gustavo> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | Gavin Barraclough <barraclough> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | barraclough, oliver, plaes, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
Attachments: |
|
Description
Gustavo Noronha (kov)
2009-08-14 14:25:54 PDT
Created attachment 34872 [details]
backtrace
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. 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 ;) Created attachment 35036 [details]
webkit-bug-28317-debug-fix-js-jit-assertion.patch
Change the size to 35, so it doesn't assert anymore...
(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 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; 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) Comment on attachment 35036 [details]
webkit-bug-28317-debug-fix-js-jit-assertion.patch
Tabs in your ChangeLog will cause the commit to fail.
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. Created attachment 38459 [details]
dump of breakpoint
Here's a dump I got following gbarra's instructions =).
Created attachment 38606 [details]
jit_bactrace.txt
Backtrace I'm getting when starting epiphany.
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.
Comment on attachment 38646 [details]
Speculative fix
r=me, assuming the fix works.
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. 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?
Created attachment 38681 [details]
Fix pt II.
Comment on attachment 38681 [details]
Fix pt II.
r=me
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. (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 I believe kov has confirmed this, so marking fixed. Please reopen if this is not the case. |