Reland "[compiler] Emit a function-entry stack check on OSR-entry"

This is a reland of 8703c38d

The reland marks the new test as slow, skips all variants, and
skips all non-release modes.

Original change's description:
> [compiler] Emit a function-entry stack check on OSR-entry
>
> This CL extends the smarter function-entry stack check logic (see
> v8:9534) to OSR'd code. These smarter stack checks prevent
> overflowing the stack during deoptimization.
>
> The challenge for both function-entry (FE) and OSR-entry (OE) stack
> checks is that there is no dedicated physical StackCheck to
> deoptimize into. For more context: the physical StackCheck bytecode
> was removed in crrev.com/c/1914218.
>
> FE stack checks solve this by using a marker bailout id to signify
> a deopt bytecode offset before the first bytecode.
>
> In this CL, OE stack checks take a similar approach by using the
> OSR'd loop's JumpLoop bytecode, which is conceptually immediately
> before the OSR'd loop header.
>
> When a stack overflow at an OE stack check occurs: %StackGuard
> may cause a lazy deopt on return to the optimized OSR code,
> causing re-execution of the JumpLoop handler in the
> InterpreterEnterBytecodeAdvance builtin, ultimately continuing
> execution the interpreter at the first bytecode of the OSR'd loop
> header.
>
> Bug: chromium:1034322, v8:9534
> Change-Id: I1ae88a08702cde9a5eb84a451a9f1acc41204d5c
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2625872
> Auto-Submit: Jakob Gruber <jgruber@chromium.org>
> Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
> Reviewed-by: Georg Neis <neis@chromium.org>
> Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72153}

Tbr: neis@chromium.org, solanes@chromium.org
Bug: chromium:1034322
Bug: v8:9534
Change-Id: I28a23d0cc4b14d59c3d4a5dbadd5dab3ac31d442
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2639753Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72183}

src/compiler/bytecode-graph-builder.cc

