Bug 124727 – ARM64: Hang running pdfjs test, suspect DFG generated code for "in"

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

When running the pdfjs test, we end up in a infinite loop.  Looks like it is in DFG generated code for "in".  Here is one example of the register values and generated code after getting stuck:

(lldb) reg read x1 x16 x17
      x1 = 0x0000000000000007
     x16 = 0x0000000101695f70
     x17 = 0x0000000101695f70

Generated DFG JIT code for charToGlyph#DoDTlR:[0x14cdf7890->0x102319d70, DFGFunctionCall (StrictMode)], instruction count = 1061:
    Optimized with execution counter = 3015.000000/2830.000000, 5
    Code at [0x16540e1a0, 0x16540f9c8):
...

    Block #56 (bc#812):
      Predecessors: #30 #22 #27 #11 #14 #4 #7 #34 #44 #54 #50 #55
      Dominated by: #0 #56
      Dominates: #56 #57 #58 #59 #60 #61 #62 #63 #64 #65 #66 #67 #68 #69
      Phi Nodes: @509<3>->(@231, @208), @205<2>->(@280, @202), @282<1>->(@283, @332, @120), @119<1>->(@282, @204, @331), @281<1>->(@119, @364, @556), @363<1>->(@281, @203, @609), @280<1>->(@363, @330, @118), @102<2>->(@99, @768), @167<1>->(@168, @304, @100), @303<1>->(@167, @869, @788), @334<1>->(@303, @583, @568), @337<1>->(@334, @636, @621), @99<1>->(@337, @738, @710), @95<2>->(@260, @766), @94<1>->(@172, @216, @299), @338<1>->(@94, @339, @215), @214<1>->(@338, @298, @371), @92<1>->(@214, @93, @297), @260<1>->(@92, @370, @185), @354<1>->(@510, @272, @348), @374<1>->(@354, @351, @357), @384<1>->(@374, @359, @379), @78<1>->(@384, @382, @386), @231<1>->(@78, @388, @177), @232<2>->(@393, @286), @176<1>->(@253, @396, @407), @394<1>->(@176, @73, @230), @72<1>->(@394, @251, @276), @229<1>->(@72, @406, @133), @393<1>->(@229, @175, @250)
             0x16540e950:    tst    x29, #0x7
             0x16540e954:    b.eq   0x16540e95c
             0x16540e958:    brk    #0
     947:           < 1:-15>    JSConstant(JS|UseAsOther, Stringident, $1 = String (identifier): toUnicode, bc#812)
     948:           < 1:-11>    GetLocal(@509, JS|UseAsOther, Final, arg0(N<Final>/FlushedCell), machine:arg0, R:Variables(6), bc#812)  predicting Final
             0x16540e95c:    ldur   x0, [x29, #48]
     949:           <!1:-11>    In(@947<StringIdent>, Cell:@948<Final>, Boolean|MustGen|Clobbers|UseAsOther, Bool, R:World, W:World, bc#812)
             0x16540e960:    tst    x0, x28
             0x16540e964:    b.eq   0x16540e96c
             0x16540e968:    brk    #0
       ==>   0x16540e96c:    b      0x16540f090  <== this branch has been patched to "b 0x16540f9e0"
     950:  skipped  < 0:->      MovHint(@949<Boolean>, loc6(QH~<Boolean>/FlushedJSValue), machine:loc6, W:SideState, bc#812)
     450:           <!0:->      InvalidationPoint(MustGen|CanExit, W:SideState, bc#816)
     951:           <!0:->      Branch(Boolean:@949<Boolean>, MustGen|CanExit, T:#58, F:#57, W:SideState, bc#816)
             0x16540e970:    tbnz   x1, #0, 0x16540e990
    Block #57 (bc#819):
…

Generated JIT code for DFG In (found = yes) stub for charToGlyph#DoDTlR:[0x14cdf7890->0x102319d70, DFGFunctionCall (StrictMode)], return point 0x16540e96c:
    Code at [0x16540f9e0, 0x16540fa20):
         0x16540f9e0:    ldur   x17, [x0]
         0x16540f9e4:    movz   x16, #24432
         0x16540f9e8:    movk   x16, #361, lsl #16
         0x16540f9ec:    movk   x16, #1, lsl #32
         0x16540f9f0:    cmp    x17, x16
         0x16540f9f4:    b.ne   0x16540f090
         0x16540f9f8:    nop
         0x16540f9fc:    mov    x1, #0x7
         0x16540fa00:    b      0x16540e96c

We execute through the stub with the compare of x16 & x17 being equal.  The branch at the end of the stub takes us to the branch that has been patch, which takes us back to the stub.

[Michael Saboff]

I commented out the isConstant() fast path in SpeculativeJIT::compileIn() and pdfjs now works.  Looking into the patching that happens in operationInOptimize().

    if (isConstant(node->child1().node())) {
        JSString* string =
            jsDynamicCast<JSString*>(valueOfJSConstant(node->child1().node()));
        if (string && string->tryGetValueImpl()
            && string->tryGetValueImpl()->isIdentifier()) {
            StructureStubInfo* stubInfo = m_jit.codeBlock()->addStubInfo();
            
            GPRTemporary result(this);
            GPRReg resultGPR = result.gpr();

            use(node->child1());
                
            MacroAssembler::PatchableJump jump = m_jit.patchableJump();
            
            OwnPtr<SlowPathGenerator> slowPath = slowPathCall(
                jump.m_jump, this, operationInOptimize,
                JSValueRegs::payloadOnly(resultGPR), stubInfo, baseGPR,
                string->tryGetValueImpl());
            
            stubInfo->codeOrigin = node->codeOrigin;
            stubInfo->patch.baseGPR = static_cast<int8_t>(baseGPR);
            stubInfo->patch.valueGPR = static_cast<int8_t>(resultGPR);
            stubInfo->patch.usedRegisters = usedRegisters();
            stubInfo->patch.registersFlushed = false;
            
            m_jit.addIn(InRecord(jump, slowPath.get(), stubInfo));
            addSlowPathGenerator(slowPath.release());
                
            base.use();
                
#if USE(JSVALUE64)
            jsValueResult(
                resultGPR, node, DataFormatJSBoolean, UseChildrenCalledExplicitly);
#else
            booleanResult(resultGPR, node, UseChildrenCalledExplicitly);
#endif
            return;
        }
    }

[Michael Saboff]

The issue here is the differences between ARM64 and most other macro assemblers as to how a CodeLocationLabel records the location of a jump.  For ARM64, we record the address of the jump while most of the other architectures point at the instruction AFTER the jump.  We overuse the CodeLocationLabel for the jump to patch AND the location to continue after the stub we jumped to.  It is that returning jump that gets messed up.

[Michael Saboff]

<rdar://problem/15566923>

[Filip Pizlo]

Created attachment 219041 [details]
the patch

[WebKit Commit Bot]

Attachment 219041 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/StructureStubInfo.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp', u'Source/JavaScriptCore/jit/Repatch.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/JavaScriptCore/dfg/DFGJITCompiler.h:87:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/160493

[Geoffrey Garen]

Can we add a test for this to run-webkit-tests or run-javascriptcore-tests?

[Filip Pizlo]

(In reply to comment #7)
> Can we add a test for this to run-webkit-tests or run-javascriptcore-tests?

We have the ability to run pdfjs as part of both of those things.

Also, I believe that we already have op_in tests.