From 8094f338af48e5ff56133f3f8fa13d8d3e7e996e Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Mon, 9 Nov 2020 14:29:53 -0800 Subject: [PATCH 1/2] chore: cherry-pick 8c725f7b5bbf from v8 --- patches/v8/.patches | 1 + patches/v8/cherry-pick-8c725f7b5bbf.patch | 125 ++++++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 patches/v8/cherry-pick-8c725f7b5bbf.patch diff --git a/patches/v8/.patches b/patches/v8/.patches index 67866ccad6ddc..2f94743c247ce 100644 --- a/patches/v8/.patches +++ b/patches/v8/.patches @@ -15,3 +15,4 @@ cherry-pick-7e5c7b5964.patch cherry-pick-6a4cd97d6691.patch cherry-pick-6aa1e71fbd09.patch perf_make_getpositioninfoslow_faster.patch +cherry-pick-8c725f7b5bbf.patch diff --git a/patches/v8/cherry-pick-8c725f7b5bbf.patch b/patches/v8/cherry-pick-8c725f7b5bbf.patch new file mode 100644 index 0000000000000..ddec908a931bc --- /dev/null +++ b/patches/v8/cherry-pick-8c725f7b5bbf.patch @@ -0,0 +1,125 @@ +From 8c725f7b5bbfcfd5741c2bc722827487831311ec Mon Sep 17 00:00:00 2001 +From: Thibaud Michaud +Date: Thu, 15 Oct 2020 12:45:34 +0200 +Subject: [PATCH] Merged: [codegen] Skip invalid optimization in tail calls + +Preparing for tail call is usually done by emitting the gap moves and +then moving the stack pointer to its new position. An optimization +consists in moving the stack pointer first and transforming some of the +moves into pushes. In the attached case it looks like this (arm): + +138 add sp, sp, #40 +13c str r6, [sp, #-4]! +140 str r6, [sp, #-4]! +144 str r6, [sp, #-4]! +148 str r6, [sp, #-4]! +14c str r6, [sp, #-4]! +... +160 vldr d1, [sp - 4*3] + +The last line is a gap reload, but because the stack pointer was already +moved, the slot is now below the stack pointer. This is invalid and +triggers this DCHECK: + +Fatal error in ../../v8/src/codegen/arm/assembler-arm.cc, line 402 +Debug check failed: 0 <= offset (0 vs. -12). + +A comment already explains that we skip the optimization if the gap +contains stack moves to prevent this, but the code only checks for +non-FP slots. This is fixed by replacing "source.IsStackSlot()" with +"source.IsAnyStackSlot()": + +108 vldr d1, [sp + 4*2] +... +118 str r0, [sp, #+36] +11c str r0, [sp, #+32] +120 str r0, [sp, #+28] +124 str r0, [sp, #+24] +128 str r0, [sp, #+20] +... +134 add sp, sp, #20 + +TBR=​jgruber@chromium.org + +(cherry picked from commit 7506e063d0d7fb00e4b9c06735c91e1953296867) + +Change-Id: I66ed6187755af956e245207e940c83ea0697a5e6 +Bug: chromium:1137608 +No-Try: true +No-Presubmit: true +No-Tree-Checks: true +Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2505976 +Reviewed-by: Thibaud Michaud +Commit-Queue: Thibaud Michaud +Cr-Commit-Position: refs/branch-heads/8.6@{#42} +Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} +Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} +--- + +diff --git a/src/compiler/backend/code-generator.cc b/src/compiler/backend/code-generator.cc +index 6e740b1..a3b2f69 100644 +--- a/src/compiler/backend/code-generator.cc ++++ b/src/compiler/backend/code-generator.cc +@@ -613,8 +613,8 @@ + // then the full gap resolver must be used since optimization with + // pushes don't participate in the parallel move and might clobber + // values needed for the gap resolve. +- if (source.IsStackSlot() && LocationOperand::cast(source).index() >= +- first_push_compatible_index) { ++ if (source.IsAnyStackSlot() && LocationOperand::cast(source).index() >= ++ first_push_compatible_index) { + pushes->clear(); + return; + } +diff --git a/test/mjsunit/regress/wasm/regress-1137608.js b/test/mjsunit/regress/wasm/regress-1137608.js +new file mode 100644 +index 0000000..5011dce +--- /dev/null ++++ b/test/mjsunit/regress/wasm/regress-1137608.js +@@ -0,0 +1,46 @@ ++// Copyright 2020 the V8 project authors. All rights reserved. ++// Use of this source code is governed by a BSD-style license that can be ++// found in the LICENSE file. ++// ++// Flags: --no-liftoff --experimental-wasm-return-call --experimental-wasm-threads ++ ++load("test/mjsunit/wasm/wasm-module-builder.js"); ++ ++(function Regress1137608() { ++ print(arguments.callee.name); ++ let builder = new WasmModuleBuilder(); ++ let sig0 = builder.addType(kSig_i_iii); ++ let sig1 = builder.addType(makeSig([kWasmF64, kWasmF64, kWasmI32, ++ kWasmI32, kWasmI32, kWasmF32, kWasmI32, kWasmF64, kWasmI32, kWasmF32, ++ kWasmI32, kWasmF32, kWasmI32, kWasmF64, kWasmI32], [kWasmI32])); ++ let main = builder.addFunction("main", sig0) ++ .addBody([ ++ kExprI64Const, 0, ++ kExprF64UConvertI64, ++ kExprF64Const, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x00, 0x00, ++ kExprF64Const, 0x30, 0x30, 0x30, 0x00, 0x00, 0x00, 0x00, 0x00, ++ kExprF64Mul, ++ kExprI32Const, 0, ++ kExprF64Const, 0x30, 0x30, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, ++ kExprF64StoreMem, 0x00, 0xb0, 0xe0, 0xc0, 0x81, 0x03, ++ kExprI32Const, 0, ++ kExprI32Const, 0, ++ kExprI32Const, 0, ++ kExprF32Const, 0x00, 0x00, 0x00, 0x00, ++ kExprI32Const, 0, ++ kExprF64Const, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, ++ kExprI32Const, 0, ++ kExprF32Const, 0x00, 0x00, 0x00, 0x00, ++ kExprI32Const, 0, ++ kExprF32Const, 0x00, 0x00, 0x00, 0x00, ++ kExprI32Const, 0, ++ kExprF64Const, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, ++ kExprI32Const, 0, ++ kExprI32Const, 2, ++ kExprReturnCallIndirect, sig1, kTableZero]).exportFunc(); ++ builder.addFunction("f", sig1).addBody([kExprI32Const, 0]); ++ builder.addTable(kWasmAnyFunc, 4, 4); ++ builder.addMemory(16, 32, false, true); ++ let module = new WebAssembly.Module(builder.toBuffer()); ++ let instance = new WebAssembly.Instance(module); ++})(); From da1d8aa18cb71db084d42bc86d475ed04684de28 Mon Sep 17 00:00:00 2001 From: Electron Bot Date: Mon, 9 Nov 2020 22:43:36 +0000 Subject: [PATCH 2/2] update patches --- patches/v8/cherry-pick-8c725f7b5bbf.patch | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/patches/v8/cherry-pick-8c725f7b5bbf.patch b/patches/v8/cherry-pick-8c725f7b5bbf.patch index ddec908a931bc..46dfdcc3961e5 100644 --- a/patches/v8/cherry-pick-8c725f7b5bbf.patch +++ b/patches/v8/cherry-pick-8c725f7b5bbf.patch @@ -1,7 +1,10 @@ -From 8c725f7b5bbfcfd5741c2bc722827487831311ec Mon Sep 17 00:00:00 2001 +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Thibaud Michaud Date: Thu, 15 Oct 2020 12:45:34 +0200 -Subject: [PATCH] Merged: [codegen] Skip invalid optimization in tail calls +Subject: Merged: [codegen] Skip invalid optimization in tail calls +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit Preparing for tail call is usually done by emitting the gap moves and then moving the stack pointer to its new position. An optimization @@ -54,13 +57,12 @@ Commit-Queue: Thibaud Michaud Cr-Commit-Position: refs/branch-heads/8.6@{#42} Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1} Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472} ---- diff --git a/src/compiler/backend/code-generator.cc b/src/compiler/backend/code-generator.cc -index 6e740b1..a3b2f69 100644 +index 9dbd5fac333c557f8e7de9b63d18b68f94b817e7..0a2e37c5e67d6d2d512bb970ec38d9a25c2560c9 100644 --- a/src/compiler/backend/code-generator.cc +++ b/src/compiler/backend/code-generator.cc -@@ -613,8 +613,8 @@ +@@ -607,8 +607,8 @@ void CodeGenerator::GetPushCompatibleMoves(Instruction* instr, // then the full gap resolver must be used since optimization with // pushes don't participate in the parallel move and might clobber // values needed for the gap resolve. @@ -73,7 +75,7 @@ index 6e740b1..a3b2f69 100644 } diff --git a/test/mjsunit/regress/wasm/regress-1137608.js b/test/mjsunit/regress/wasm/regress-1137608.js new file mode 100644 -index 0000000..5011dce +index 0000000000000000000000000000000000000000..5011dced2f70ffbe2b6096a4e1863f434c8898a8 --- /dev/null +++ b/test/mjsunit/regress/wasm/regress-1137608.js @@ -0,0 +1,46 @@