Found with adresses.wast from the spec test. - out of bounds loads/stores should trap. - and there should be no wraparound on ptr + offset + accessSize computation. At the moment, I'm trying to understand how we determined the values of WasmImmediateRange and WasmCheckedImmediateRange: - WasmImmediateRange is 0x80000000, which is 2**31, that is, an int32 offset without the sign bit. As the address calculations use 32 bits arithmetic (even on x64, even though x64 has a 64 bits address calculation mode for MOV), this seems to mean any value in any direction so that the resulting offset fits in 32 bits. Makes sense. - WasmCheckedImmediateRange is less certain: it seems to have inherited the value of the minimum heap size, that is 4096 bytes, during ye olde asm.js times. But now the minimum heap size is PageSize and the checked immediate range is still 4096, so there might be some other reason. Or maybe we just haven't updated the value?

It's lower priority than implementing new opcodes and it will probably need some consequent changes to the code base, so deferring to later.

Actually, I had a few small cleanup patches already.

Note that in WasmSignalHandlers.cpp, the MOZ_RELEASE_ASSERT implies that the LHS of the inBounds check is redundant, even in release builds, and that's why we can remove it.

Comment on attachment 8746468 [details] [diff] [review] 1. Cleanup: pass down IsAtomic by hand to be more precise Review of attachment 8746468 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmIonCompile.cpp @@ -1517,5 @@ > uint32_t endOffset = access->endOffset(); > if (endOffset < offset) > return false; > bool accessNeedsBoundsCheck = true; > - // Assume worst case. Can you move this comment above 'accessNeedsBoundsCheck' (where it should've been)?

Comment on attachment 8746469 [details] [diff] [review] 2. Random cleanups Review of attachment 8746469 [details] [diff] [review]: ----------------------------------------------------------------- Some of these changes were actually made in the postorder patch before landing, so I'm afraid you'll have some rebase conflicts.

https://hg.mozilla.org/integration/mozilla-inbound/rev/638d7e8791c9 https://hg.mozilla.org/integration/mozilla-inbound/rev/e466d661a968

https://hg.mozilla.org/mozilla-central/rev/638d7e8791c9 https://hg.mozilla.org/mozilla-central/rev/e466d661a968

Taking back. The plan is the following: - implement WasmBoundsCheck, WasmStore, WasmLoad. For the two last ones, we can just throw on OOB. - we keep AsmJSLoad/StoreHeap, but we can move the SIMD and atomic loads/stores to WasmStore/WasmLoad, as they have throwing on oob semantics.

Thanks Benjamin! If it makes things easier, maybe good to start with pure-wasm loads/stores first and save SIMD/atomic for a follow-up patch (since these are not in wasm yet).

Current status: this patch splits HeapAccess into BoundsCheck and MemoryAccess. I'd like to work it a bit more for x86, because I think this will raise memory usage there: we used to store an instruction offset (load/store) and a uint8 delta for the comparison; with this patch, we store the instruction offset and the comparison offset (that is, 24 bytes more per access).

