Bug 1637328 - Make stack copies in stubs handle v128 data. r=rhunt
authorLars T Hansen <lhansen@mozilla.com>
Mon, 18 May 2020 08:41:42 +0000
changeset 530547 9d54a4e4532c719106671c2ee082aa2f01a63788
parent 530546 45263d780e6e47261412ffaac0a327f870c47fd9
child 530548 7c4143522947a372cbf768602b21cf3b761e039d
push id37428
push usernbeleuzu@mozilla.com
push dateMon, 18 May 2020 21:48:24 +0000
treeherdermozilla-central@a3941f42e662 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrhunt
bugs1637328
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1637328 - Make stack copies in stubs handle v128 data. r=rhunt We can't MOZ_CRASH for the v128 cases of the stack copies in SetupABIArguments and StackCopy. In the former case we can emit a breakpoint instruction because the code should never be executed. In the latter case we just copy the data. Adds test cases that triggered the MOZ_CRASHes, and other test cases that are related but left out of the test cases when SIMD for baseline first landed. Differential Revision: https://phabricator.services.mozilla.com/D74934
js/src/jit-test/tests/wasm/simd/ad-hack.js
js/src/wasm/WasmStubs.cpp
--- a/js/src/jit-test/tests/wasm/simd/ad-hack.js
+++ b/js/src/jit-test/tests/wasm/simd/ad-hack.js
@@ -1501,16 +1501,85 @@ var insrun = wasmEvalText(`
                           {"":insworker.exports});
 
 var mem = new Uint8Array(insrun.exports.mem.buffer);
 var xs = iota(16).map((x) => x+5);
 set(mem, 0, xs);
 insrun.exports.run(0, 16);
 assertSame(get(mem, 16, 16), xs.map((x,i) => x+i))
 
+// Make sure JS<->wasm call guards are sensible.
+
+// Calling from JS to export that accepts v128.
+assertErrorMessage(() => insworker.exports.worker(),
+                   TypeError,
+                   /cannot pass.*v128.*to or from JS/);
+
+// Calling from wasm with v128 to import that comes from JS.  The instantiation
+// will succeed even if the param type of the import is v128 (see "create a host
+// function" in the Wasm JSAPI spec), it is the act of invoking it that checks
+// that verboten types are not used (see "run a host function", ibid.).
+var badImporter = wasmEvalText(`
+  (module
+    (import "" "worker" (func $worker (param v128) (result v128)))
+    (func (export "run")
+      (drop (call $worker (v128.const i32x4 0 1 2 3)))))`,
+             {"":{worker: function(a) { return a; }}});
+
+assertErrorMessage(() => badImporter.exports.run(),
+                   TypeError,
+                   /cannot pass.*v128.*to or from JS/);
+
+// Imports and exports that pass and return v128 as stack (not register) args.
+
+var exportWithStackArgs = wasmEvalText(`
+  (module
+    (func (export "worker") (param v128) (param v128) (param v128) (param v128)
+                            (param v128) (param v128) (param v128) (param v128)
+                            (param v128) (param v128) (param v128) (param v128)
+                            (param v128) (param v128)
+           (result v128 v128)
+      (i8x16.add (local.get 3) (local.get 12))
+      (local.get 7)))`);
+
+var importWithStackArgs = wasmEvalText(`
+  (module
+    (type $t1 (func (param v128) (param v128) (param v128) (param v128)
+                    (param v128) (param v128) (param v128) (param v128)
+                    (param v128) (param v128) (param v128) (param v128)
+                    (param v128) (param v128)
+                    (result v128 v128)))
+    (import "" "worker" (func $worker (type $t1)))
+    (memory (export "mem") 1 1)
+    (table funcref (elem $worker))
+    (func (export "run")
+      (i32.const 16)
+      (call_indirect (type $t1) (v128.const i32x4 1 1 1 1) (v128.const i32x4 2 2 2 2) (v128.const i32x4 3 3 3 3)
+                    (v128.const i32x4 4 4 4 4) (v128.const i32x4 5 5 5 5) (v128.const i32x4 6 6 6 6)
+                    (v128.const i32x4 7 7 7 7) (v128.const i32x4 8 8 8 8) (v128.const i32x4 9 9 9 9)
+                    (v128.const i32x4 10 10 10 10) (v128.const i32x4 11 11 11 11) (v128.const i32x4 12 12 12 12)
+                    (v128.const i32x4 13 13 13 13) (v128.const i32x4 14 14 14 14)
+           (i32.const 0))
+      drop
+      v128.store
+      (i32.const 0)
+      (call $worker (v128.const i32x4 1 1 1 1) (v128.const i32x4 2 2 2 2) (v128.const i32x4 3 3 3 3)
+                    (v128.const i32x4 4 4 4 4) (v128.const i32x4 5 5 5 5) (v128.const i32x4 6 6 6 6)
+                    (v128.const i32x4 7 7 7 7) (v128.const i32x4 8 8 8 8) (v128.const i32x4 9 9 9 9)
+                    (v128.const i32x4 10 10 10 10) (v128.const i32x4 11 11 11 11) (v128.const i32x4 12 12 12 12)
+                    (v128.const i32x4 13 13 13 13) (v128.const i32x4 14 14 14 14))
+      drop
+      v128.store))`,
+                                       {"": exportWithStackArgs.exports});
+
+var mem = new Int32Array(importWithStackArgs.exports.mem.buffer);
+importWithStackArgs.exports.run();
+assertSame(get(mem, 0, 4), [17, 17, 17, 17]);
+assertSame(get(mem, 4, 4), [17, 17, 17, 17]);
+
 // Imports and exports of v128 globals
 
 var insexporter = wasmEvalText(`
   (module
     (global (export "myglobal") (mut v128) (v128.const i8x16 ${iota(16).join(' ')})))`);
 
 var insimporter = wasmEvalText(`
   (module
@@ -1519,16 +1588,26 @@ var insimporter = wasmEvalText(`
     (func (export "run") (param $dest i32)
       (v128.store (local.get $dest) (global.get $g))))`,
                                {m:insexporter.exports});
 
 var mem = new Uint8Array(insimporter.exports.mem.buffer);
 insimporter.exports.run(16);
 assertSame(get(mem, 16, 16), iota(16));
 
+// Guards on accessing v128 globals from JS
+
+assertErrorMessage(() => insexporter.exports.myglobal.value = 0,
+                   TypeError,
+                   /cannot pass.*v128.*to or from JS/);
+
+assertErrorMessage(function () { let v = insexporter.exports.myglobal.value },
+                   TypeError,
+                   /cannot pass.*v128.*to or from JS/);
+
 // Multi-value cases
 
 var ins = wasmEvalText(`
   (module
     (memory (export "mem") 1 1)
     (func $mvreturn (result v128 v128 v128)
       (v128.load (i32.const 16))
       (v128.load (i32.const 0))
--- a/js/src/wasm/WasmStubs.cpp
+++ b/js/src/wasm/WasmStubs.cpp
@@ -244,29 +244,42 @@ static void GenPrintF64(DebugChannel cha
     if (inWasm) {
       masm.callDebugWithABI(SymbolicAddress::PrintF64);
     } else {
       masm.callWithABI((void*)PrintF64, MoveOp::GENERAL,
                        CheckUnsafeCallWithABI::DontCheckOther);
     }
   });
 }
+
+#  ifdef ENABLE_WASM_SIMD
+static void GenPrintV128(DebugChannel channel, MacroAssembler& masm,
+                         const FloatRegister& src) {
+  // TODO: We might try to do something meaningful here once SIMD data are
+  // aligned and hence C++-ABI compliant.  For now, just make ourselves visible.
+  GenPrintf(channel, masm, "v128");
+}
+#  endif
 #else
 static void GenPrintf(DebugChannel channel, MacroAssembler& masm,
                       const char* fmt, ...) {}
 static void GenPrintIsize(DebugChannel channel, MacroAssembler& masm,
                           const Register& src) {}
 static void GenPrintPtr(DebugChannel channel, MacroAssembler& masm,
                         const Register& src) {}
 static void GenPrintI64(DebugChannel channel, MacroAssembler& masm,
                         const Register64& src) {}
 static void GenPrintF32(DebugChannel channel, MacroAssembler& masm,
                         const FloatRegister& src) {}
 static void GenPrintF64(DebugChannel channel, MacroAssembler& masm,
                         const FloatRegister& src) {}
+#  ifdef ENABLE_WASM_SIMD
+static void GenPrintV128(DebugChannel channel, MacroAssembler& masm,
+                         const FloatRegister& src) {}
+#  endif
 #endif
 
 static bool FinishOffsets(MacroAssembler& masm, Offsets* offsets) {
   // On old ARM hardware, constant pools could be inserted and they need to
   // be flushed before considering the size of the masm.
   masm.flushBuffer();
   offsets->end = masm.size();
   return !masm.oom();
@@ -350,17 +363,20 @@ static void SetupABIArguments(MacroAssem
           case MIRType::Double:
             masm.loadDouble(src, iter->fpu());
             break;
           case MIRType::Float32:
             masm.loadFloat32(src, iter->fpu());
             break;
           case MIRType::Int8x16:
 #ifdef ENABLE_WASM_SIMD
-            masm.loadUnalignedSimd128(src, iter->fpu());
+            // We will reach this point when we generate interpreter entry stubs
+            // for exports that receive v128 values, but the code will never be
+            // executed because such exports cannot be called from JS.
+            masm.breakpoint();
             break;
 #else
             MOZ_CRASH("V128 not supported in SetupABIArguments");
 #endif
           default:
             MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("unexpected FPU type");
             break;
         }
@@ -393,17 +409,25 @@ static void SetupABIArguments(MacroAssem
           case MIRType::Float32: {
             ScratchFloat32Scope fpscratch(masm);
             masm.loadFloat32(src, fpscratch);
             masm.storeFloat32(fpscratch, Address(masm.getStackPointer(),
                                                  iter->offsetFromArgBase()));
             break;
           }
           case MIRType::Int8x16: {
-            MOZ_CRASH("SIMD stack argument");
+#ifdef ENABLE_WASM_SIMD
+            // We will reach this point when we generate interpreter entry stubs
+            // for exports that receive v128 values, but the code will never be
+            // executed because such exports cannot be called from JS.
+            masm.breakpoint();
+            break;
+#else
+            MOZ_CRASH("V128 not supported in SetupABIArguments");
+#endif
           }
           case MIRType::StackResults: {
             MOZ_ASSERT(args.isSyntheticStackResultPointerArg(iter.index()));
             masm.loadPtr(src, scratch);
             masm.storePtr(scratch, Address(masm.getStackPointer(),
                                            iter->offsetFromArgBase()));
             break;
           }
@@ -1632,16 +1656,23 @@ static void StackCopy(MacroAssembler& ma
     masm.loadFloat32(src, fpscratch);
     GenPrintF32(DebugChannel::Import, masm, fpscratch);
     masm.storeFloat32(fpscratch, dst);
   } else if (type == MIRType::Double) {
     ScratchDoubleScope fpscratch(masm);
     masm.loadDouble(src, fpscratch);
     GenPrintF64(DebugChannel::Import, masm, fpscratch);
     masm.storeDouble(fpscratch, dst);
+#ifdef ENABLE_WASM_SIMD
+  } else if (type == MIRType::Int8x16) {
+    ScratchSimd128Scope fpscratch(masm);
+    masm.loadUnalignedSimd128(src, fpscratch);
+    GenPrintV128(DebugChannel::Import, masm, fpscratch);
+    masm.storeUnalignedSimd128(fpscratch, dst);
+#endif
   } else {
     MOZ_CRASH("StackCopy: unexpected type");
   }
 }
 
 using ToValue = bool;
 
 // Note, when toValue is true then this may destroy the values in incoming