Bug 1639486 - Fix shuffling of stack results in br_if / br_table r=lth
authorAndy Wingo <wingo@igalia.com>
Thu, 21 May 2020 11:36:32 +0000
changeset 531438 a1e751f0ab229fcf45353cc0cdeee8c8714544bf
parent 531437 f8994fa4909eba9fd2034b17a332ef35387c6fae
child 531439 ea57e047f524113afc011567a5b83e9f2bad0a0d
push id116655
push userwingo@igalia.com
push dateThu, 21 May 2020 12:09:04 +0000
treeherderautoland@a1e751f0ab22 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslth
bugs1639486
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 1639486 - Fix shuffling of stack results in br_if / br_table r=lth Rather confusingly, sometimes stack height refers to a height with stack results, and sometimes a height without results. We were confusing the two: the raw shuffleStackResultsTowardFP function that took uint32_t heights assumed the former, but the version that took StackHeight assumed the latter. The result was a mis-shuffle that could be catastrophic. Differential Revision: https://phabricator.services.mozilla.com/D76136
js/src/jit-test/tests/wasm/multi-value/regress-1621645-2.js
js/src/wasm/WasmBaselineCompile.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/wasm/multi-value/regress-1621645-2.js
@@ -0,0 +1,19 @@
+wasmFullPass(`
+  ;; Iterative factorial without locals.
+  (func $pick0 (param i64) (result i64 i64)
+    (local.get 0) (local.get 0)
+  )
+  (func $pick1 (param i64 i64) (result i64 i64 i64)
+    (local.get 0) (local.get 1) (local.get 0)
+  )
+  (func (export "run") (param i64) (result i64)
+    (i64.const 1) (local.get 0)
+    (loop $l (param i64 i64) (result i64)
+      (call $pick1) (call $pick1) (i64.mul)
+      (call $pick1) (i64.const 1) (i64.sub)
+      (call $pick0) (i64.const 0) (i64.gt_u)
+      (br_if $l)
+      (drop) (return)
+    )
+  )`,
+             7034535277573963776n, {}, 25n);
--- a/js/src/wasm/WasmBaselineCompile.cpp
+++ b/js/src/wasm/WasmBaselineCompile.cpp
@@ -2083,16 +2083,17 @@ class BaseStackFrame final : public Base
   }
 
   void finishStackResultArea(StackHeight stackBase, uint32_t stackResultBytes) {
     uint32_t end = computeHeightWithStackResults(stackBase, stackResultBytes);
     MOZ_ASSERT(currentStackHeight() >= end);
     popBytes(currentStackHeight() - end);
   }
 
+  // |srcHeight| and |destHeight| are stack heights *including* |bytes|.
   void shuffleStackResultsTowardFP(uint32_t srcHeight, uint32_t destHeight,
                                    uint32_t bytes, Register temp) {
     MOZ_ASSERT(destHeight < srcHeight);
     MOZ_ASSERT(bytes % sizeof(uint32_t) == 0);
     uint32_t destOffset = stackOffset(destHeight) + bytes;
     uint32_t srcOffset = stackOffset(srcHeight) + bytes;
     while (bytes >= sizeof(intptr_t)) {
       destOffset -= sizeof(intptr_t);
@@ -2105,28 +2106,31 @@ class BaseStackFrame final : public Base
       MOZ_ASSERT(bytes == sizeof(uint32_t));
       destOffset -= sizeof(uint32_t);
       srcOffset -= sizeof(uint32_t);
       masm.load32(Address(sp_, srcOffset), temp);
       masm.store32(temp, Address(sp_, destOffset));
     }
   }
 
+  // Unlike the overload that operates on raw heights, |srcHeight| and
+  // |destHeight| are stack heights *not including* |bytes|.
   void shuffleStackResultsTowardFP(StackHeight srcHeight,
                                    StackHeight destHeight, uint32_t bytes,
                                    Register temp) {
     MOZ_ASSERT(srcHeight.isValid());
     MOZ_ASSERT(destHeight.isValid());
     uint32_t src = computeHeightWithStackResults(srcHeight, bytes);
     uint32_t dest = computeHeightWithStackResults(destHeight, bytes);
     MOZ_ASSERT(src <= currentStackHeight());
     MOZ_ASSERT(dest <= currentStackHeight());
-    shuffleStackResultsTowardFP(src - bytes, dest - bytes, bytes, temp);
-  }
-
+    shuffleStackResultsTowardFP(src, dest, bytes, temp);
+  }
+
+  // |srcHeight| and |destHeight| are stack heights *including* |bytes|.
   void shuffleStackResultsTowardSP(uint32_t srcHeight, uint32_t destHeight,
                                    uint32_t bytes, Register temp) {
     MOZ_ASSERT(destHeight > srcHeight);
     MOZ_ASSERT(bytes % sizeof(uint32_t) == 0);
     uint32_t destOffset = stackOffset(destHeight);
     uint32_t srcOffset = stackOffset(srcHeight);
     while (bytes >= sizeof(intptr_t)) {
       masm.loadPtr(Address(sp_, srcOffset), temp);