(In reply to Benjamin Bouvier [:bbouvier] from comment #10) And I meant bits, not bytes.

This patch just finishes the refactoring HeapAccess => {MemoryAccess, BoundsCheck} and optimizes for asm.js on x86. Some results on asm.js DeadTrigger2 on x86: - before the previous patch, there was ~8Mb of HeapAccesses; - with the previous patch, there was ~8Mb of MemoryAccesses + ~4Mb of BoundsCheck, since the comparison offset wasn't stored as a delta in the MemoryAccess but as an independent BoundsCheck structure. - with the current patch, it's back to ~8Mb of AsmJSMemoryAccesses, a specialized MemoryAccess that embeds the delta to the comparison (which is equivalent to what the HeapAccess did). So we're in a better shape. On x64 we don't have BoundsChecks, so this is fine; on ARM and MIPS, we only have BoundsChecks, no MemoryAccesses, so this is fine too. Posting this here for self-review before flipping to review.

On x86, the AsmJSMemoryAccess vs MemoryAccess distinction was not useful: after padding, both structures have the same size, so let's just embed the comparison delta in the MemoryAccess directly, on x86. This should be ready for review, but I'd prefer to use it for the wasm load/store/boundscheck to make sure I haven't forgotten anything.

This implements WasmLoad/Store/BoundsCheck. It's not entirely tested, but it passes basic tests, so asking for feedback here. This should be enough for starting work on folding MWasmBoundsChecks.

Rolled up patch, that applies cleanly on top of mozilla-inbound 300869:56b9bf9665b1.

Comment on attachment 8761290 [details] [diff] [review] 1.refactoring.patch Review of attachment 8761290 [details] [diff] [review]: ----------------------------------------------------------------- Great, looks like the right direction, but I think there is an alternative to the cmpOffset hack to avoid regressing x86 memory usage: ::: js/src/asmjs/WasmTypes.h @@ +590,4 @@ > { > uint32_t insnOffset_; > uint8_t opLength_; // the length of the load/store instruction > + uint8_t cmpDelta_; In the case of x86, we have no need of insnOffset_ directly; it's only used as a base for length/base patching. If the cmpOffset is in BoundsCheck, then it seems like we can store a single uint32 in MemoryAccess which is equivalent to (insnOffset_ + opLength_) today. So you'd make lookupMemoryAccess #ifdef ASMJS_MAY_USE_SIGNAL_HANDLERS (which is the only direct use of insnOffset). In that case, x86 can use BoundsCheck just like the other archs while avoiding regressing memory. ::: js/src/jit/x64/CodeGenerator-x64.cpp @@ +924,5 @@ > + masm.append(AsmJSMemoryAccess(before, wasm::MemoryAccess::CarryOn)); > +} > + > +static void > +MaybeAddBoundsCheck(MacroAssemblerX64& masm, MAsmJSHeapAccess* mir, Register ptr) How about 'MaybeAddAtomicsBoundCheck'?

Comment on attachment 8761292 [details] [diff] [review] 2.wasmloadstore.patch Review of attachment 8761292 [details] [diff] [review]: ----------------------------------------------------------------- Nice ::: js/src/jit/MIR.cpp @@ +4944,5 @@ > + if (!base()->isConstant() || !store->base()->isConstant()) > + return AliasType::MayAlias; > + > + const MConstant* otherBase = store->base()->toConstant(); > + if (base()->toConstant()->equals(otherBase) && offset() == store->offset()) I wonder whether it's worth defining congruentTo/mightAlias at all for wasm given that they cost compilation time and we're only picking up super-trivial stuff that a wasm generator should've already done itself. In particular, this mightAlias() can only determine that constant loads/stores don't alias. ::: js/src/jit/MIR.h @@ +14306,5 @@ > +}; > + > +class MWasmBoundsCheck > + : public MUnaryInstruction, > + public MAsmJSHeapAccess, Maybe time to rename to MWasmHeapAccess? @@ +14313,5 @@ > + explicit MWasmBoundsCheck(MDefinition* index, const MAsmJSHeapAccess& access) > + : MUnaryInstruction(index), > + MAsmJSHeapAccess(access) > + { > + setMovable(); Since the bounds check is precisely observable w.r.t stores, calls and other effects, I think we should start with non-movable. Then we can work on precisely defining when bounds checks can be moved. @@ +14351,5 @@ > + MAsmJSHeapAccess(access) > + { > + if (!(access.barrierBefore() | access.barrierAfter())) > + setMovable(); > + setGuard(); Do we need setGuard given that this operation is infallible? @@ +14388,5 @@ > + MWasmStore(const MAsmJSHeapAccess& access, MDefinition* base, MDefinition* value) > + : MBinaryInstruction(base, value), > + MAsmJSHeapAccess(access) > + { > + setGuard(); Also? ::: js/src/jit/x64/CodeGenerator-x64.cpp @@ +757,5 @@ > + Scalar::Type accessType = mir->accessType(); > + MOZ_ASSERT(!Scalar::isSimdType(accessType), "SIMD NYI"); > + > + MOZ_ASSERT(!mir->barrierBefore(), "atomic NYI"); > + MOZ_ASSERT(!mir->barrierAfter(), "atomic NYI"); In CodeGenerator-x86.cpp, you put both into a single MOZ_ASSERT (no \n after SIMD assert) which I think was a bit nicer. Actually, can we share more or all of the code between X86/X64?

Note, uses of setGuard() may be involved in tagging the operation (even a load) as "side-effecting", for the purposes of not optimizing across it for atomics. Hannes and I were supposed to discuss this in London, possibly more people want to involve themselves...

Comment on attachment 8761292 [details] [diff] [review] 2.wasmloadstore.patch Review of attachment 8761292 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +14315,5 @@ > + MAsmJSHeapAccess(access) > + { > + setMovable(); > + setGuard(); // Effectful: throws for OOB. > + setResultType(MIRType::Int32); It looks like MWasmBoundsCheck does not have a return value, so this type is incorrect. ::: js/src/jit/x86/CodeGenerator-x86.cpp @@ +454,5 @@ > + masm.breakpoint(); > + return; > + } > + > + canonicalizeIfDeterministic(accessType, value); There are spec tests (in float_memory.wast) which test that storing a NaN value preserves the NaN's bits, and it seems desirable to be able to run the tests in deterministic mode, so this canonicalization seems over-eager (And similar for X64's copy).

(In reply to Luke Wagner [:luke] from comment #17) > Comment on attachment 8761290 [details] [diff] [review] > 1.refactoring.patch > > Review of attachment 8761290 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great, looks like the right direction, but I think there is an alternative > to the cmpOffset hack to avoid regressing x86 memory usage: > > ::: js/src/asmjs/WasmTypes.h > @@ +590,4 @@ > > { > > uint32_t insnOffset_; > > uint8_t opLength_; // the length of the load/store instruction > > + uint8_t cmpDelta_; > > In the case of x86, we have no need of insnOffset_ directly; it's only used > as a base for length/base patching. If the cmpOffset is in BoundsCheck, > then it seems like we can store a single uint32 in MemoryAccess which is > equivalent to (insnOffset_ + opLength_) today. So you'd make > lookupMemoryAccess #ifdef ASMJS_MAY_USE_SIGNAL_HANDLERS (which is the only > direct use of insnOffset). In that case, x86 can use BoundsCheck just like > the other archs while avoiding regressing memory. Even better, thanks for the suggestion! Indeed there are no other direct uses for insnOffset(); I was worried about the insnOffset() getter (but it's only used in lookupMemoryAccess, that's defined #iff ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB in the patch, just declared otherwise) and the setInsnOffset() function (which was dead code). It's actually even simpler now and removes the outparam in emitAsmJSBoundsCheck*. Uploading the new patch now.

(In reply to Lars T Hansen [:lth] from comment #19) > Note, uses of setGuard() may be involved in tagging the operation (even a > load) as "side-effecting", for the purposes of not optimizing across it for > atomics. > > Hannes and I were supposed to discuss this in London, possibly more people > want to involve themselves... Yes, that's why I kept those: this marks instructions as non-removable by GVN as well. Maybe we should rename it to "effectful", or split it into two bits that have different meanings. A good discussion to have in London.

(In reply to Luke Wagner [:luke] from comment #18) > ::: js/src/jit/x64/CodeGenerator-x64.cpp > @@ +757,5 @@ > > + Scalar::Type accessType = mir->accessType(); > > + MOZ_ASSERT(!Scalar::isSimdType(accessType), "SIMD NYI"); > > + > > + MOZ_ASSERT(!mir->barrierBefore(), "atomic NYI"); > > + MOZ_ASSERT(!mir->barrierAfter(), "atomic NYI"); > > In CodeGenerator-x86.cpp, you put both into a single MOZ_ASSERT (no \n after > SIMD assert) which I think was a bit nicer. > > Actually, can we share more or all of the code between X86/X64? The intent is really good and I share it (pun unintended). Unfortunately, the operands don't look the same (x64 uses an effective address involving the HeapRegister) and the recorded MemoryAccesses are different (on x86 we record the offset after the actual load/store; on x64 before).

Comment on attachment 8762060 [details] [diff] [review] 3. refactoring.patch Review of attachment 8762060 [details] [diff] [review]: ----------------------------------------------------------------- Perfect, thanks! ::: js/src/asmjs/WasmModule.h @@ +266,5 @@ > const CodeRange* lookupCodeRange(void* pc) const; > + > +#ifdef ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB > + const MemoryAccess* lookupMemoryAccess(void* pc) const; > +#endif // ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB I'd leave off the #endif comment; seems clear without it :)

Update: this patch contains code for unaligned loads on ARM (see CodeGenerator-arm.cpp). This uses a pretty stupid byte per byte copying when (ptr + offset) doesn't satisfy the alignment requirements. Alternative strategies included: - call out to memcpy (but this has almost the same complexity as per byte copying, since we need to push all registers, etc.) - use signal handlers tricks (?) The same thing needs to be implemented for stores. Fortunately, most of the code could be shared there too.

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a134a50729fa split HeapAccess into MemoryAccess and BoundsCheck; r=luke

Folded WIP, applies cleanly on top of m-i 302074:e9723c6c6136. This fixes a few nasty x86 bounds checks issues, e.g. when the included offset is less than INT32_MAX but bigger than the actual heap size. I've also locally split the renaming MAsmJSHeapAccess -> MWasmMemoryAccess. The last part should be the ARM unaligned accesses and this should be all set for reviews.

Status report: I've found out about bug 1255695 earlier this afternoon (I was actually worried that on ARM, unaligned memory accesses wouldn't cause crashes, but that's expected in the simulator environment [1]). I am now buildling a toolchain to be able to build on a raspberry pi, so as to be able to test my patch against a real-world device. [1] http://searchfox.org/mozilla-central/source/js/src/jit/arm/Simulator-arm.cpp#1516-1519

So at this point, I've got what seems to be a correct implementation but there's only one issue. On ARM, unaligned accesses go to the signal handler, from which we try to retrieve values from the registers. For GPR registers, it's easy; but it seems that it is not trivial for FP registers, as stated by the long comment at [0]. This also recommends using ptrace with an enum value PTRACE_{G,S}ETFPREGS (gdb's code seems to use PTRACE_{G,S}ETVFPREGS, note the VFP regs instead of FP regs), but I am not sure this is the easiest approach, and I need to learn a bit more to experiment with this here. (although if this can work, we can probably rely on this for *all* platforms instead) Another alternative (suggested by :pipcet on irc) was to use a small stub of code that would do a simple memcpy and then a reinterpret_cast; in the signal handler and fake a call from the signalee to this stub (so that a subsequent return would continue just thereafter the initial load/store). Luke, Dan, what does seem to be the best solution here? Do you have any other idea about how to do this, considering we don't have direct access to the FP regs in the mcontext_t? [0] https://android.googlesource.com/platform/bionic/+/c124baa/libc/arch-arm/include/machine/ucontext.h#86

It's a good idea to generate stubs; I can see that working. But this is all rather a lot of work for, iiuc, a diminishing number of ARMv6 devices so maybe a better scheme is to (1) in the short-term disable wasm altogether (via wasm::HasCompilerSupport) as we do for softfp, (2) later emit all byte-loads (so no faults). This requires finding a reliable way to detect whether the current device faults on unaligned and *that* requires having some actual devices to test on. Perhaps we could just stick with the throw-on-unaligned behavior and fix problems once we have actual devices that repro and allow us to test?

Oh, I just saw comment 31. If you can test on a real device, that'd be fantastic to just fix this now. As for detecting support, perhaps this is as simple as testing the absence of HWCAP_ARMv7? This could also be tested with the simulator, I expect.

ARMv7 must reliably support unaligned FP load and store and we can test SCTRL.A to find out if the chip is so configured; seeing HWCAP_ARMv7 allows a JIT to handle this sanely and it never has to depend on the fault handler, it can always generate single accesses or a sequence of byte accesses. (Not supporting Wasm for ARMv6 seems like a rational decision. Even new lowish-end Android phones are ARMv8: http://arstechnica.co.uk/gadgets/2016/06/wileyfox-spark-price-specs-release-date/)

Comments from Dan posted on bug 1283121.

This is just a renaming patch: MAsmJSHeapAccess -> MWasmMemoryAccess.

A bunch of unrelated changes. Nice to have overall, so I've put them in a different patch so that they don't bother when reviewing actual features.

Validation changes: there is no such thing in the spec about "greater than alignment" errors (related: https://twitter.com/horse_wasm/status/724791533422043136). Also, the min memory must be less than the max memory (was tested in the memory.wast spec test).

This adds a (hopefully) temporary trap target for unaligned accesses: handling unaligned accesses on ARM is terrible, for reasons exposed in the follow-up bug. This creates the infra necessary to be able to use the unaligned access entry from the signal handler, later.

So this: 1) implements a very basic disassembler for ARM. The code is not beautiful, but I really reverse-engineered the way we write these instructions. I would like to defer improvements to this code in a follow-up, if that's fine. 2) uses this disassembler in the ARM simulator, at numerous places, to make sure it works correctly. Testing this can be done with ARM_HWCAP=vfp3,align js ./blablabla.js 3) installs a signal handler for misalignment issues (SIGBUS) and implements memory access emulation. Except that for all the reasons exposed before, on irc, and in the follow-up ARM bug, I've decided to just jump to the unaligned access target, when we hit this. Everything down to the switch in EmulateHeapAccess (included) has been tested, so this code could be kept in tree in the meanwhile; otherwise, extracting the patch could be done too with a little bit of effort.

And that's the actual disassembler patch. See previous comment.

This is the actual implementation of WasmLoad/Store/BoundsCheck on all platforms. I don't think there are special caveats about this patch that are not explained in comments, but let me know if you have any questions about it.

And some useful tests on our side.

Folded patch, if dbounov wants to base his work on something more recent: this applies cleanly on top of mozilla-inbound 303105:b7188125e731.

Comment on attachment 8766414 [details] [diff] [review] 6-random-changes.patch Review of attachment 8766414 [details] [diff] [review]: ----------------------------------------------------------------- Mmm, good stuff. ::: js/src/asmjs/WasmIonCompile.cpp @@ +622,5 @@ > + { > + MOZ_ASSERT(Scalar::isSimdType(access.accessType()), "non-SIMD loads should use loadHeap"); > + return loadHeapPrivate(base, access); > + } > + MDefinition* atomicLoadHeap(MDefinition* base, const MWasmMemoryAccess& access) For symmetry with the above, perhaps (load|store)AtomicHeap?

Comment on attachment 8766415 [details] [diff] [review] 7-validation.patch Review of attachment 8766415 [details] [diff] [review]: ----------------------------------------------------------------- It's not in AstSemantics.md, but it is in BinaryEncoding.md: https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#memory-related-operators-described-here (see the last bit about `memory_immediate`). In fact, the encoding depends on this so that the bits after can be used for other things in the future.

Comment on attachment 8766421 [details] [diff] [review] 10-actual-impl.patch Review of attachment 8766421 [details] [diff] [review]: ----------------------------------------------------------------- IIUC, the signal handler won't be hit at all here since wasm::HasCompilerSupport(cx) would be false on pre-ARMv7. IIUC, technically, unaligned access can fault on ARMv7, but according to https://codereview.chromium.org/1731013 this might not actually happen in practice (at least on Android) and we could always extend wasm::HasCompilerSupport(cx) to also check SCTLR.A if necessary. Given that, would it be ok to shelve the patch instead of landing it? Having dead code in the tree tends to rot anyway and the core bits you've written should still be salvageable if need be.

(Oops, that comment was intended for patches 8 and 9, not 10)

Comment on attachment 8766420 [details] [diff] [review] 9-disassembler-unaligned-signal-handlers.patch Review of attachment 8766420 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks good, though I'm concerned about what may be a spurious lookupMemoryAccess call in the case of a SIGBUS. If the code is as intended, please add a comment about why it's important to try both pc and pc - 4. ::: js/src/asmjs/WasmSignalHandlers.cpp @@ -141,5 @@ > -# define R10_sig(p) ((p)->uc_mcontext.gregs[REG_R10]) > -# define R11_sig(p) ((p)->uc_mcontext.gregs[REG_R11]) > -# define R12_sig(p) ((p)->uc_mcontext.gregs[REG_R12]) > -# define R13_sig(p) ((p)->uc_mcontext.gregs[REG_R13]) > -# define R14_sig(p) ((p)->uc_mcontext.gregs[REG_R14]) I guess these are moving to avoid conflicts with ARM; in that case, it'd make sense to move RAX_sig through RDI_sig too, since there's no point in defining those macros for ARM, even if they don't actually conflict. @@ +560,5 @@ > + > +MOZ_COLD static void* > +AddressOfGPRegisterSlot(EMULATOR_CONTEXT* context, Registers::Code code) > +{ > +# if not defined(JS_SIMULATOR_ARM) "not" is an alternate keyword for "!", but the rest of this file and most other code besides uses "!", so it'd be good to be consistent. @@ +613,5 @@ > + int8_t msb = AtomicOperations::loadSafeWhenRacy(addr.cast<uint8_t*>() + (size - 1)); > + memset(gp_reg, 0, sizeof(void*)); > + memset(gp_reg, msb >> 7, sizeof(int32_t)); > + AtomicOperations::memcpySafeWhenRacy(gp_reg, addr, size); > +} Several of these functions are identical to their x64 counterparts; would it be feasible to adjust the ifdefs so that this code is shared rather than duplicated? @@ +831,5 @@ > MOZ_COLD static uint8_t* > EmulateHeapAccess(EMULATOR_CONTEXT* context, uint8_t* pc, uint8_t* faultingAddress, > const MemoryAccess* memoryAccess, const Instance& instance) > { > + // TODO (bug 1283121) Implement complete heap accesse emulation for typo: accesse -> access @@ +1290,5 @@ > + // Different settings of the kernel can make that SIGBUS error are > + // sent on the instruction or the one thereafter (controlled by > + // /proc/cpu/alignment). > + static_assert(sizeof(js::jit::Instruction) == 4, "one instruction is four bytes"); > + memoryAccess = instance.lookupMemoryAccess(pc - 4); Do I understand this code correctly to mean that on a SIGBUS, the PC is set to the instruction after the access? If so, then it seems like we shouldn't be doing the earlier lookupMemoryAccess(pc) in the BusError case. @@ +1340,2 @@ > } > +# endif // XP_WIN || XP_DARWIN || assume unix Is this a convention? I was initially confused by this comment, thinking it meant that the code above the ifdef was guarded by something like "#if XP_WIN || XP_DARWIN || ...". @@ +1571,5 @@ > + return false; > + > + Disassembler::HeapAccess access; > + uint8_t* end = DisassembleHeapAccess(pc, &access); > + MOZ_ASSERT(end = recordedPC); Should this be a "=="? ::: js/src/jit/arm/Assembler-arm.cpp @@ +3388,5 @@ > + Instruction instr = *(Instruction*)ptr; > + uint32_t insn = instr.encode(); > + > + Assembler::Condition cc(instr.extractCond()); > + MOZ_ASSERT(cc == Assembler::Always, "other conditions than Always NYI"); The x86 disassembler code uses MOZ_RELEASE_ASSERTs exclusively because the code is both cold and "interesting"; I recommend doing so in this code as well (here and below).

Comment on attachment 8766421 [details] [diff] [review] 10-actual-impl.patch Review of attachment 8766421 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmIonCompile.cpp @@ +2085,3 @@ > uint32_t endOffset = offset + access->byteSize(); > + > + MOZ_ASSERT_IF(endOffset < offset, f.mg().kind == ModuleKind::Wasm); I think you can go one further and have: MOZ_ASSERT_IF(offset, f.mg().kind == ModuleKind::AsmJS); Since asm.js (in WriteArrayAccessFlags) always has a 0 offset; offset folding for asm.js is done later (during EAA). If that's right, then I think this whole function can be removed and all callers can simply have access.setOffset(addr.offset) (or maybe add it to the MWasmMemoryAccess ctor like you did with alignment). ::: js/src/jit/x64/Lowering-x64.cpp @@ +157,5 @@ > > void > +LIRGeneratorX64::visitWasmBoundsCheck(MWasmBoundsCheck* ins) > +{ > + // x64 uses signal handling, not explicit bounds checks. Nothing to do here! What about when signal handling is disabled? Given that we haven't seen it ever not be available on x64, I'd be fine removing that mode altogether; it was mostly useful before the when we used mprotect() to interrupt Ion code so you'd otherwise get faults in gdb all the time. Assuming that, could we avoid creating the MWasmBoundsCheck altogether (to optimize compilation)? ::: js/src/jit/x86/Lowering-x86.cpp @@ +211,5 @@ > + add(lir, ins); > +} > + > +void > +LIRGeneratorX86::visitWasmLoad(MWasmLoad* ins) I think this function could be hoisted into Lowering-x86-shared.cpp.

Comment on attachment 8766421 [details] [diff] [review] 10-actual-impl.patch Review of attachment 8766421 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/x86/CodeGenerator-x86.cpp @@ +398,5 @@ > +CodeGeneratorX86::visitWasmBoundsCheck(LWasmBoundsCheck* ins) > +{ > + const MWasmBoundsCheck* mir = ins->mir(); > + > + if (mir->offset() > INT32_MAX) { Could you add a comment here mentioning why we don't support offsets greater than INT32_MAX? The bounds-check code itself can support it, since it does an unsigned comparison. I think it's because we don't support ArrayBuffers that big.

(In reply to Luke Wagner [:luke] from comment #47) > Comment on attachment 8766415 [details] [diff] [review] > 7-validation.patch > > Review of attachment 8766415 [details] [diff] [review]: > ----------------------------------------------------------------- > > It's not in AstSemantics.md, but it is in BinaryEncoding.md: > > https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#memory- > related-operators-described-here > (see the last bit about `memory_immediate`). In fact, the encoding depends > on this so that the bits after can be used for other things in the future. Haha, discrepancy spotted: https://github.com/WebAssembly/spec/blob/master/ml-proto/test/memory.wast#L49-L52 Will open a PR against ml-proto.

(In reply to Dan Gohman [:sunfish] from comment #50) > Comment on attachment 8766420 [details] [diff] [review] > 9-disassembler-unaligned-signal-handlers.patch > > Review of attachment 8766420 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall this looks good, though I'm concerned about what may be a spurious > lookupMemoryAccess call in the case of a SIGBUS. If the code is as intended, > please add a comment about why it's important to try both pc and pc - 4. Well, there is one, it is probably not clear enough though. > @@ +1290,5 @@ > > + // Different settings of the kernel can make that SIGBUS error are > > + // sent on the instruction or the one thereafter (controlled by > > + // /proc/cpu/alignment). > > + static_assert(sizeof(js::jit::Instruction) == 4, "one instruction is four bytes"); > > + memoryAccess = instance.lookupMemoryAccess(pc - 4); > > Do I understand this code correctly to mean that on a SIGBUS, the PC is set > to the instruction after the access? If so, then it seems like we shouldn't > be doing the earlier lookupMemoryAccess(pc) in the BusError case. The comment just above this code says: // Different settings of the kernel can make that SIGBUS error are // sent on the instruction or the one thereafter (controlled by // /proc/cpu/alignment). This is what happened on my raspberry pi, at least: say the instruction doing the unaligned access is at pc N. if /proc/cpu/alignment is set to 2 (fixup), the SIGBUS would happen at pc N; if /proc/cpu/alignment is set to 3 (signal), the SIGBUS would happen at pc N + 4. So I had to take this into account. I think this is problematic if there are several accesses following each other, such that the two adjacent instructions are both memory accesses. In this case, one of these lookups could be wrong. This patch is probably going to be shelved anyways, but a workaround here could be to insert a NOP just after the load/store, to be sure we're not getting the wrong memory access when looking it up.

(In reply to Luke Wagner [:luke] from comment #48) > Comment on attachment 8766421 [details] [diff] [review] > 10-actual-impl.patch > > Review of attachment 8766421 [details] [diff] [review]: > ----------------------------------------------------------------- > > IIUC, the signal handler won't be hit at all here since > wasm::HasCompilerSupport(cx) would be false on pre-ARMv7. IIUC, > technically, unaligned access can fault on ARMv7, but according to > https://codereview.chromium.org/1731013 this might not actually happen in > practice (at least on Android) and we could always extend > wasm::HasCompilerSupport(cx) to also check SCTLR.A if necessary. Given > that, would it be ok to shelve the patch instead of landing it? Having dead > code in the tree tends to rot anyway and the core bits you've written should > still be salvageable if need be. I need to check on an ARMv7 device (just realized my phone is one), but reading the comment in the link suggests that: 1. SCTLR.A is 0 by default, 2. when it is set to 0 by default, *some* instructions can be misaligned, but not all. Our ARM codebase is such that I can't tell whether we're using some instructions that are not affected by SCTLR.A. So I need to test whether all the instructions we use for WasmLoad/WasmStore allow unaligned accesses on a phone.

(In reply to Benjamin Bouvier [:bbouvier] from comment #54) > (In reply to Dan Gohman [:sunfish] from comment #50) > > Comment on attachment 8766420 [details] [diff] [review] > > 9-disassembler-unaligned-signal-handlers.patch > > > > Review of attachment 8766420 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Overall this looks good, though I'm concerned about what may be a spurious > > lookupMemoryAccess call in the case of a SIGBUS. If the code is as intended, > > please add a comment about why it's important to try both pc and pc - 4. > > Well, there is one, it is probably not clear enough though. > > > @@ +1290,5 @@ > > > + // Different settings of the kernel can make that SIGBUS error are > > > + // sent on the instruction or the one thereafter (controlled by > > > + // /proc/cpu/alignment). > > > + static_assert(sizeof(js::jit::Instruction) == 4, "one instruction is four bytes"); > > > + memoryAccess = instance.lookupMemoryAccess(pc - 4); > > > > Do I understand this code correctly to mean that on a SIGBUS, the PC is set > > to the instruction after the access? If so, then it seems like we shouldn't > > be doing the earlier lookupMemoryAccess(pc) in the BusError case. > > The comment just above this code says: > > // Different settings of the kernel can make that SIGBUS error are > // sent on the instruction or the one thereafter (controlled by > // /proc/cpu/alignment). > > This is what happened on my raspberry pi, at least: say the instruction > doing the unaligned access is at pc N. if /proc/cpu/alignment is set to 2 > (fixup), the SIGBUS would happen at pc N; if /proc/cpu/alignment is set to 3 > (signal), the SIGBUS would happen at pc N + 4. So I had to take this into > account. Oh, wow. > I think this is problematic if there are several accesses following each > other, such that the two adjacent instructions are both memory accesses. In > this case, one of these lookups could be wrong. This patch is probably going > to be shelved anyways, but a workaround here could be to insert a NOP just > after the load/store, to be sure we're not getting the wrong memory access > when looking it up. Makes sense.

Fwiw, I confirm that ARMv7 still disallow unaligned floating-point accesses; just tested on my ARMv7 processor phone. My current course of action, as discussed yesterday f2f, will be the following: - shelve the disassembler patch and related use in the signal handler code. - keep the signal handler code and the throw exit for misaligned accesses - in a follow-up bug (bug 1283126), take alignment hints into account so that each unaligned access generates byte loads getting combined.

One avenue that seems fruitful to explore is that if the chip has NEON, then VLD1 may be used to load unaligned floating point data (instead of VLDR). VLDn obeys the SCTRL.A bit. I confess I'm a little bit of a neophyte here and there are several variants of VLD1, and there may be performance penalties, and who knows how prevalent NEON is anyway, but it might still beat loading with 2xLDR and moving to the FPU.

Same patch with the validation change, disallowing having a memory max size lower than the initial size (as in the ml-proto). (noted that you said yesterday that in the future, this might not be enforced anymore, but this is also tested in the spec; if this is wrong, so the spec needs another spec bug) And filed https://github.com/WebAssembly/spec/issues/302 for accesses with greater than natural alignment.

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/25d979f7c932 Fix the use of -g for wasm test cases; r=lth https://hg.mozilla.org/integration/mozilla-inbound/rev/38c84afa8cde Rename MAsmJSHeapAccess to MWasmMemoryAccess; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/d1abee3e755d Unrelated changes; r=luke

https://hg.mozilla.org/mozilla-central/rev/25d979f7c932 https://hg.mozilla.org/mozilla-central/rev/38c84afa8cde https://hg.mozilla.org/mozilla-central/rev/d1abee3e755d

This adds the trap for misaligned accesses, not making use of it yet.

Drive-by ARM changes.

Minimalistic infra to catch SIGBUS (misaligned accesses) in the signal handlers.

Actual implementation of wasm loads/stores. (In reply to Luke Wagner [:luke] from comment #51) > Comment on attachment 8766421 [details] [diff] [review] > 10-actual-impl.patch > > Review of attachment 8766421 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/asmjs/WasmIonCompile.cpp > @@ +2085,3 @@ > > uint32_t endOffset = offset + access->byteSize(); > > + > > + MOZ_ASSERT_IF(endOffset < offset, f.mg().kind == ModuleKind::Wasm); > > I think you can go one further and have: > MOZ_ASSERT_IF(offset, f.mg().kind == ModuleKind::AsmJS); > Since asm.js (in WriteArrayAccessFlags) always has a 0 offset; offset > folding for asm.js is done later (during EAA). > > If that's right, then I think this whole function can be removed and all > callers can simply have access.setOffset(addr.offset) (or maybe add it to > the MWasmMemoryAccess ctor like you did with alignment). Yes indeed: it could have been much worse, since in AsmJS code, the MAdd that could be inserted would have had an offset of 0, resulting in a no-op; but this path was never executed. > ::: js/src/jit/x64/Lowering-x64.cpp > @@ +157,5 @@ > > > > void > > +LIRGeneratorX64::visitWasmBoundsCheck(MWasmBoundsCheck* ins) > > +{ > > + // x64 uses signal handling, not explicit bounds checks. Nothing to do here! > > What about when signal handling is disabled? Given that we haven't seen it > ever not be available on x64, I'd be fine removing that mode altogether; it > was mostly useful before the when we used mprotect() to interrupt Ion code > so you'd otherwise get faults in gdb all the time. Assuming that, could we > avoid creating the MWasmBoundsCheck altogether (to optimize compilation)? So using signal handling doesn't depend only on signal handling being enabled/disabled (viz. not broken), but also on the system's memory page size being a multiple of a Wasm page's size. Considering we can just not emit the WasmBoundsCheck if we use signal handling for OOB, I've kept it simple in this patch and added support for explicit bounds checks on x64. Fortunately, the code can be reused on x86, so the impl of visitWasmBoundsCheck can be hoisted up in CodeGenerator-x86. (The alternative is, on x64, to disable wasm if the preconditions to using signal handlings for OOB are not met: signal handling not broken + preconditions on page's sizes. Then we can just remove JS_NO_SIGNALS as well and all traces of this in the code, namely in mach's code, etc.) > > ::: js/src/jit/x86/Lowering-x86.cpp > @@ +211,5 @@ > > + add(lir, ins); > > +} > > + > > +void > > +LIRGeneratorX86::visitWasmLoad(MWasmLoad* ins) > > I think this function could be hoisted into Lowering-x86-shared.cpp. Yes indeed.

So the unaligned accesses handling path is never triggered on the ARM simulator, so the tests can remain the same. Maybe it's unfortunate and we should force the ARM simulator into testing that the wasm::MemoryAccess is found, at least? That would not make it depend on the disassembler at all.

Let's shelve the ARM disassembler patch. For what it's worth, I thought we could use this for non-FPU memory accesses; but with the new ARM precondition (that is, ARMv7 being used and set to the default behavior), there's no way an integer memory access will trigger a signal and thus the signal handler. So this would have been dead code! Before I realized this, I made disassembling bit-perfect: when we disassemble any load/store on ARM, we now have control on every single bit of the 32 bits composing the ARM instruction itself. A nice lesson of ARM assembly encoding.

Comment on attachment 8768041 [details] [diff] [review] 10-catch-sigbus.patch Review of attachment 8768041 [details] [diff] [review]: ----------------------------------------------------------------- Talking to Dan today, given that we check that (1) pc is in wasm code, (2) the faulting address is in the wasm heap, perhaps we don't even need to have *any* MemoryAccess for ARM (or, with some tweaks, x64). It's fine as is now, but I think you could delete some parts of this patch (and that `pc - 4` hack) if you wanted.

Comment on attachment 8768046 [details] [diff] [review] 11-impl.patch Review of attachment 8768046 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks! It's unfortunate we have to do the slow instructions for ARM, but I think we can improve upon that (in a later patch) by using signal handling to round up the allocation (to an Operand2-friendly immediate) and add a guard page to catch (small-enough) offsets so there is no need to subtract an offset from the heap length. ::: js/src/asmjs/WasmIonCompile.cpp @@ +598,5 @@ > + MInstruction* load = nullptr; > + if (mg().kind == ModuleKind::Wasm) { > + if (!mg().args.useSignalHandlersForOOB) { > + auto* boundsCheck = MWasmBoundsCheck::New(alloc(), base, access); > + curBlock_->add(boundsCheck); nit: I think this could be a single-liner, and drop the {} @@ +617,5 @@ > + > + MInstruction* store = nullptr; > + if (mg().kind == ModuleKind::Wasm) { > + auto* boundsCheck = MWasmBoundsCheck::New(alloc(), base, access); > + curBlock_->add(boundsCheck); Can this also be `if (!mg().args.useSignalHandlersForOOB)`? (and a one liner too) ::: js/src/jit/arm/CodeGenerator-arm.cpp @@ +2265,5 @@ > + // about the kernel settings). > + masm.nop(); > + > + if (byteSize) > + masm.append(wasm::MemoryAccess(after - sizeof(Instruction))); If you remove the MemoryAccess() (as suggested in the previous comment), then this nop could go as well. ::: js/src/jit/x64/Lowering-x64.cpp @@ +155,5 @@ > define(lir, ins); > } > > void > +LIRGeneratorX64::visitWasmBoundsCheck(MWasmBoundsCheck* ins) I think this lowering function also be moved to Lowering-x86-shared.cpp (it's ok for x86 to test gen->needsBoundsCheckBranch(ins)).

Comment on attachment 8768041 [details] [diff] [review] 10-catch-sigbus.patch Review of attachment 8768041 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmSignalHandlers.cpp @@ +1160,5 @@ > + static_assert(sizeof(js::jit::Instruction) == 4, "one instruction is four bytes"); > + memoryAccess = instance.lookupMemoryAccess(pc - 4); > + if (!memoryAccess) > + return false; > + } My impression from comment 54 this part of the patch was going to be shelved for now; if it's going to stay, will there be nops between accesses to prevent accessing an adjacent access here? @@ +1348,5 @@ > + busHandler.sa_flags = SA_SIGINFO | SA_NODEFER; > + busHandler.sa_sigaction = &AsmJSFaultHandler<Signal::BusError>; > + sigemptyset(&busHandler.sa_mask); > + if (sigaction(SIGBUS, &busHandler, &sPrevSIGBUSHandler)) > + MOZ_CRASH("unable to install sigbus handler"); If I read this correctly, it's installing the signal handler for all architectures, right? It seems like it'd be desirable to avoid registering a SIGBUS handler on architectures where we don't expect SIGBUS faults for asm.js/wasm code, such as x86/x64.

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d21d1aa42342 Don't allow the memory's max size to be lower than the initial size; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/7028215a4aaf Add an unaligned access trap; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c67b926f98 Unrelated changes in the ARM backend; r=luke

Carrying forward r=luke. Diff: - we don't need wasm::MemoryAccess at all for ARM since we know PC is in wasm code *and* the crashing SIGBUS is in the heap range. This also removes the `pc - 4` hack (there were no-ops inserted in the next patch). - set up the signal handlers only when we need them. We assume unaligned accesses i.e. ARM is Linux-only; we might want to change that in the future.

Comment on attachment 8768394 [details] [diff] [review] 10-catch-sigbus.patch Review of attachment 8768394 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!

https://hg.mozilla.org/mozilla-central/rev/d21d1aa42342 https://hg.mozilla.org/mozilla-central/rev/7028215a4aaf https://hg.mozilla.org/mozilla-central/rev/f1c67b926f98

During tests, on win64 opt, we VirtualAlloc memory faster than we free it. A workaround is that after each instantiation of an mmapped Wasm array, we force a GC to release previously dormant VirtualAlloc'd memory (attached to dead wasm modules).

Comment on attachment 8768836 [details] [diff] [review] 13. force-triggering-gc-after-mmap.patch Review of attachment 8768836 [details] [diff] [review]: ----------------------------------------------------------------- Nice job tracking this down!

Finally, the time has come: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d5cc271f094

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d23fa9b95430 Minimal signal handling features to catch and reject SIGBUS; r=luke, r=sunfish https://hg.mozilla.org/integration/mozilla-inbound/rev/6a6dc5483468 Baldr: implement WasmBoundsCheck/Load/Store and semantics; r=luke, r=sunfish https://hg.mozilla.org/integration/mozilla-inbound/rev/51492e49def4 Tests; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/5010bd1c3b01 Force triggering a GC after creating a mmapped array; r=luke

https://hg.mozilla.org/mozilla-central/rev/d23fa9b95430 https://hg.mozilla.org/mozilla-central/rev/6a6dc5483468 https://hg.mozilla.org/mozilla-central/rev/51492e49def4 https://hg.mozilla.org/mozilla-central/rev/5010bd1c3b01