@@ -207,10 +207,54 @@ class BytecodeGraphBuilder {
 
   // Prepare information for lazy deoptimization. This information is attached
   // to the given node and the output value produced by the node is combined.
-  // Conceptually this frame state is "after" a given operation.
-  void PrepareFrameState(Node* node, OutputFrameStateCombine combine);
+  //
+  // The low-level chokepoint - use the variants below instead.
   void PrepareFrameState(Node* node, OutputFrameStateCombine combine,
-                         BailoutId bailout_id);
+                         BailoutId bailout_id,
+                         const BytecodeLivenessState* liveness);
+
+  // In the common case, frame states are conceptually "after" a given
+  // operation and at the current bytecode offset.
+  void PrepareFrameState(Node* node, OutputFrameStateCombine combine) {
+    if (!OperatorProperties::HasFrameStateInput(node->op())) return;
+    const int offset = bytecode_iterator().current_offset();
+    return PrepareFrameState(node, combine, BailoutId(offset),
+                             bytecode_analysis().GetOutLivenessFor(offset));
+  }
+
+  // For function-entry stack checks, they're conceptually "before" the first
+  // bytecode and at a special marker bytecode offset.
+  // In the case of FE stack checks, the current bytecode is also the first
+  // bytecode, so we use a special marker bytecode offset to signify a virtual
+  // bytecode before the first physical bytecode.
+  void PrepareFrameStateForFunctionEntryStackCheck(Node* node) {
+    DCHECK_EQ(bytecode_iterator().current_offset(), 0);
+    DCHECK(OperatorProperties::HasFrameStateInput(node->op()));
+    DCHECK(node->opcode() == IrOpcode::kJSStackCheck);
+    return PrepareFrameState(node, OutputFrameStateCombine::Ignore(),
+                             BailoutId(kFunctionEntryBytecodeOffset),
+                             bytecode_analysis().GetInLivenessFor(0));
+  }
+
+  // For OSR-entry stack checks, they're conceptually "before" the first
+  // bytecode of the current loop. We implement this in a similar manner to
+  // function-entry (FE) stack checks above, i.e. we deopt at the predecessor
+  // of the current bytecode.
+  // In the case of OSR-entry stack checks, a physical predecessor bytecode
+  // exists: the JumpLoop bytecode. We attach to JumpLoop by using
+  // `bytecode_analysis().osr_bailout_id()` instead of current_offset (the
+  // former points at JumpLoop, the latter at the loop header, i.e. the target
+  // of JumpLoop).
+  void PrepareFrameStateForOSREntryStackCheck(Node* node) {
+    DCHECK_EQ(bytecode_iterator().current_offset(),
+              bytecode_analysis().osr_entry_point());
+    DCHECK(OperatorProperties::HasFrameStateInput(node->op()));
+    DCHECK(node->opcode() == IrOpcode::kJSStackCheck);
+    const int offset = bytecode_analysis().osr_bailout_id().ToInt();
+    return PrepareFrameState(node, OutputFrameStateCombine::Ignore(),
+                             BailoutId(offset),
+                             bytecode_analysis().GetOutLivenessFor(offset));
+  }
 
   void BuildCreateArguments(CreateArgumentsType type);
   Node* BuildLoadGlobal(NameRef name, uint32_t feedback_slot_index,
@@ -304,6 +348,7 @@ class BytecodeGraphBuilder {
   // StackChecks.
   void BuildFunctionEntryStackCheck();
   void BuildIterationBodyStackCheck();
+  void MaybeBuildOSREntryStackCheck();
 
   // Control flow plumbing.
   void BuildJump();
@@ -434,6 +479,7 @@ class BytecodeGraphBuilder {
   Environment* environment_;
   bool const osr_;
   int currently_peeled_loop_offset_;
+  bool is_osr_entry_stack_check_pending_;
 
   const bool skip_first_stack_check_;
 
@@ -1022,6 +1068,7 @@ BytecodeGraphBuilder::BytecodeGraphBuilder(
       environment_(nullptr),
       osr_(!osr_offset.IsNone()),
       currently_peeled_loop_offset_(-1),
+      is_osr_entry_stack_check_pending_(osr_),
       skip_first_stack_check_(flags &
                               BytecodeGraphBuilderFlag::kSkipFirstStackCheck),
       merge_environments_(local_zone),
@@ -1239,36 +1286,18 @@ void BytecodeGraphBuilder::PrepareEagerCheckpoint() {
 #endif  // DEBUG
 }
 
-void BytecodeGraphBuilder::PrepareFrameState(Node* node,
-                                             OutputFrameStateCombine combine) {
-  if (OperatorProperties::HasFrameStateInput(node->op())) {
-    PrepareFrameState(node, combine,
-                      BailoutId(bytecode_iterator().current_offset()));
-  }
-}
-
-void BytecodeGraphBuilder::PrepareFrameState(Node* node,
-                                             OutputFrameStateCombine combine,
-                                             BailoutId bailout_id) {
+void BytecodeGraphBuilder::PrepareFrameState(
+    Node* node, OutputFrameStateCombine combine, BailoutId bailout_id,
+    const BytecodeLivenessState* liveness) {
   if (OperatorProperties::HasFrameStateInput(node->op())) {
     // Add the frame state for after the operation. The node in question has
     // already been created and had a {Dead} frame state input up until now.
     DCHECK_EQ(1, OperatorProperties::GetFrameStateInputCount(node->op()));
     DCHECK_EQ(IrOpcode::kDead,
               NodeProperties::GetFrameStateInput(node)->opcode());
-    DCHECK_IMPLIES(bailout_id.ToInt() == kFunctionEntryBytecodeOffset,
-                   bytecode_iterator().current_offset() == 0);
-
-    // If we have kFunctionEntryBytecodeOffset as the bailout_id, we want to get
-    // the liveness at the moment of function entry. This is the same as the IN
-    // liveness of the first actual bytecode.
-    const BytecodeLivenessState* liveness_after =
-        bailout_id.ToInt() == kFunctionEntryBytecodeOffset
-            ? bytecode_analysis().GetInLivenessFor(0)
-            : bytecode_analysis().GetOutLivenessFor(bailout_id.ToInt());
 
     Node* frame_state_after =
-        environment()->Checkpoint(bailout_id, combine, liveness_after);
+        environment()->Checkpoint(bailout_id, combine, liveness);
     NodeProperties::ReplaceFrameStateInput(node, frame_state_after);
   }
 }
@@ -1378,8 +1407,7 @@ void BytecodeGraphBuilder::BuildFunctionEntryStackCheck() {
   if (!skip_first_stack_check()) {
     Node* node =
         NewNode(javascript()->StackCheck(StackCheckKind::kJSFunctionEntry));
-    PrepareFrameState(node, OutputFrameStateCombine::Ignore(),
-                      BailoutId(kFunctionEntryBytecodeOffset));
+    PrepareFrameStateForFunctionEntryStackCheck(node);
   }
 }
 
@@ -1389,6 +1417,15 @@ void BytecodeGraphBuilder::BuildIterationBodyStackCheck() {
   environment()->RecordAfterState(node, Environment::kAttachFrameState);
 }
 
+void BytecodeGraphBuilder::MaybeBuildOSREntryStackCheck() {
+  if (V8_UNLIKELY(is_osr_entry_stack_check_pending_)) {
+    is_osr_entry_stack_check_pending_ = false;
+    Node* node =
+        NewNode(javascript()->StackCheck(StackCheckKind::kJSFunctionEntry));
+    PrepareFrameStateForOSREntryStackCheck(node);
+  }
+}
+
 // We will iterate through the OSR loop, then its parent, and so on
 // until we have reached the outmost loop containing the OSR loop. We do
 // not generate nodes for anything before the outermost loop.
@@ -1469,6 +1506,13 @@ void BytecodeGraphBuilder::VisitSingleBytecode() {
 
   if (environment() != nullptr) {
     BuildLoopHeaderEnvironment(current_offset);
+
+    // The OSR-entry stack check must be emitted during the first call to
+    // VisitSingleBytecode in an OSR'd function. We don't know if that call
+    // will be made from AdvanceToOsrEntryAndPeelLoops or from VisitBytecodes,
+    // therefore we insert the logic here inside VisitSingleBytecode itself.
+    MaybeBuildOSREntryStackCheck();
+
     switch (bytecode_iterator().current_bytecode()) {
 #define BYTECODE_CASE(name, ...)       \
   case interpreter::Bytecode::k##name: \

test/mjsunit/mjsunit.status

@@ -136,6 +136,7 @@
   'regress/regress-1122': [PASS, SLOW],
   'regress/regress-605470': [PASS, SLOW],
   'regress/regress-655573': [PASS, SLOW],
+  'regress/regress-1034322': [PASS, SLOW, NO_VARIANTS, ['mode != release', SKIP]],
   'regress/regress-1200351': [PASS, SLOW],
   'regress/regress-crbug-808192': [PASS, SLOW, NO_VARIANTS, ['arch not in (ia32, x64)', SKIP]],
   'regress/regress-crbug-918301': [PASS, SLOW, NO_VARIANTS, ['mode != release or dcheck_always_on', SKIP], ['(arch == arm or arch == arm64) and simulator_run', SKIP], ['tsan', SKIP]],

test/mjsunit/regress/regress-1034322.js

+// Copyright 2021 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: --allow-natives-syntax --stack-size=103
+
+let ticks = 0;
+
+function v0() {
+  try { v1(); } catch {}
+  // This triggers the deopt that may overflow the stack.
+  try { undefined[null] = null; } catch {}
+}
+
+function v1() {
+  while (!v0()) {
+    // Trigger OSR early to get a crashing case asap.
+    if (ticks == 5) %OptimizeOsr();
+    // With the bug fixed, there's no easy way to trigger termination. Instead,
+    // run until we reach a certain number of ticks. The crash triggers locally
+    // at tick 7562, thus running until 20k ticks to be somewhat safe.
+    if (ticks >= 20000) exit(0);
+    ticks++;
+  }
+}
+
+%PrepareFunctionForOptimization(v0);
+%PrepareFunctionForOptimization(v1);
+
+v0();