Closed Bug 1516720 Opened 2 years ago Closed 2 years ago
Assertion failure: byte
Regs .has(r), at js/src/jit/x86-shared/Macro Assembler-x86-shared .cpp:1013
26.16 KB, text/plain
199.86 KB, application/octet-stream
9.00 MB, application/octet-stream
4.19 MB, application/octet-stream
1.46 KB, patch
|Details | Diff | Splinter Review|
The following testcase crashes on mozilla-beta revision 0e5689df129b (build with --32 --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-baseline --no-ion --test-wasm-await-tier2 w2-out.wrapper): See attachment. Backtrace: #0 CheckBytereg (r=...) at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1013 #1 0x56e64595 in CheckBytereg (r=...) at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1172 #2 AtomicFetchOp<js::jit::Address, js::jit::Register> (masm=..., access=0xf5d048f8, arrayType=js::Scalar::Uint8, op=js::jit::AtomicFetchXorOp, value=..., mem=..., temp=..., output=...) at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1177 #3 0x56e64a55 in js::jit::MacroAssembler::wasmAtomicFetchOp (this=<optimized out>, access=..., op=<optimized out>, value=..., mem=..., temp=..., output=...) at js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1282 #4 0x57153163 in js::wasm::BaseCompiler::atomicRMW32<js::jit::Address> (temps=..., rd=..., rv=..., op=js::jit::AtomicFetchXorOp, srcAddr=..., access=..., this=0xf5d04ad4) at js/src/wasm/WasmBaselineCompile.cpp:4936 /snip For detailed crash information, see attachment. Setting s-s as I'm not sure how bad this is yet. This is the first result of hooking jsfunfuzz together with the binaryen fuzzer (see https://github.com/WebAssembly/binaryen/wiki/Fuzzing ).
Run: ./js --fuzzing-safe --no-baseline --no-ion --test-wasm-await-tier2 w2-out.wrapper w2-out.wasm
This seems 32-bit only.
autobisectjs shows this is probably related to the following changeset: The first bad revision is: changeset: b48d2197a9a0 user: Luke Wagner date: Fri Aug 24 13:41:13 2018 -0500 summary: Bug 1486027 - don't oom on failed ModuleGenerator memory reservations (r=bbouvier) Luke, is bug 1486027 a likely regressor?
Rather unlikely; before bug 1486027 the testcase's failure would be masked by an OOM failure. OOM in wasm baseline, so perhaps Lars could take a look when he's back from PTO.
Same problem as bug 1516738 really, failing to properly use a register with a byte personality. This one is a little more curious because I thought I had handled it properly, but I guess not...
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Priority: -- → P2
What sec rating should this have?
Do sec-high for now, it's hard to tell how it can be abused (if at all).
The reason this is failing is that I remembered to add byte register support to cmpxchg and xchg, but not to the rmw operations. Go figure... This is probably not sec-high because we don't yet ship atomics on-by-default, but I'd still like to uplift the eventual fix to beta so that there will be no surprises even for users that turn atomics on via about:config.
Repro instructions in full should be: cat the .001 and .002 files together into w2-out-wasm.zip unzip that file to produce w2-out.wasm append w2-out.wasm as an additional command line argument when running the tests it's sufficient to run with --no-wasm-ion I'm having a hard time producing a reduced test case so far. Anyway, I think this is not a real bug - I think it's an incorrect MOZ_ASSERT in the macroassembler, there's no reason why the register in question need be a byte register for the bitwise operations.
Move the assertion to where it belongs, see commit comment for more info. Ryan, can you open this bug? It's not s-s at all. (We maybe should uplift to beta just because it reduces the risk of false test failures; will ponder.)
Attachment #9033922 - Flags: review?(jseward)
Comment on attachment 9033922 [details] [diff] [review] bug1516720-move-overbroad-assertion.patch Review of attachment 9033922 [details] [diff] [review]: ----------------------------------------------------------------- Loosk OK.
Attachment #9033922 - Flags: review?(jseward) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/33c57870ca18 Make an overbroad assertion narrower. r=jseward
Comment on attachment 9033922 [details] [diff] [review] bug1516720-move-overbroad-assertion.patch [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: unknown User impact if declined: No user impact. We may run into this during testing however. Is this code covered by automated tests?: No Has the fix been verified in Nightly?: Yes Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): It restricts the scope of an assertion, making it less aggressive. Also, the assertion is only active in debug builds. String changes made/needed:
Attachment #9033922 - Flags: approval-mozilla-beta?
Comment on attachment 9033922 [details] [diff] [review] bug1516720-move-overbroad-assertion.patch [Triage Comment] Fixes an overly-aggressive assertion. Approved for 65.0b8.
Attachment #9033922 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.