Bug 169724

Summary: WebAssembly: function-tests/load-offset.js fails on ARM64
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, jfbastien, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 161709    
Attachments:
Description Flags
Patch keith_miller: review+

Description JF Bastien 2017-03-15 17:21:05 PDT
Segfaults on ARM64, not on x86:

# DYLD_FRAMEWORK_PATH=... lldb .../jsc -- -m --useWebAssembly=1 ./function-tests/load-offset.js ; echo $?
(lldb) r
Process 64578 stopped
* thread #1: tid = 0x5a2002, 0x00000002cca6d860, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x2c2800004)
    frame #0: 0x00000002cca6d860
->  0x2cca6d860: ldur   w0, [x1, #4]
    0x2cca6d864: movz   x1, #0xf38
    0x2cca6d868: movk   x1, #0xa0, lsl #16
    0x2cca6d86c: movk   x1, #0x1, lsl #32
(lldb) bt
* thread #1: tid = 0x5a2002, 0x00000002cca6d860, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x2c2800004)
  * frame #0: 0x00000002cca6d860
    frame #1: 0x00000002cca6d8f4
    frame #2: 0x0000000100861fc8 JavaScriptCore`llintPCRangeStart + 264
    frame #3: 0x0000000100a4bc44 JavaScriptCore`JSC::callWebAssemblyFunction(JSC::ExecState*) + 1620
    frame #4: 0x000000010085fbf4 JavaScriptCore`JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 384
    frame #5: 0x00000001008688a8 JavaScriptCore`llint_entry + 26392
    frame #6: 0x0000000100861fc8 JavaScriptCore`llintPCRangeStart + 264
    frame #7: 0x00000001006fd9b4 JavaScriptCore`JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 136
    frame #8: 0x00000001006d1278 JavaScriptCore`JSC::Interpreter::execute(JSC::ModuleProgramExecutable*, JSC::ExecState*, JSC::JSModuleEnvironment*) + 468
    frame #9: 0x000000010078d800 JavaScriptCore`JSC::JSModuleRecord::evaluate(JSC::ExecState*) + 60
    frame #10: 0x0000000100789cb8 JavaScriptCore`JSC::JSModuleLoader::evaluate(JSC::ExecState*, JSC::JSValue, JSC::JSValue, JSC::JSValue) + 412
    frame #11: 0x00000002cca50030
    frame #12: 0x0000000100868854 JavaScriptCore`llint_entry + 26308
    frame #13: 0x00000001008688b8 JavaScriptCore`llint_entry + 26408
    frame #14: 0x00000002cca584a0
    frame #15: 0x0000000100861fc8 JavaScriptCore`llintPCRangeStart + 264
    frame #16: 0x00000001006fd9b4 JavaScriptCore`JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 136
    frame #17: 0x00000001006d0aec JavaScriptCore`JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 400
    frame #18: 0x0000000100330c60 JavaScriptCore`JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 168
    frame #19: 0x0000000100783628 JavaScriptCore`JSC::JSJobMicrotask::run(JSC::ExecState*) + 400
    frame #20: 0x00000001009f2f58 JavaScriptCore`JSC::VM::drainMicrotasks() + 272
    frame #21: 0x0000000100007b40 jsc`jscmain(int, char**) + 3328
    frame #22: 0x0000000100006e30 jsc`main + 52
    frame #23: 0x0000000182ddd59c libdyld.dylib`start + 4
Comment 1 JF Bastien 2017-03-15 17:23:40 PDT
Generated JIT code for WebAssembly function[0] I32 (I32):
    Code at [0x2709b5ba0, 0x2709b5c20):
         0x2709b5ba0:    stp    fp, lr, [sp, #-16]!
         0x2709b5ba4:    mov    fp, sp
         0x2709b5ba8:    movz   x2, #0x0
         0x2709b5bac:    movk   x2, #0x0, lsl #16
         0x2709b5bb0:    movk   x2, #0x0, lsl #32
         0x2709b5bb4:    stur   x2, [fp, #24]
         0x2709b5bb8:    movz   x2, #0x0
         0x2709b5bbc:    stur   x2, [fp, #16]
         0x2709b5bc0:    .long  d53bd062
         0x2709b5bc4:    and    x2, x2, #0xfffffffffffffff8
         0x2709b5bc8:    ldr    x2, [x2, #736]
         0x2709b5bcc:    ubfx   x0, x0, #0, #32
         0x2709b5bd0:    add    x0, x0, #7
         0x2709b5bd4:    cmp    x0, x19
         0x2709b5bd8:    b.hs   0x2709b5bfc
         0x2709b5bdc:    add    x1, x1, x20
         0x2709b5be0:    ldur   w0, [x1, #4]                 <------------- here
         0x2709b5be4:    movz   x1, #0xf38
         0x2709b5be8:    movk   x1, #0xa0, lsl #16
         0x2709b5bec:    movk   x1, #0x1, lsl #32
         0x2709b5bf0:    blr    x1
         0x2709b5bf4:    ldp    fp, lr, [sp], #16
         0x2709b5bf8:    ret    lr
         0x2709b5bfc:    movz   w1, #0x0
         0x2709b5c00:    b      0x2709b1260


x1 is 0x0000000203f00000.
x20 is 0x0000000101700000.
Comment 2 JF Bastien 2017-03-16 09:47:07 PDT
I may have posted the wrong symptom: the test segfaults when you run it *without* lldb, but when you run it *with* lldb the segfaults w=encountered may be the ones we intentionally generate for trapping out-of-bounds loads.

Therefore, reproducing in lldb requires ignoring all but the last segfault, or teaching the trap handler to be very mad when it doesn't like where the segfault occurs.
Comment 3 JF Bastien 2017-03-16 09:50:14 PDT
Running lldb with `pro hand -p true -s false -n false SIGSEGV` doesn't seem to help much :(
Comment 4 Michael Saboff 2017-03-16 10:31:47 PDT
I can reproduce this intermittently without JF's patch.
Comment 5 Michael Saboff 2017-03-16 14:02:30 PDT
Created attachment 304689 [details]
Patch
Comment 6 Keith Miller 2017-03-16 14:04:09 PDT
Comment on attachment 304689 [details]
Patch

r=me. bug=me.
Comment 7 Michael Saboff 2017-03-16 14:10:45 PDT
Committed r214068: <http://trac.webkit.org/changeset/214068>
Comment 8 Filip Pizlo 2017-03-16 14:18:20 PDT
Comment on attachment 304689 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304689&action=review

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2769
>          case WasmAddress: {
>              WasmAddressValue* address = m_value->as<WasmAddressValue>();
>  
> -            append(Add64, Arg(address->pinnedGPR()), tmp(address));
> +            append(Add64, Arg(address->pinnedGPR()), tmp(m_value->child(0)), tmp(address));
>              return;
>          }

Seems like if the offset is not legal, we should have a legalizeoffsets pass that extracts the offset add into a separate B3 op that can be CSE'd. https://bugs.webkit.org/show_bug.cgi?id=169782