[compiler] Check for stack overflow when unrolling JSBoundFunctions

Gracefully handle hugely nested JSBoundFunctions by checking against
the local isolate's stack limit in relevant recursive functions.

This is based on d734bb4c (which was
reverted).

In order to get access to the local isolate, the CL replaces the heap
broker's LocalHeap pointer with a LocalIsolate pointer.

Bug: chromium:1125145
Change-Id: I15d6265c7dfcd8a70af4ab4ce6f30149a886be00
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2480682
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70654}

src/compiler/heap-refs.h

@@ -13,6 +13,7 @@
 #include "src/utils/boxed-float.h"
 
 namespace v8 {
+
 class CFunctionInfo;
 
 namespace internal {
@@ -32,6 +33,7 @@ class NativeContext;
 class ScriptContextTable;
 
 namespace compiler {
+
 // Whether we are loading a property or storing to a property.
 // For a store during literal creation, do not walk up the prototype chain.
 enum class AccessMode { kLoad, kStore, kStoreInLiteral, kHas };
@@ -317,7 +319,7 @@ class JSBoundFunctionRef : public JSObjectRef {
 
   Handle<JSBoundFunction> object() const;
 
-  void Serialize();
+  bool Serialize();
   bool serialized() const;
 
   // The following are available only after calling Serialize().

src/compiler/js-call-reducer.cc

@@ -3990,6 +3990,8 @@ bool JSCallReducer::IsBuiltinOrApiFunction(JSFunctionRef function) const {
 }
 
 Reduction JSCallReducer::ReduceJSCall(Node* node) {
+  if (broker()->StackHasOverflowed()) return NoChange();
+
   JSCallNode n(node);
   CallParameters const& p = n.Parameters();
   Node* target = n.target();

src/compiler/js-heap-broker.cc

@@ -628,7 +628,7 @@ class JSBoundFunctionData : public JSObjectData {
   JSBoundFunctionData(JSHeapBroker* broker, ObjectData** storage,
                       Handle<JSBoundFunction> object);
 
-  void Serialize(JSHeapBroker* broker);
+  bool Serialize(JSHeapBroker* broker);
   bool serialized() const { return serialized_; }
 
   ObjectData* bound_target_function() const { return bound_target_function_; }
@@ -1514,19 +1514,24 @@ JSBoundFunctionData::JSBoundFunctionData(JSHeapBroker* broker,
                                          Handle<JSBoundFunction> object)
     : JSObjectData(broker, storage, object) {}
 
-void JSBoundFunctionData::Serialize(JSHeapBroker* broker) {
-  if (serialized_) return;
-  serialized_ = true;
+bool JSBoundFunctionData::Serialize(JSHeapBroker* broker) {
+  if (serialized_) return true;
+  if (broker->StackHasOverflowed()) return false;
 
   TraceScope tracer(broker, this, "JSBoundFunctionData::Serialize");
   Handle<JSBoundFunction> function = Handle<JSBoundFunction>::cast(object());
 
+  // We set {serialized_} at the end in order to correctly handle the case where
+  // a recursive call to this method reaches the stack limit.
+  bool serialized = true;
+
   DCHECK_NULL(bound_target_function_);
   bound_target_function_ =
       broker->GetOrCreateData(function->bound_target_function());
   if (!bound_target_function_->should_access_heap()) {
     if (bound_target_function_->IsJSBoundFunction()) {
-      bound_target_function_->AsJSBoundFunction()->Serialize(broker);
+      serialized =
+          bound_target_function_->AsJSBoundFunction()->Serialize(broker);
     } else if (bound_target_function_->IsJSFunction()) {
       bound_target_function_->AsJSFunction()->Serialize(broker);
     }
@@ -1540,6 +1545,9 @@ void JSBoundFunctionData::Serialize(JSHeapBroker* broker) {
 
   DCHECK_NULL(bound_this_);
   bound_this_ = broker->GetOrCreateData(function->bound_this());
+
+  serialized_ = serialized;
+  return serialized;
 }
 
 JSObjectData::JSObjectData(JSHeapBroker* broker, ObjectData** storage,
@@ -2441,7 +2449,7 @@ JSHeapBroker::JSHeapBroker(Isolate* isolate, Zone* broker_zone,
   TRACE(this, "Constructing heap broker");
 }
 
-JSHeapBroker::~JSHeapBroker() { DCHECK(!local_heap_); }
+JSHeapBroker::~JSHeapBroker() { DCHECK_NULL(local_isolate_); }
 
 void JSHeapBroker::SetPersistentAndCopyCanonicalHandlesForTesting(
     std::unique_ptr<PersistentHandles> persistent_handles,
@@ -2471,21 +2479,22 @@ std::string JSHeapBroker::Trace() const {
   return oss.str();
 }
 
-void JSHeapBroker::InitializeLocalHeap(OptimizedCompilationInfo* info,
-                                       LocalHeap* local_heap) {
+void JSHeapBroker::AttachLocalIsolate(OptimizedCompilationInfo* info,
+                                      LocalIsolate* local_isolate) {
   set_canonical_handles(info->DetachCanonicalHandles());
-  DCHECK_NULL(local_heap_);
-  local_heap_ = local_heap;
-  DCHECK_NOT_NULL(local_heap_);
-  local_heap_->AttachPersistentHandles(info->DetachPersistentHandles());
+  DCHECK_NULL(local_isolate_);
+  local_isolate_ = local_isolate;
+  DCHECK_NOT_NULL(local_isolate_);
+  local_isolate_->heap()->AttachPersistentHandles(
+      info->DetachPersistentHandles());
 }
 
-void JSHeapBroker::TearDownLocalHeap(OptimizedCompilationInfo* info) {
+void JSHeapBroker::DetachLocalIsolate(OptimizedCompilationInfo* info) {
   DCHECK_NULL(ph_);
-  DCHECK(local_heap_);
+  DCHECK_NOT_NULL(local_isolate_);
   std::unique_ptr<PersistentHandles> ph =
-      local_heap_->DetachPersistentHandles();
-  local_heap_ = nullptr;
+      local_isolate_->heap()->DetachPersistentHandles();
+  local_isolate_ = nullptr;
   info->set_canonical_handles(DetachCanonicalHandles());
   info->set_persistent_handles(std::move(ph));
 }
@@ -4357,10 +4366,10 @@ bool JSTypedArrayRef::serialized() const {
   return data()->AsJSTypedArray()->serialized();
 }
 
-void JSBoundFunctionRef::Serialize() {
-  if (data_->should_access_heap()) return;
+bool JSBoundFunctionRef::Serialize() {
+  if (data_->should_access_heap()) return true;
   CHECK_EQ(broker()->mode(), JSHeapBroker::kSerializing);
-  data()->AsJSBoundFunction()->Serialize(broker());
+  return data()->AsJSBoundFunction()->Serialize(broker());
 }
 
 void PropertyCellRef::Serialize() {
@@ -5331,6 +5340,14 @@ BytecodeAnalysis const& JSHeapBroker::GetBytecodeAnalysis(
   return *analysis;
 }
 
+bool JSHeapBroker::StackHasOverflowed() const {
+  DCHECK_IMPLIES(local_isolate_ == nullptr,
+                 ThreadId::Current() == isolate_->thread_id());
+  return (local_isolate_ != nullptr)
+             ? StackLimitCheck::HasOverflowed(local_isolate_)
+             : StackLimitCheck(isolate_).HasOverflowed();
+}
+
 OffHeapBytecodeArray::OffHeapBytecodeArray(BytecodeArrayRef bytecode_array)
     : array_(bytecode_array) {}
 

src/compiler/js-heap-broker.h

@@ -14,6 +14,7 @@
 #include "src/compiler/processed-feedback.h"
 #include "src/compiler/refs-map.h"
 #include "src/compiler/serializer-hints.h"
+#include "src/execution/local-isolate.h"
 #include "src/handles/handles.h"
 #include "src/handles/persistent-handles.h"
 #include "src/heap/local-heap.h"
@@ -33,6 +34,7 @@ namespace compiler {
 
 class BytecodeAnalysis;
 class ObjectRef;
+
 std::ostream& operator<<(std::ostream& os, const ObjectRef& ref);
 
 #define TRACE_BROKER(broker, x)                                      \
@@ -115,17 +117,22 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
 
   enum BrokerMode { kDisabled, kSerializing, kSerialized, kRetired };
   BrokerMode mode() const { return mode_; }
-  // Initialize the local heap with the persistent and canonical handles
-  // provided by {info}.
-  void InitializeLocalHeap(OptimizedCompilationInfo* info,
-                           LocalHeap* local_heap);
-  // Tear down the local heap and pass the persistent and canonical handles
-  // provided back to {info}. {info} is responsible for disposing of them.
-  void TearDownLocalHeap(OptimizedCompilationInfo* info);
+
   void StopSerializing();
   void Retire();
   bool SerializingAllowed() const;
 
+  // Remember the local isolate and initialize its local heap with the
+  // persistent and canonical handles provided by {info}.
+  void AttachLocalIsolate(OptimizedCompilationInfo* info,
+                          LocalIsolate* local_isolate);
+  // Forget about the local isolate and pass the persistent and canonical
+  // handles provided back to {info}. {info} is responsible for disposing of
+  // them.
+  void DetachLocalIsolate(OptimizedCompilationInfo* info);
+
+  bool StackHasOverflowed() const;
+
 #ifdef DEBUG
   void PrintRefsAnalysis() const;
 #endif  // DEBUG
@@ -228,7 +235,7 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   bool IsSerializedForCompilation(const SharedFunctionInfoRef& shared,
                                   const FeedbackVectorRef& feedback) const;
 
-  LocalHeap* local_heap() { return local_heap_; }
+  LocalIsolate* local_isolate() const { return local_isolate_; }
 
   // Return the corresponding canonical persistent handle for {object}. Create
   // one if it does not exist.
@@ -252,8 +259,9 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
       auto find_result = canonical_handles_->FindOrInsert(obj);
       if (!find_result.already_exists) {
         // Allocate new PersistentHandle if one wasn't created before.
-        DCHECK(local_heap_);
-        *find_result.entry = local_heap_->NewPersistentHandle(obj).location();
+        DCHECK_NOT_NULL(local_isolate());
+        *find_result.entry =
+            local_isolate()->heap()->NewPersistentHandle(obj).location();
       }
       return Handle<T>(*find_result.entry);
     } else {
@@ -360,7 +368,7 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   bool const is_concurrent_inlining_;
   CodeKind const code_kind_;
   std::unique_ptr<PersistentHandles> ph_;
-  LocalHeap* local_heap_ = nullptr;
+  LocalIsolate* local_isolate_ = nullptr;
   std::unique_ptr<CanonicalHandlesMap> canonical_handles_;
   unsigned trace_indentation_ = 0;
   PerIsolateCompilerCache* compiler_cache_ = nullptr;
@@ -452,7 +460,7 @@ class OffHeapBytecodeArray final : public interpreter::AbstractBytecodeArray {
 
 // Scope that unparks the LocalHeap, if:
 //   a) We have a JSHeapBroker,
-//   b) Said JSHeapBroker has a LocalHeap,
+//   b) Said JSHeapBroker has a LocalIsolate and thus a LocalHeap,
 //   c) Said LocalHeap has been parked and
 //   d) The given condition evaluates to true.
 // Used, for example, when printing the graph with --trace-turbo with a
@@ -462,9 +470,9 @@ class UnparkedScopeIfNeeded {
   explicit UnparkedScopeIfNeeded(JSHeapBroker* broker,
                                  bool extra_condition = true) {
     if (broker != nullptr && extra_condition) {
-      LocalHeap* local_heap = broker->local_heap();
-      if (local_heap != nullptr && local_heap->IsParked()) {
-        unparked_scope.emplace(local_heap);
+      LocalIsolate* local_isolate = broker->local_isolate();
+      if (local_isolate != nullptr && local_isolate->heap()->IsParked()) {
+        unparked_scope.emplace(local_isolate->heap());
       }
     }
   }

src/compiler/pipeline.cc

@@ -766,20 +766,21 @@ class PipelineRunScope {
   RuntimeCallTimerScope runtime_call_timer_scope;
 };
 
-// LocalHeapScope encapsulates the phase where persistent handles are attached
-// to the LocalHeap.
-class LocalHeapScope {
+// LocalIsolateScope encapsulates the phase where persistent handles are
+// attached to the LocalHeap inside {local_isolate}.
+class LocalIsolateScope {
  public:
-  explicit LocalHeapScope(JSHeapBroker* broker, OptimizedCompilationInfo* info,
-                          LocalHeap* local_heap)
+  explicit LocalIsolateScope(JSHeapBroker* broker,
+                             OptimizedCompilationInfo* info,
+                             LocalIsolate* local_isolate)
       : broker_(broker), info_(info) {
-    broker_->InitializeLocalHeap(info_, local_heap);
-    info_->tick_counter().AttachLocalHeap(broker_->local_heap());
+    broker_->AttachLocalIsolate(info_, local_isolate);
+    info_->tick_counter().AttachLocalHeap(local_isolate->heap());
   }
 
-  ~LocalHeapScope() {
+  ~LocalIsolateScope() {
     info_->tick_counter().DetachLocalHeap();
-    broker_->TearDownLocalHeap(info_);
+    broker_->DetachLocalIsolate(info_);
   }
 
  private:
@@ -1185,8 +1186,8 @@ PipelineCompilationJob::Status PipelineCompilationJob::ExecuteJobImpl(
   // Ensure that the RuntimeCallStats table is only available during execution
   // and not during finalization as that might be on a different thread.
   PipelineJobScope scope(&data_, stats);
-  LocalHeapScope local_heap_scope(data_.broker(), data_.info(),
-                                  local_isolate->heap());
+  LocalIsolateScope local_isolate_scope(data_.broker(), data_.info(),
+                                        local_isolate);
 
   if (data_.broker()->is_concurrent_inlining()) {
     if (!pipeline_.CreateGraph()) {
@@ -3018,7 +3019,7 @@ MaybeHandle<Code> Pipeline::GenerateCodeForTesting(
     info->ReopenHandlesInNewHandleScope(isolate);
     pipeline.Serialize();
     // Emulating the proper pipeline, we call CreateGraph on different places
-    // (i.e before or after creating a LocalHeapScope) depending on
+    // (i.e before or after creating a LocalIsolateScope) depending on
     // is_concurrent_inlining.
     if (!data.broker()->is_concurrent_inlining()) {
       if (!pipeline.CreateGraph()) return MaybeHandle<Code>();
@@ -3027,7 +3028,7 @@ MaybeHandle<Code> Pipeline::GenerateCodeForTesting(
 
   {
     LocalIsolate local_isolate(isolate, ThreadKind::kMain);
-    LocalHeapScope local_heap_scope(data.broker(), info, local_isolate.heap());
+    LocalIsolateScope local_isolate_scope(data.broker(), info, &local_isolate);
     if (data.broker()->is_concurrent_inlining()) {
       if (!pipeline.CreateGraph()) return MaybeHandle<Code>();
     }

src/compiler/serializer-for-background-compilation.cc

@@ -2084,7 +2084,7 @@ void SerializerForBackgroundCompilation::ProcessCalleeForCallOrConstruct(
   if (callee->IsJSBoundFunction()) {
     JSBoundFunctionRef bound_function(broker(),
                                       Handle<JSBoundFunction>::cast(callee));
-    bound_function.Serialize();
+    if (!bound_function.Serialize()) return;
     callee = UnrollBoundFunction(bound_function, broker(), arguments,
                                  &expanded_arguments, zone())
                  .object();

test/mjsunit/compiler/regress-1125145.js

+// 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: --allow-natives-syntax --opt
+
+function foo() {}
+for (let i = 0; i < 100000; ++i) {
+  foo = foo.bind();
+}
+
+function main() {
+  foo();
+}
+
+%PrepareFunctionForOptimization(main);
+main();
+%OptimizeFunctionOnNextCall(main);
+main();

test/mjsunit/mjsunit.status

@@ -191,6 +191,7 @@
   # Skip slow tests in debug mode.
   'array-functions-prototype-misc': [SKIP],
   'compiler/regress-808472': [SKIP],
+  'compiler/regress-1125145': [SKIP],
   'es6/promise-all-overflow-2': [SKIP],
   'generated-transition-stub': [SKIP],
   'regress/regress-524': [SKIP],
@@ -627,6 +628,7 @@
   'es6/large-classes-properties': [SKIP],
 
   # Slow tests.
+  'compiler/regress-1125145': [SKIP],
   'es6/block-conflicts-sloppy': [PASS, SLOW],
   'math-floor-part1': [PASS, SLOW],
   'regress/regress-430201': [SKIP],