Bug 1546157 - Wasm/Baseline: ensure stackmaps take into account reftyped locals. r=lhansen.
authorJulian Seward <jseward@acm.org>
Wed, 24 Apr 2019 13:43:43 +0200
changeset 530098 93e4873ffd1dd508057bd1049750fcf08ad2b1be
parent 530097 ec15db17478768038e372a69583e9a76949896f6
child 530099 07efc6e32c8729fc9ef50e4e955beca58ff91d48
child 530241 6166f57a8f8359583cfa21b2db1857d27f63b8f1
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslhansen
bugs1546157
milestone68.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 1546157 - Wasm/Baseline: ensure stackmaps take into account reftyped locals. r=lhansen. Prior to this patch, Wasm/Baseline's stackmap creation logic failed to take into account reftyped locals which are not also parameters. This patch fixes that. It also adds a new test that reliably exposes the failure on all 4 primary targets. The test case also runs on Ion, and it appears that Wasm/Ion does not suffer from an analogous problem. Differential Revision: https://phabricator.services.mozilla.com/D28654
js/src/jit-test/tests/wasm/gc/stackmaps4-params-n-locals.js
js/src/wasm/WasmBaselineCompile.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/wasm/gc/stackmaps4-params-n-locals.js
@@ -0,0 +1,145 @@
+// |jit-test| skip-if: !wasmReftypesEnabled()
+
+// A stress test for stackmap creation as it relates to params and locals.
+
+function Stuff(n) {
+    this.n = n;
+}
+function allocate(n) {
+    const res = new Stuff(n);
+    // The webassembly loop below will provide us with 254 as an arg after not
+    // very long.
+    if (n == 254) {
+        gc();
+    }
+    return res;
+}
+
+function visit(aStuff) {
+    return aStuff.n;
+}
+
+let bin = wasmTextToBinary(
+`
+(module
+  (import $allocate "" "allocate" (func (result anyref) (param i32)))
+  (import $visit "" "visit" (func (result i32) (param anyref)))
+
+  ;; A function with many params and locals, most of which are ref-typed.
+  ;; The purpose of having so many is to defeat any reasonable attempt at
+  ;; allocating them all in registers.  The asymmetrically-placed i32s are
+  ;; an attempt to expose any misalignment or inversion of the stack layout
+  ;; vs what the stackmap claims the layout to be.
+
+  (func $manyParamsAndLocals (export "manyParamsAndLocals")
+    (result i32)
+    (param $p1 anyref) (param $p2 i32)    (param $p3 anyref)
+    (param $p4 anyref) (param $p5 anyref) (param $p6 anyref)
+    (param $p7 anyref) (param $p8 anyref) (param $p9 i32)
+    (local $l1 anyref) (local $l2 anyref) (local $l3 anyref)
+    (local $l4 i32)    (local $l5 anyref) (local $l6 i32)
+    (local $l7 anyref) (local $l8 anyref) (local $l9 anyref)
+
+    (local $i i32)
+    (local $runningTotal i32)
+
+    ;; Bind some objects to l1 .. l9.  The JS harness will already
+    ;; have done the same for p1 .. p9.
+    (local.set $l1 (call $allocate (i32.const 1)))
+    (local.set $l2 (call $allocate (i32.const 3)))
+    (local.set $l3 (call $allocate (i32.const 5)))
+    (local.set $l4 (i32.const 7))
+    (local.set $l5 (call $allocate (i32.const 9)))
+    (local.set $l6 (i32.const 11))
+    (local.set $l7 (call $allocate (i32.const 13)))
+    (local.set $l8 (call $allocate (i32.const 15)))
+    (local.set $l9 (call $allocate (i32.const 17)))
+
+    ;; Now loop, allocating as we go, and forcing GC every 256 iterations.
+    ;; Also in each iteration, visit all the locals and params, in the hope
+    ;; of exposing any cases where they are not held live across GC.
+    (loop $CONT
+      ;; Allocate, and hold on to the resulting value, so that Ion can't
+      ;; delete the allocation.
+      (local.set $l9 (call $allocate (i32.and (local.get $i) (i32.const 255))))
+
+      ;; Visit all params and locals
+
+      local.get $runningTotal
+
+      (call $visit (local.get $p1))
+      i32.add
+      local.get $p2
+      i32.add
+      (call $visit (local.get $p3))
+      i32.add
+      (call $visit (local.get $p4))
+      i32.add
+      (call $visit (local.get $p5))
+      i32.add
+      (call $visit (local.get $p6))
+      i32.add
+      (call $visit (local.get $p7))
+      i32.add
+      (call $visit (local.get $p8))
+      i32.add
+      local.get $p9
+      i32.add
+
+      (call $visit (local.get $l1))
+      i32.add
+      (call $visit (local.get $l2))
+      i32.add
+      (call $visit (local.get $l3))
+      i32.add
+      local.get $l4
+      i32.add
+      (call $visit (local.get $l5))
+      i32.add
+      local.get $l6
+      i32.add
+      (call $visit (local.get $l7))
+      i32.add
+      (call $visit (local.get $l8))
+      i32.add
+      (call $visit (local.get $l9))
+      i32.add
+
+      local.set $runningTotal
+
+      (local.set $i (i32.add (local.get $i) (i32.const 1)))
+      (br_if $CONT (i32.lt_s (local.get $i) (i32.const 10000)))
+    ) ;; loop
+
+    local.get $runningTotal
+  ) ;; func
+)
+`);
+
+let mod = new WebAssembly.Module(bin);
+let ins = new WebAssembly.Instance(mod, {"":{allocate, visit}});
+
+let p1 = allocate(97);
+let p2 = 93;
+let p3 = allocate(91);
+let p4 = allocate(83);
+let p5 = allocate(79);
+let p6 = allocate(73);
+let p7 = allocate(71);
+let p8 = allocate(67);
+let p9 = 61;
+
+let res = ins.exports.manyParamsAndLocals(p1, p2, p3, p4, p5, p6, p7, p8, p9);
+
+// Check that GC didn't mess up p1 .. p9
+res += visit(p1);
+res += p2;
+res += visit(p3);
+res += visit(p4);
+res += visit(p5);
+res += visit(p6);
+res += visit(p7);
+res += visit(p8);
+res += p9;
+
+assertEq(res, 9063795);
--- a/js/src/wasm/WasmBaselineCompile.cpp
+++ b/js/src/wasm/WasmBaselineCompile.cpp
@@ -4151,35 +4151,47 @@ class BaseCompiler final : public BaseCo
 
     masm.reserveStack(reservedBytes);
     fr.onFixedStackAllocated();
     if (!stackMapGenerator_.machineStackTracker.pushNonGCPointers(
             reservedBytes / sizeof(void*))) {
       return false;
     }
 
