Closed Bug 1255695 Opened 9 years ago Closed 9 years ago

wasm: don't mask load/store addresses

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(1 file)

Attached patch wasm-unaligned.patch (deleted) — Splinter Review
WebAssembly doesn't need alignment masks! This patch removes them, adds support to the ARM simulator for specially handling unaligned loads and stores, and adds a signal handler on ARM to catch unaligned access traps. In the current patch, the signal handler throws an OutOfBounds exception, which is sufficient to support code translated from asm.js which never does unaligned accesses, however it'll need to do more for full wasm conformance. I don't presently have a suitable ARM device to test this patch on. A try run[0] confirms that it builds (the orange seems unrelated), however try runs do not appear to run jit-tests, so I've not yet actually tested the patch. [0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fb0dc1edc50&selectedJob=17922383
Comment on attachment 8729339 [details] [diff] [review] wasm-unaligned.patch Review of attachment 8729339 [details] [diff] [review]: ----------------------------------------------------------------- We've tested general segfault signal handling on ARM before so we know that path works so reusing existing functionality to redirect to the oob handler seems like a safe step. We'll obviously need to get a real device to test on before shipping and to test the proper unaligned-access handling. Ship it! ::: js/src/asmjs/WasmSignalHandlers.cpp @@ +778,2 @@ > // These checks aren't necessary, but, since we can, check anyway to make > // sure we aren't covering up a real bug. Could you instead tighten the below assert to require faultingAddress to be within module.heap() + module.heapLength() (instead of MappedSize) here and below x2? @@ +1384,5 @@ > #endif > } > + > +bool > +js::wasm::isPCInWasmCode(void *pc) Nit: bool wasm::IsPCInWasmCode ::: js/src/asmjs/WasmSignalHandlers.h @@ +66,5 @@ > #endif > > +// Test whether the given PC is within the innermost wasm activation. Return > +// false if it is not, or it cannot be determined. > +bool isPCInWasmCode(void *pc); Nit: bool IsPCInWasmCode(void* pc);
Attachment #8729339 - Flags: review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: