I was interested to see how much WasmSignalHandler.cpp could be simplified by removing this one particular asm.js constant-offset-folding optimizations (which wasm doesn't need b/c of non-wraparound offsets) and I realized that most simplifications are significant if we simply remove asm.js use of the signal handlers altogether: 1837 lines removed (and the majority isn't just removing the x86 decompiler). This patch removes a bunch of complexity and probably some more in a follow-up. Practically speaking, deployed asm.js apps have to deal with 2x worse perf due to Chrome/Safari, so the small perf hit due to removing the signal-handler optimizations on x64 shouldn't be a major problem for anyone. It's also good to give people more of a reason to switch to wasm (in preparation for later total asm.js removal).

With the other use of LookupFaultingInstance() gone, it's possible to rename and repurpose this now-simulator-only function to factor out a ton of code from our 4 simulators. Once I started doing that, I realize that the illegal-instruction fault handling could be factored out too, so I did that.

That removes an impressive amount of code \o/

Comment on attachment 9013041 [details] [diff] [review] simplify-signal-handling Review of attachment 9013041 [details] [diff] [review]: ----------------------------------------------------------------- Generally I'm in favor of this, though as I said on the call yesterday I am worried that some users will experience significant regressions in content that's important to them but about which we've never heard. I am probably too pessimistic, but I just don't know. An experiment that switches to explicit bounds checks would not be costly to do but would take a long time to yield results... :-/ ::: js/src/wasm/WasmSignalHandlers.cpp @@ +396,5 @@ > } > > +// ============================================================================= > +// All signals/exceptions funnel down to this one trap-handling function which > +// tests whether the pc is in a wasm module and, if so, whether there is actuall "actually" @@ +562,5 @@ > return false; > } > } > > + Spurious space at start of line.

Comment on attachment 9013409 [details] [diff] [review] factor-simulator-fault-code Review of attachment 9013409 [details] [diff] [review]: ----------------------------------------------------------------- I approve of this cleanup :)

Comment on attachment 9013041 [details] [diff] [review] simplify-signal-handling Review of attachment 9013041 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: js/src/jit/MIR.h @@ +13559,1 @@ > MDefinition* memoryBase() const { return getOperand(memoryBaseIndex_); } Can you assert here hasMemoryBase()? @@ +13615,1 @@ > MDefinition* memoryBase() const { return getOperand(memoryBaseIndex_); } ditto ::: js/src/jit/x86-shared/Lowering-x86-shared.cpp @@ +296,5 @@ > + MDefinition* boundsCheckLimit = ins->boundsCheckLimit(); > + MOZ_ASSERT_IF(ins->needsBoundsCheck(), boundsCheckLimit->type() == MIRType::Int32); > + > + // For simplicity, require a register if we're going to emit a bounds-check > + // branch, so that we don't have special cases for constants. I think the reason behind this was that these bounds checks only happened on x86; maybe we could implement the constant special cases now to lower register allocation and not regress too much? @@ +343,5 @@ > + case Scalar::Int16: case Scalar::Uint16: > + case Scalar::Int32: case Scalar::Uint32: > + case Scalar::Float32: case Scalar::Float64: > + // For now, don't allow constant values. The immediate operand affects > + // instruction layout which affects patching. Uh, we don't patch code anymore, so we could just remove this comment.

Comment on attachment 9013409 [details] [diff] [review] factor-simulator-fault-code Review of attachment 9013409 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/src/jit/mips32/Simulator-mips32.h @@ +318,5 @@ > + if (!js::wasm::MemoryAccessTraps(registerState(), (uint8_t*)addr, numBytes, &newPC)) { > + return false; > + } > + > + set_pc(int32_t(newPC)); nit: set LLBit_ to false here ::: js/src/wasm/WasmSignalHandlers.cpp @@ +947,5 @@ > + const wasm::ModuleSegment& segment = *codeSegment->asModule(); > + > + Trap trap; > + BytecodeOffset bytecode; > + if (!segment.code().lookupTrap(regs.pc, &trap, &bytecode)) { Not sure, but don't we need to check the trap isn't OutOfBounds here?

(In reply to Benjamin Bouvier [:bbouvier] from comment #5) > > + // For simplicity, require a register if we're going to emit a bounds-check > > + // branch, so that we don't have special cases for constants. > > I think the reason behind this was that these bounds checks only happened on > x86; maybe we could implement the constant special cases now to lower > register allocation and not regress too much? Good though, but as I was implementing this (which would require generalizing MacroAssembler::wasmStore() to take either a register or constant), I remember why this wasn't done in the first place: asm.js implicitly grows the minimum heap size when accessed by syntactic constants, so this case would only arise due to GVN producing a constant. (Commented accordingly now.) FWIW, while investigating this, I did notice that I had unnecessarily brought in the "x86 store requires useFixed(ins->value(), eax)" constraint, so I conditionalized that on #ifdef JS_CODEGEN_X86.

(In reply to Benjamin Bouvier [:bbouvier] from comment #6) > Not sure, but don't we need to check the trap isn't OutOfBounds here? No, because when we fall back on a dynamic branch, we actually use masm.wasmTrap() which triggers OOB via illegal instruction.

Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1094983384c Baldr: don't use signal handlers for asm.js bounds checks (r=lth,bbouvier) https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6344a8c25c Baldr: factor out trap-handling code from simulators (r=lth,bbouvier)

https://hg.mozilla.org/mozilla-central/rev/d1094983384c https://hg.mozilla.org/mozilla-central/rev/8b6344a8c25c