+    // Locals are stack allocated.  Mark ref-typed ones in the stackmap
+    // accordingly.
+    for (const Local& l : localInfo_) {
+      if (l.type == MIRType::RefOrNull) {
+        uint32_t offs = fr.localOffset(l);
+        MOZ_ASSERT(0 == (offs % sizeof(void*)));
+        stackMapGenerator_.machineStackTracker
+                          .setGCPointer(offs / sizeof(void*));
+      }
+    }
+
     // Copy arguments from registers to stack.
     for (ABIArgIter<const ValTypeVector> i(args); !i.done(); i++) {
       if (!i->argInRegister()) {
         continue;
       }
       Local& l = localInfo_[i.index()];
       switch (i.mirType()) {
         case MIRType::Int32:
           fr.storeLocalI32(RegI32(i->gpr()), l);
           break;
         case MIRType::Int64:
           fr.storeLocalI64(RegI64(i->gpr64()), l);
           break;
         case MIRType::RefOrNull: {
-          uint32_t offs = fr.localOffset(l);
+          DebugOnly<uint32_t> offs = fr.localOffset(l);
           MOZ_ASSERT(0 == (offs % sizeof(void*)));
           fr.storeLocalPtr(RegPtr(i->gpr()), l);
-          stackMapGenerator_.machineStackTracker.setGCPointer(offs /
-                                                              sizeof(void*));
+          // We should have just visited this local in the preceding loop.
+          MOZ_ASSERT(stackMapGenerator_.machineStackTracker
+                                       .isGCPointer(offs / sizeof(void*)));
           break;
         }
         case MIRType::Double:
           fr.storeLocalF64(RegF64(i->fpu()), l);
           break;
         case MIRType::Float32:
           fr.storeLocalF32(RegF32(i->fpu()), l);
           break;