The Google Pixel 2 traps on unaligned FP accesses in ARMv7 mode, see bug 1517351 comment 15 et seq. THat hardware is mainstream enough that we probably have to deal with this problem. Increasing priority.

ARM-32 failures on Pixel 2 and Moto G5:

wasm/spec/address.wast.js fails because of an unaligned f32 load
wasm/spec/call_indirect.wast.js fails because of an unaligned f64 store
wasm/spec/memory_redundancy.wast.js fails because of an unaligned f32 load
wasm/spec/float_memory.wast.js fails because of an unaligned f32 load

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc15a1b5c336927fb3987fb9314f59b66204425&selectedJob=221078434

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4188c079bd4a2c8f10236333f717c4752d654930

These tests are all currently disabled on ARM for this reason.

Fairly grubby code to emulate trapping FP loads. Completely untested, not even test compiled, but everything used here is part of the Linux ABI in principle, so this is not entirely insane.

This compiles but does not run (it segfaults w/o much information when it should have emulated the instruction). Hard to debug locally unless you have a device that traps on unaligned FP accesses...

Wohoo, I have a device that has the problem.

(This is a very mainstream system, BTW, a Samsung Galaxy Tab S2, so the problem with trapping unaligned FP accesses appears to be widespread in the wild.)

Also my LG/Google Nexus 5x with Android 8.1. I think at this point the inescapable conclusion is that this problem is common or possibly even the majority case.

Simple test case (temporary home): https://axis-of-eval.org/sandbox/oob-arm.html, runs quickly and the result is reported in the window.

Works locally on my Nexus 5X. Needs:

• cleanup & maybe generalization
• test cases
• testing on various android versions (the VFP_MAGIC may have changed at
  various points in the past)
• scrutiny
• maybe additional ifdeffery so that we don't expose ARM Linux to it, just
  Android

This is green on the Pixel 2 on try with my new test cases and after reinstating the test cases that were disabled. (https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b21e231d93f13c7abde89a02f1542cf6f082ec0)

Primary worries for reviews:

• Not enough guards / risk of double-faulting because I've forgotten to check some precondition. I'm pretty sure I got "this is a known wasm instance with a PC that is for sure valid and a memory address that will for sure be in-bound when I read the data" right, but is there more?
• Unnecessary kernel dependencies
• Overbroad kernel assumptions
• "Unknown unknowns"

Bob, can you put a couple of Moto G5s back in the pool for me to use? I'll want to run jittest-10 in both opt and debug mode, just to be sure.

Done, I am on pto today. If you can kick these off soon before I have to leave, we can make sure they are working properly. Otherwise I'll check in around 9AM PT.

(In reply to Julian Seward [:jseward] from comment #26)

Comment on attachment 9036554 [details] [diff] [review]
bug1283121-emulate-fp-accesses-v4.patch

Review of attachment 9036554 [details] [diff] [review]:

r+ if at least |pc| is asserted to be 0 % 4.

::: js/src/wasm/WasmInstance.cpp
@@ +1143,5 @@

      lastByteOffset < memoryMappedSize();

}

+bool Instance::memoryAccessInBounds(uint8_t* addr,

•                                unsigned numBytes) const {
  

Does this correctly handle (reject) the case where [addr, +numBytes) wraps
around the end of the address space? I asumme that a wasm memory itself
can't wrap around the end of the address space.

The wasm memory can't wrap but it's possible this method should be more paranoid. Will investigate.

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +573,5 @@

+#if defined(linux) && defined(arm)

• // wasmLoadImpl() and wasmStoreImpl() in MacroAssembler-arm.cpp emit plain
• // unconditional VLDR and VSTR instructions that do not use the PC as the
• // base register.
• uint32_t instr = (uint32_t)pc;

This appears to assume that |pc| refers to an ARM-encoded instruction.

Yes.

Please assert this; possibly release-assert so that we can't end up doing
memory accesses on behalf of who-knows-what Thumb instruction by accident.

I can assert pc % 4 == 0 at most and will do that, just because. For the instructions of interest, Arm and Thumb have the same encoding (the only difference is that Arm allows the instruction to be conditional), so further tests on arm-vs-thumb are not interesting.

@@ +580,5 @@

• bool isVSTR = masked == 0x0D000A00;
• DebugOnly<bool> oob = true;
• if (isVLDR || isVSTR) {

•  bool isUnconditional = (instr >> 28) == 0xE;
  

•  MOZ_RELEASE_ASSERT(isUnconditional);
  

To release-assert this (and below) is to imply that we can only get here as
a result of a fault with a wasm-jit generated instruction.

That is what the earlier guards are supposed to guarantee - that the faulting instruction is in a wasm code segment.

If we take a
fault elsewhere in Gecko and end up in here, the release assert could cause
the system to fail unnecessarily. So do we have that guarantee? If not, it
would seem safer to simply ignore unexpected instructions.

If we fail the earlier guards then we return false and the superstructure in this file will invoke the next handler.

Memo to self: in JSContext.cpp there is code that checks for certain Android versions (to disable the JIT in some cases). It might be possible to reduce the risk of going wrong in the instruction emulation by checking the OS version and only trying to emulate if the OS is new "enough".

(In reply to Lars T Hansen [:lth] from comment #24)

• Not enough guards / risk of double-faulting because I've forgotten to check some precondition. I'm pretty sure I got "this is a known wasm instance with a PC that is for sure valid and a memory address that will for sure be in-bound when I read the data" right, but is there more?

That seems sufficient to me. Note that double-faulting is uniformly and conservatively handled by sAlreadyHandlingTrap; if you have a bug here it'll just end in a proper crash report.

(In reply to Luke Wagner [:luke] from comment #29)

Comment on attachment 9036554 [details] [diff] [review]
bug1283121-emulate-fp-accesses-v4.patch

Great feedback, thanks. Some followup questions:

@@ +578,5 @@

• uint32_t masked = instr & 0x0F300E00;
• bool isVLDR = masked == 0x0D100A00;
• bool isVSTR = masked == 0x0D000A00;
• DebugOnly<bool> oob = true;
• if (isVLDR || isVSTR) {

It seems like we should be able to release assert it's one of these two
since we know we are at a wasm-generated trap for an out-of-bounds. If it's
false, that means we're generating a new form of OOB-trapping load/store and
need to add a new pattern here.

That is a little risky. We know there may be device configurations that also trap on unaligned integer loads and stores, even if we haven't seen any, and by my later comment I think it would be better to just throw a range error if we encounter one of those.

@@ +631,5 @@

• if (!oob) {

•  // If we get to this point, we did not understand the instruction or we
  

•  // could not access the FP register.  This is probably a bug / missing
  

•  // feature or an incompatibility with the kernel.  It is best to MOZ_CRASH
  

•  // only in debug builds and throw an OOB error in release builds.
  

I like this logic. I wonder if the __android_log_print() is necessary
though: the MOZ_CRASH() will __android_log_print() its string before
crashing and the instruction is asserted to be fine; the only way to reach
here is that we're not able to find the f32/f64 register in WriteFPR*.

As above: it's here in case we encounter a fault from unaligned integer access.

But as you point out, having moved the code into a function makes it easier to create better logic around these things, and I think I see a good compromise here.

(In reply to Lars T Hansen [:lth] from comment #31)

It seems like we should be able to release assert it's one of these two
since we know we are at a wasm-generated trap for an out-of-bounds. If it's
false, that means we're generating a new form of OOB-trapping load/store and
need to add a new pattern here.

That is a little risky. We know there may be device configurations that also trap on unaligned integer loads and stores, even if we haven't seen any, and by my later comment I think it would be better to just throw a range error if we encounter one of those.

Ah hah; I hadn't thought about trapping integer loads. You're right; it would be best if we trapped in those cases.

Updated after reviews. Green in opt and debug on pixel 2 and moto g5. Will land it after a more general try run.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7e0aca6faf936cf877722d0421acfbbdcbf5369

Bob, you should be able to reclaim the G5s now. Thanks!

done. thanks!

Do you know how this affects JS with typed arrays?

(In reply to Jan de Mooij [:jandem] from comment #36)

Do you know how this affects JS with typed arrays?

Should not affect JS at all. Float32Array and Float64Array are always aligned, and for DataView I'm pretty sure we're always careful to use memcpy for access. This is basically a consequence of wasm allowing unaligned accesses even when the instruction says the access is aligned, combined with the desire always to emit the best possible code for accesses that say they are aligned. (See commit msg for more, or lmk if you need more information.)

(In reply to Lars T Hansen [:lth] from comment #37)

Float32Array and Float64Array are always aligned, and for DataView I'm pretty sure we're always careful to use memcpy for access.

Ah right. I thought TypedArray views could be unaligned but you're right. Great!

https://hg.mozilla.org/mozilla-central/rev/1230184adda1