1445907 - [regex] Assertion failure: xreg(28) == x28, at js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:765

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision fcb11e93adf5+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize --enable-simulator=arm64, run with --fuzzing-safe):

function wasmRunWithDebugger(wast, lib, init, done) {
    let g = newGlobal('');
    let dbg = new Debugger(g);
    g.eval(`
      var wasm = wasmTextToBinary('${wast}');
      var lib = ${lib || 'undefined'};
      var m = new WebAssembly.Instance(new WebAssembly.Module(wasm), lib);
    `);
    var wasmScript = dbg.findScripts().filter(s => s.format == 'wasm')[0];
    init({dbg, wasmScript, g,});
    result = g.eval("m.exports.test()");
}
wasmRunWithDebugger('(module (func (nop) (nop)) (export "test" 0))', undefined, function({dbg}) {
    offsets = [];
    dbg.onEnterFrame = function(frame) {
        offsets.push(
            (r = /^\u0$/.exec("u0")) && r[0], 
        );
    };
});


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00000000009dedd0 in vixl::Simulator::VisitCallRedirection (this=0x7ffff5f3b000, instr=0x7ffff4647f28) at js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:765
#0  0x00000000009dedd0 in vixl::Simulator::VisitCallRedirection (this=0x7ffff5f3b000, instr=0x7ffff4647f28) at js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:765
#1  0x00000000009e49f5 in vixl::Simulator::VisitException (this=0x7ffff5f3b000, instr=<optimized out>) at js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:478
#2  0x00000000009761e8 in vixl::Decoder::VisitException (this=<optimized out>, instr=0x7ffff4647f28) at js/src/jit/arm64/vixl/Decoder-vixl.cpp:872
#3  0x00000000009d4d75 in vixl::Decoder::Decode (instr=<optimized out>, this=<optimized out>) at js/src/jit/arm64/vixl/Decoder-vixl.h:158
#4  vixl::Simulator::ExecuteInstruction (this=0x7ffff5f3b000) at js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:195
#5  0x00000000009d73cc in vixl::Simulator::Run (this=0x7ffff5f3b000) at js/src/jit/arm64/vixl/Simulator-vixl.cpp:68
#6  0x00000000009d523d in vixl::Simulator::RunFrom (first=0x2488c2539090, this=0x7ffff5f3b000) at js/src/jit/arm64/vixl/Simulator-vixl.cpp:76
#7  vixl::Simulator::call (this=0x7ffff5f3b000, entry=entry@entry=0x2488c2539090 "\377C", argument_count=argument_count@entry=2) at js/src/jit/arm64/vixl/MozSimulator-vixl.cpp:326
#8  0x0000000000de104b in js::wasm::Instance::callExport (this=this@entry=0x7ffff450d5f0, cx=cx@entry=0x7ffff5f16000, funcIndex=<optimized out>, args=...) at js/src/wasm/WasmInstance.cpp:736
#9  0x0000000000de18b0 in WasmCall (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/wasm/WasmJS.cpp:1148
#10 0x00000000005796fd in js::CallJSNative (cx=0x7ffff5f16000, native=0xde17f0 <WasmCall(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:290
[...]
#18 0x00000000005a5a90 in js::IndirectEval (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Eval.cpp:416
#19 0x00000000005796fd in js::CallJSNative (cx=0x7ffff5f16000, native=0x5a59a0 <js::IndirectEval(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:290
#20 0x000000000056dcff in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff5f16000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:467
#21 0x000000000056e0dd in InternalCall (cx=0x7ffff5f16000, args=...) at js/src/vm/Interpreter.cpp:516
#22 0x000000000056e260 in js::Call (cx=<optimized out>, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:535
#23 0x0000000000a607e4 in js::ForwardingProxyHandler::call (this=<optimized out>, cx=0x7ffff5f16000, proxy=..., args=...) at js/src/proxy/Wrapper.cpp:176
#24 0x0000000000a4a6cb in js::CrossCompartmentWrapper::call (this=0x20956f0 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff5f16000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:358
#25 0x0000000000a5d2f5 in js::Proxy::call (cx=0x7ffff5f16000, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:510
#26 0x0000000000a5d3bd in js::proxy_Call (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:769
#27 0x00000000005796fd in js::CallJSNative (cx=0x7ffff5f16000, native=0xa5d340 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:290
[...]
#41 0x0000000000442f92 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9410
rax	0x0	0
rbx	0x7ffff5f3b000	140737319776256
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffaf70	140737488334704
rsp	0x7fffffffae80	140737488334464
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x0	0
r11	0x0	0
r12	0x7ffff4647f28	140737293614888
r13	0xd9e5e0	14280160
r14	0xbadbeef	195935983
r15	0xbadbeef	195935983
rip	0x9dedd0 <vixl::Simulator::VisitCallRedirection(vixl::Instruction const*)+2736>
=> 0x9dedd0 <vixl::Simulator::VisitCallRedirection(vixl::Instruction const*)+2736>:	movl   $0x0,0x0
   0x9deddb <vixl::Simulator::VisitCallRedirection(vixl::Instruction const*)+2747>:	ud2

Comment 1

[Lars T Hansen [:lth]]

A symptom that the PseudoStackPointer is not saved and restored properly across a callout.

Comment 2

[Lars T Hansen [:lth]]

Looks like it's related to regex compilation or interpretation, here's an updated test case:

 // the regex is needed, which is a clue
 // if the compile is outside the onEnterFrame then no crash

 var g = newGlobal('');
 var dbg = new Debugger(g);
 g.eval(`var m = new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary('(module (func (export "test")))')))`);
 var re = /./;
 //re.compile();
 dbg.onEnterFrame = function(frame) { re.exec("x") };
 result = g.eval("m.exports.test()");

Comment 3

[Lars T Hansen [:lth]]

NativeRegExpMacroAssembler::GenerateCode() clobbers x28 by setting up the PSP without saving x28 first, same bug as 1375074 but in a different location in the code.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1445907-save-x28-in-regex-compiler.patch

Tentative fix.  Fixes the TC but I need to make sure no other paths need to be handled.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1445907-save-x28-in-regex-compiler.patch

Same patch as for bug 1375074 (see discussion there), ported to regex compiler.  I'm pretty sure I got all the paths; only the normal path restores registers and so we should really only need to restore the saved x28 there.

Bonus: the test case that uncovered the failure on the simulator.

Comment 6

[Sean Stangl [:sstangl]]

Comment on attachment 8960113 [details] [diff] [review]
bug1445907-save-x28-in-regex-compiler.patch

Review of attachment 8960113 [details] [diff] [review]:
-----------------------------------------------------------------

You know, another option to all this is that we could just use a volatile register, like x18.

This problem is occurring because we're using x28 -- the compiler is correctly assuming that it's unchanged. But if we used a volatile, the compiler's assumption that it could be changed would handle this for us automatically and probably produce better code.

Comment 7

[Sean Stangl [:sstangl]]

Although, then we'd have the same problem on JIT exit points. That may be a bit nicer, but maybe it doesn't matter.

Comment 8

[Lars T Hansen [:lth]]

(In reply to Sean Stangl [:sstangl] from comment #6)
> Comment on attachment 8960113 [details] [diff] [review]
> bug1445907-save-x28-in-regex-compiler.patch
> 
> Review of attachment 8960113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You know, another option to all this is that we could just use a volatile
> register, like x18.
> 
> This problem is occurring because we're using x28 -- the compiler is
> correctly assuming that it's unchanged. But if we used a volatile, the
> compiler's assumption that it could be changed would handle this for us
> automatically and probably produce better code.

Definitely worth investigating.  As you say, there's an issue with JIT exit points.  ISTR the wasm Tls register had to be in a nonvolatile register in order for code not having to worry about that; so it may be that using a volatile just shifts the complexity around.

The previous error was caught by a user, this one and another issue that never made it into the trees were caught in the simulator.  I've been toying with the idea of writing tests that stress the problem more systematically by deliberately poisoning registers so that the simulator's check is more likely to catch bugs.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/903a79a1efff
Save x28 before clobbering it in the regex compiler. r=sstangl

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/903a79a1efff