Reland "[builtins] Support embedded builtins in nosnapshot builds"

This is a reland of bf2f0a02

Original change's description:
> [builtins] Support embedded builtins in nosnapshot builds
>
> This CL adds support for embedded builtins in nosnap builds by creating
> and setting an 'embedded blob' after builtin generation. Unlike
> snapshot builds, the blob is not embedded into the .text section but
> located on the C++ heap.
>
> This makes nosnap builds more consistent with mksnapshot, and allows us
> to simplify there and in serializer cctests.
>
> Complications arise from the different workflows we need to support:
>
> 1. the standard mksnapshot build process,
> 2. nosnap builds (which reuse the blob created by the first Isolate),
> 2. and tests with various complicated serialization workflows.
>
> To cover all of these cases, this CL introduces two knobs to twiddle:
>
> 1. A 'sticky' embedded blob which overrides compiled-in default
>    embedded blobs at Isolate setup.
> 2. The blob lifecycle can be managed manually or through refcounting.
>
> These are described in more detail in isolate.cc.
>
> Tbr: ulan@chromium.org
> Bug: v8:6666, v8:8350
> Change-Id: I3842e40cdaf45d2cadd05c6eb1ec2f5e3d83568d
> Reviewed-on: https://chromium-review.googlesource.com/c/1310195
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Reviewed-by: Yang Guo <yangguo@chromium.org>
> Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#57523}

Tbr: ulan@chromium.org,yangguo@chromium.org
Bug: v8:6666, v8:8350
Change-Id: I13b523c9e7406b39a3cd28465c06f17f1744a738
Reviewed-on: https://chromium-review.googlesource.com/c/1337578
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57540}

BUILD.gn

@@ -24,6 +24,8 @@ import("snapshot_toolchain.gni")
 is_target_simulator = (target_cpu != v8_target_cpu && !v8_multi_arch_build) ||
                       (current_cpu != v8_current_cpu && v8_multi_arch_build)
 
+is_msvc = is_win && !is_clang
+
 declare_args() {
   # Print to stdout on Android.
   v8_android_log_stdout = false
@@ -83,9 +85,8 @@ declare_args() {
   v8_enable_fast_mksnapshot = false
 
   # Enable embedded builtins.
-  # TODO(v8:6666): Support no-snapshot builds, aix and MSVC.
-  v8_enable_embedded_builtins =
-      v8_use_snapshot && !is_aix && (!is_win || is_clang)
+  # TODO(v8:6666): Support aix and MSVC.
+  v8_enable_embedded_builtins = !is_aix && !is_msvc
 
   # Build-time flag for enabling nojit mode.
   # TODO(v8:7777): Remove the build-time flag once the --jitless runtime flag
@@ -220,8 +221,6 @@ if (v8_check_microtasks_scopes_consistency == "") {
       v8_enable_debugging_features || dcheck_always_on
 }
 
-assert(!v8_enable_embedded_builtins || v8_use_snapshot,
-       "Embedded builtins only work with snapshots")
 assert(v8_current_cpu != "x86" || !v8_untrusted_code_mitigations,
        "Untrusted code mitigations are unsupported on ia32")
 

src/assembler.cc

@@ -65,17 +65,20 @@ AssemblerOptions AssemblerOptions::EnableV8AgnosticCode() const {
 AssemblerOptions AssemblerOptions::Default(
     Isolate* isolate, bool explicitly_support_serialization) {
   AssemblerOptions options;
-  bool serializer =
+  const bool serializer =
       isolate->serializer_enabled() || explicitly_support_serialization;
+  const bool generating_embedded_builtin =
+      isolate->ShouldLoadConstantsFromRootList();
   options.record_reloc_info_for_serialization = serializer;
-  options.enable_root_array_delta_access = !serializer;
+  options.enable_root_array_delta_access =
+      !serializer && !generating_embedded_builtin;
 #ifdef USE_SIMULATOR
   // Don't generate simulator specific code if we are building a snapshot, which
   // might be run on real hardware.
   options.enable_simulator_code = !serializer;
 #endif
-  options.inline_offheap_trampolines = !serializer;
-
+  options.inline_offheap_trampolines =
+      !serializer && !generating_embedded_builtin;
 #if V8_TARGET_ARCH_X64 || V8_TARGET_ARCH_ARM64
   const base::AddressRegion& code_range =
       isolate->heap()->memory_allocator()->code_range();

src/builtins/builtins.cc

@@ -296,7 +296,6 @@ class OffHeapTrampolineGenerator {
 // static
 Handle<Code> Builtins::GenerateOffHeapTrampolineFor(Isolate* isolate,
                                                     Address off_heap_entry) {
-  DCHECK(isolate->serializer_enabled());
   DCHECK_NOT_NULL(isolate->embedded_blob());
   DCHECK_NE(0, isolate->embedded_blob_size());
 

src/builtins/constants-table-builder.cc

@@ -35,8 +35,8 @@ uint32_t BuiltinsConstantsTableBuilder::AddObject(Handle<Object> object) {
   // Must be on the main thread.
   DCHECK(ThreadId::Current().Equals(isolate_->thread_id()));
 
-  // Must be serializing.
-  DCHECK(isolate_->serializer_enabled());
+  // Must be generating embedded builtin code.
+  DCHECK(isolate_->ShouldLoadConstantsFromRootList());
 #endif
 
   uint32_t* maybe_key = map_.Find(object);
@@ -62,7 +62,7 @@ void BuiltinsConstantsTableBuilder::PatchSelfReference(
   DCHECK_EQ(ReadOnlyRoots(isolate_).empty_fixed_array(),
             isolate_->heap()->builtins_constants_table());
 
-  DCHECK(isolate_->serializer_enabled());
+  DCHECK(isolate_->ShouldLoadConstantsFromRootList());
 
   DCHECK(self_reference->IsOddball());
   DCHECK(Oddball::cast(*self_reference)->kind() ==
@@ -86,7 +86,7 @@ void BuiltinsConstantsTableBuilder::Finalize() {
 
   DCHECK_EQ(ReadOnlyRoots(isolate_).empty_fixed_array(),
             isolate_->heap()->builtins_constants_table());
-  DCHECK(isolate_->serializer_enabled());
+  DCHECK(isolate_->ShouldLoadConstantsFromRootList());
 
   // An empty map means there's nothing to do.
   if (map_.size() == 0) return;

src/compiler/pipeline.cc

@@ -1569,7 +1569,8 @@ struct InstructionSelectionPhase {
         FLAG_turbo_instruction_scheduling
             ? InstructionSelector::kEnableScheduling
             : InstructionSelector::kDisableScheduling,
-        !data->isolate() || data->isolate()->serializer_enabled()
+        !data->isolate() || data->isolate()->serializer_enabled() ||
+                data->isolate()->ShouldLoadConstantsFromRootList()
             ? InstructionSelector::kDisableRootsRelativeAddressing
             : InstructionSelector::kEnableRootsRelativeAddressing,
         data->info()->GetPoisoningMitigationLevel(),

src/heap/factory.cc

@@ -2681,7 +2681,6 @@ Handle<Code> Factory::NewCode(
 
 Handle<Code> Factory::NewOffHeapTrampolineFor(Handle<Code> code,
                                               Address off_heap_entry) {
-  CHECK(isolate()->serializer_enabled());
   CHECK_NOT_NULL(isolate()->embedded_blob());
   CHECK_NE(0, isolate()->embedded_blob_size());
   CHECK(Builtins::IsIsolateIndependentBuiltin(*code));

src/isolate.cc

@@ -108,24 +108,103 @@ namespace {
 
 std::atomic<const uint8_t*> current_embedded_blob_(nullptr);
 std::atomic<uint32_t> current_embedded_blob_size_(0);
+
+// The various workflows around embedded snapshots are fairly complex. We need
+// to support plain old snapshot builds, nosnap builds, and the requirements of
+// subtly different serialization tests. There's two related knobs to twiddle:
+//
+// - The default embedded blob may be overridden by setting the sticky embedded
+// blob. This is set automatically whenever we create a new embedded blob.
+//
+// - Lifecycle management can be either manual or set to refcounting.
+//
+// A few situations to demonstrate their use:
+//
+// - A plain old snapshot build neither overrides the default blob nor
+// refcounts.
+//
+// - mksnapshot sets the sticky blob and manually frees the embedded
+// blob once done.
+//
+// - Most serializer tests do the same.
+//
+// - Nosnapshot builds set the sticky blob and enable refcounting.
+
+// This mutex protects access to the following variables:
+// - sticky_embedded_blob_
+// - sticky_embedded_blob_size_
+// - enable_embedded_blob_refcounting_
+// - current_embedded_blob_refs_
+base::LazyMutex current_embedded_blob_refcount_mutex_ = LAZY_MUTEX_INITIALIZER;
+
+const uint8_t* sticky_embedded_blob_ = nullptr;
+uint32_t sticky_embedded_blob_size_ = 0;
+
+bool enable_embedded_blob_refcounting_ = true;
+int current_embedded_blob_refs_ = 0;
+
+const uint8_t* StickyEmbeddedBlob() { return sticky_embedded_blob_; }
+uint32_t StickyEmbeddedBlobSize() { return sticky_embedded_blob_size_; }
+
+void SetStickyEmbeddedBlob(const uint8_t* blob, uint32_t blob_size) {
+  sticky_embedded_blob_ = blob;
+  sticky_embedded_blob_size_ = blob_size;
+}
+
 }  // namespace
 
+void DisableEmbeddedBlobRefcounting() {
+  base::MutexGuard guard(current_embedded_blob_refcount_mutex_.Pointer());
+  enable_embedded_blob_refcounting_ = false;
+}
+
+void FreeCurrentEmbeddedBlob() {
+  CHECK(!enable_embedded_blob_refcounting_);
+  base::MutexGuard guard(current_embedded_blob_refcount_mutex_.Pointer());
+
+  if (StickyEmbeddedBlob() == nullptr) return;
+
+  CHECK_EQ(StickyEmbeddedBlob(), Isolate::CurrentEmbeddedBlob());
+
+  InstructionStream::FreeOffHeapInstructionStream(
+      const_cast<uint8_t*>(Isolate::CurrentEmbeddedBlob()),
+      Isolate::CurrentEmbeddedBlobSize());
+
+  current_embedded_blob_.store(nullptr, std::memory_order_relaxed);
+  current_embedded_blob_size_.store(0, std::memory_order_relaxed);
+  sticky_embedded_blob_ = nullptr;
+  sticky_embedded_blob_size_ = 0;
+}
+
 void Isolate::SetEmbeddedBlob(const uint8_t* blob, uint32_t blob_size) {
+  CHECK_NOT_NULL(blob);
+
   embedded_blob_ = blob;
   embedded_blob_size_ = blob_size;
   current_embedded_blob_.store(blob, std::memory_order_relaxed);
   current_embedded_blob_size_.store(blob_size, std::memory_order_relaxed);
 
 #ifdef DEBUG
-  if (blob != nullptr) {
-    // Verify that the contents of the embedded blob are unchanged from
-    // serialization-time, just to ensure the compiler isn't messing with us.
-    EmbeddedData d = EmbeddedData::FromBlob();
-    CHECK_EQ(d.Hash(), d.CreateHash());
-  }
+  // Verify that the contents of the embedded blob are unchanged from
+  // serialization-time, just to ensure the compiler isn't messing with us.
+  EmbeddedData d = EmbeddedData::FromBlob();
+  CHECK_EQ(d.Hash(), d.CreateHash());
 #endif  // DEBUG
 }
 
+void Isolate::ClearEmbeddedBlob() {
+  CHECK(enable_embedded_blob_refcounting_);
+  CHECK_EQ(embedded_blob_, CurrentEmbeddedBlob());
+  CHECK_EQ(embedded_blob_, StickyEmbeddedBlob());
+
+  embedded_blob_ = nullptr;
+  embedded_blob_size_ = 0;
+  current_embedded_blob_.store(nullptr, std::memory_order_relaxed);
+  current_embedded_blob_size_.store(0, std::memory_order_relaxed);
+  sticky_embedded_blob_ = nullptr;
+  sticky_embedded_blob_size_ = 0;
+}
+
 const uint8_t* Isolate::embedded_blob() const { return embedded_blob_; }
 uint32_t Isolate::embedded_blob_size() const { return embedded_blob_size_; }
 
@@ -2723,17 +2802,7 @@ Isolate::Isolate(std::unique_ptr<i::IsolateAllocator> isolate_allocator)
   InitializeLoggingAndCounters();
   debug_ = new Debug(this);
 
-  if (FLAG_embedded_builtins) {
-#ifdef V8_MULTI_SNAPSHOTS
-  if (FLAG_untrusted_code_mitigations) {
-    SetEmbeddedBlob(DefaultEmbeddedBlob(), DefaultEmbeddedBlobSize());
-  } else {
-    SetEmbeddedBlob(TrustedEmbeddedBlob(), TrustedEmbeddedBlobSize());
-  }
-#else
-  SetEmbeddedBlob(DefaultEmbeddedBlob(), DefaultEmbeddedBlobSize());
-#endif
-  }
+  InitializeDefaultEmbeddedBlob();
 }
 
 void Isolate::CheckIsolateLayout() {
@@ -2836,14 +2905,7 @@ void Isolate::Deinit() {
     wasm_engine_.reset();
   }
 
-  if (FLAG_embedded_builtins) {
-    if (DefaultEmbeddedBlob() == nullptr && embedded_blob() != nullptr) {
-      // We own the embedded blob. Free it.
-      uint8_t* data = const_cast<uint8_t*>(embedded_blob_);
-      InstructionStream::FreeOffHeapInstructionStream(data,
-                                                      embedded_blob_size_);
-    }
-  }
+  TearDownEmbeddedBlob();
 
   delete interpreter_;
   interpreter_ = nullptr;
@@ -3015,7 +3077,6 @@ void PrintBuiltinSizes(Isolate* isolate) {
 }
 
 void CreateOffHeapTrampolines(Isolate* isolate) {
-  DCHECK(isolate->serializer_enabled());
   DCHECK_NOT_NULL(isolate->embedded_blob());
   DCHECK_NE(0, isolate->embedded_blob_size());
 
@@ -3047,23 +3108,75 @@ void CreateOffHeapTrampolines(Isolate* isolate) {
 }
 }  // namespace
 
-void Isolate::PrepareEmbeddedBlobForSerialization() {
-  // When preparing the embedded blob, ensure it doesn't exist yet.
-  DCHECK_NULL(embedded_blob());
-  DCHECK_NULL(DefaultEmbeddedBlob());
-  DCHECK(serializer_enabled());
-
-  // The isolate takes ownership of this pointer into an executable mmap'd
-  // area. We muck around with const-casts because the standard use-case in
-  // shipping builds is for embedded_blob_ to point into a read-only
-  // .text-embedded section.
-  uint8_t* data;
-  uint32_t size;
-  InstructionStream::CreateOffHeapInstructionStream(this, &data, &size);
-  SetEmbeddedBlob(const_cast<const uint8_t*>(data), size);
+void Isolate::InitializeDefaultEmbeddedBlob() {
+  const uint8_t* blob = DefaultEmbeddedBlob();
+  uint32_t size = DefaultEmbeddedBlobSize();
+
+#ifdef V8_MULTI_SNAPSHOTS
+  if (!FLAG_untrusted_code_mitigations) {
+    blob = TrustedEmbeddedBlob();
+    size = TrustedEmbeddedBlobSize();
+  }
+#endif
+
+  if (StickyEmbeddedBlob() != nullptr) {
+    base::MutexGuard guard(current_embedded_blob_refcount_mutex_.Pointer());
+    // Check again now that we hold the lock.
+    if (StickyEmbeddedBlob() != nullptr) {
+      blob = StickyEmbeddedBlob();
+      size = StickyEmbeddedBlobSize();
+      current_embedded_blob_refs_++;
+    }
+  }
+
+  if (blob == nullptr) {
+    CHECK_EQ(0, size);
+  } else {
+    SetEmbeddedBlob(blob, size);
+  }
+}
+
+void Isolate::CreateAndSetEmbeddedBlob() {
+  base::MutexGuard guard(current_embedded_blob_refcount_mutex_.Pointer());
+
+  // If a sticky blob has been set, we reuse it.
+  if (StickyEmbeddedBlob() != nullptr) {
+    CHECK_EQ(embedded_blob(), StickyEmbeddedBlob());
+    CHECK_EQ(CurrentEmbeddedBlob(), StickyEmbeddedBlob());
+  } else {
+    // Create and set a new embedded blob.
+    uint8_t* data;
+    uint32_t size;
+    InstructionStream::CreateOffHeapInstructionStream(this, &data, &size);
+
+    CHECK_EQ(0, current_embedded_blob_refs_);
+    const uint8_t* const_data = const_cast<const uint8_t*>(data);
+    SetEmbeddedBlob(const_data, size);
+    current_embedded_blob_refs_++;
+
+    SetStickyEmbeddedBlob(const_data, size);
+  }
+
   CreateOffHeapTrampolines(this);
 }
 
+void Isolate::TearDownEmbeddedBlob() {
+  // Nothing to do in case the blob is embedded into the binary or unset.
+  if (StickyEmbeddedBlob() == nullptr) return;
+
+  CHECK_EQ(embedded_blob(), StickyEmbeddedBlob());
+  CHECK_EQ(CurrentEmbeddedBlob(), StickyEmbeddedBlob());
+
+  base::MutexGuard guard(current_embedded_blob_refcount_mutex_.Pointer());
+  current_embedded_blob_refs_--;
+  if (current_embedded_blob_refs_ == 0 && enable_embedded_blob_refcounting_) {
+    // We own the embedded blob and are the last holder. Free it.
+    InstructionStream::FreeOffHeapInstructionStream(
+        const_cast<uint8_t*>(embedded_blob()), embedded_blob_size());
+    ClearEmbeddedBlob();
+  }
+}
+
 bool Isolate::Init(StartupDeserializer* des) {
   TRACE_ISOLATE(init);
 
@@ -3166,19 +3279,16 @@ bool Isolate::Init(StartupDeserializer* des) {
 
   bootstrapper_->Initialize(create_heap_objects);
 
-  if (FLAG_embedded_builtins) {
-    if (create_heap_objects && serializer_enabled()) {
-      builtins_constants_table_builder_ =
-          new BuiltinsConstantsTableBuilder(this);
-    }
+  if (FLAG_embedded_builtins && create_heap_objects) {
+    builtins_constants_table_builder_ = new BuiltinsConstantsTableBuilder(this);
   }
   setup_delegate_->SetupBuiltins(this);
-  if (FLAG_embedded_builtins) {
-    if (create_heap_objects && serializer_enabled()) {
-      builtins_constants_table_builder_->Finalize();
-      delete builtins_constants_table_builder_;
-      builtins_constants_table_builder_ = nullptr;
-    }
+  if (FLAG_embedded_builtins && create_heap_objects) {
+    builtins_constants_table_builder_->Finalize();
+    delete builtins_constants_table_builder_;
+    builtins_constants_table_builder_ = nullptr;
+
+    CreateAndSetEmbeddedBlob();
   }
 
   if (create_heap_objects) heap_.CreateFixedStubs();

src/isolate.h

@@ -335,6 +335,11 @@ class WasmEngine;
   inline void set_##name(type v) { name##_ = v; }  \
   inline type name() const { return name##_; }
 
+// Controls for manual embedded blob lifecycle management, used by tests and
+// mksnapshot.
+V8_EXPORT_PRIVATE void DisableEmbeddedBlobRefcounting();
+V8_EXPORT_PRIVATE void FreeCurrentEmbeddedBlob();
+
 class ThreadLocalTop {
  public:
   // Does early low-level initialization that does not depend on the
@@ -1462,20 +1467,12 @@ class Isolate final : private HiddenFactory {
 
   // Off-heap builtins cannot embed constants within the code object itself,
   // and thus need to load them from the root list.
+  // TODO(jgruber): Rename to IsGeneratingEmbeddedBuiltins().
   bool ShouldLoadConstantsFromRootList() const {
-    if (FLAG_embedded_builtins) {
-      return (serializer_enabled() &&
-              builtins_constants_table_builder() != nullptr);
-    } else {
-      return false;
-    }
+    return FLAG_embedded_builtins &&
+           builtins_constants_table_builder() != nullptr;
   }
 
-  // Called only prior to serialization.
-  // This function copies off-heap-safe builtins off the heap, creates off-heap
-  // trampolines, and sets up this isolate's embedded blob.
-  void PrepareEmbeddedBlobForSerialization();
-
   BuiltinsConstantsTableBuilder* builtins_constants_table_builder() const {
     return builtins_constants_table_builder_;
   }
@@ -1863,7 +1860,12 @@ class Isolate final : private HiddenFactory {
   // which is stored on the root list prior to serialization.
   BuiltinsConstantsTableBuilder* builtins_constants_table_builder_ = nullptr;
 
+  void InitializeDefaultEmbeddedBlob();
+  void CreateAndSetEmbeddedBlob();
+  void TearDownEmbeddedBlob();
+
   void SetEmbeddedBlob(const uint8_t* blob, uint32_t blob_size);
+  void ClearEmbeddedBlob();
 
   const uint8_t* embedded_blob_ = nullptr;
   uint32_t embedded_blob_size_ = 0;

src/snapshot/mksnapshot.cc

@@ -418,9 +418,7 @@ v8::StartupData WarmUpSnapshotDataBlob(v8::SnapshotCreator* snapshot_creator,
   return result;
 }
 
-void WriteEmbeddedFile(v8::SnapshotCreator* creator, SnapshotWriter* writer) {
-  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(creator->GetIsolate());
-  isolate->PrepareEmbeddedBlobForSerialization();
+void WriteEmbeddedFile(SnapshotWriter* writer) {
   i::EmbeddedData embedded_blob = i::EmbeddedData::FromBlob();
   writer->WriteEmbedded(&embedded_blob);
 }
@@ -463,6 +461,7 @@ int main(int argc, char** argv) {
     std::unique_ptr<char> warmup_script(
         GetExtraCode(argc >= 3 ? argv[2] : nullptr, "warm up"));
 
+    i::DisableEmbeddedBlobRefcounting();
     v8::StartupData blob;
     {
       v8::Isolate* isolate = v8::Isolate::Allocate();
@@ -478,13 +477,7 @@ int main(int argc, char** argv) {
         i_isolate->heap()->ConfigureHeap(0, 0, code_range_size);
       }
       v8::SnapshotCreator snapshot_creator(isolate);
-      if (i::FLAG_embedded_builtins) {
-        // This process is a bit tricky since we might go on to make a second
-        // snapshot if a warmup script is passed. In that case, create the first
-        // snapshot without off-heap trampolines and only move code off-heap for
-        // the warmed-up snapshot.
-        if (!warmup_script) WriteEmbeddedFile(&snapshot_creator, &writer);
-      }
+      if (i::FLAG_embedded_builtins) WriteEmbeddedFile(&writer);
       blob = CreateSnapshotDataBlob(&snapshot_creator, embed_script.get());
     }
 
@@ -492,9 +485,6 @@ int main(int argc, char** argv) {
       CHECK(blob.raw_size > 0 && blob.data != nullptr);
       v8::StartupData cold = blob;
       v8::SnapshotCreator snapshot_creator(nullptr, &cold);
-      if (i::FLAG_embedded_builtins) {
-        WriteEmbeddedFile(&snapshot_creator, &writer);
-      }
       blob = WarmUpSnapshotDataBlob(&snapshot_creator, warmup_script.get());
       delete[] cold.data;
     }
@@ -503,6 +493,7 @@ int main(int argc, char** argv) {
     writer.WriteSnapshot(blob);
     delete[] blob.data;
   }
+  i::FreeCurrentEmbeddedBlob();
 
   v8::V8::Dispose();
   v8::V8::ShutdownPlatform();

test/cctest/cctest.status

@@ -176,6 +176,13 @@
   'test-api/Float64Array': [SKIP],
 }],  # 'arch == arm64 and mode == debug and simulator_run'
 
+##############################################################################
+['arch == ia32', {
+  # Requires call to non-isolate-independent code target, unsupported by
+  # MacroAssembler::Call() on ia32.
+  'test-api/SetFunctionEntryHook': [SKIP],
+}],  # 'no_snap == False'
+
 ##############################################################################
 ['variant == nooptimization and (arch == arm or arch == arm64) and simulator_run', {
   # Slow tests: https://crbug.com/v8/7783

test/cctest/test-api.cc

@@ -14582,6 +14582,8 @@ class SetFunctionEntryHookTest {
   SymbolLocationMap symbol_locations_;
   InvocationMap invocations_;
 
+  i::Isolate* isolate_ = nullptr;
+
   static SetFunctionEntryHookTest* instance_;
 };
 SetFunctionEntryHookTest* SetFunctionEntryHookTest::instance_ = nullptr;
@@ -14668,8 +14670,8 @@ void SetFunctionEntryHookTest::OnJitEvent(const v8::JitCodeEvent* event) {
 void SetFunctionEntryHookTest::OnEntryHook(
     uintptr_t function, uintptr_t return_addr_location) {
   // Get the function's code object.
-  i::Code function_code =
-      i::Code::GetCodeFromTargetAddress(static_cast<i::Address>(function));
+  i::Code function_code = isolate_->heap()->GcSafeFindCodeForInnerPointer(
+      static_cast<i::Address>(function));
   CHECK(!function_code.is_null());
 
   // Then try and look up the caller's code object.
@@ -14795,7 +14797,9 @@ void SetFunctionEntryHookTest::RunTest() {
   create_params.entry_hook = EntryHook;
   create_params.code_event_handler = JitEvent;
   create_params.array_buffer_allocator = CcTest::array_buffer_allocator();
-  v8::Isolate* isolate = v8::Isolate::New(create_params);
+  v8::Isolate* isolate = v8::Isolate::Allocate();
+  isolate_ = reinterpret_cast<i::Isolate*>(isolate);
+  v8::Isolate::Initialize(isolate, create_params);
 
   {
     v8::Isolate::Scope scope(isolate);
@@ -14838,7 +14842,9 @@ void SetFunctionEntryHookTest::RunTest() {
   // Make sure a second isolate is unaffected by the previous entry hook.
   create_params = v8::Isolate::CreateParams();
   create_params.array_buffer_allocator = CcTest::array_buffer_allocator();
-  isolate = v8::Isolate::New(create_params);
+  isolate = v8::Isolate::Allocate();
+  isolate_ = reinterpret_cast<i::Isolate*>(isolate);
+  v8::Isolate::Initialize(isolate, create_params);
   {
     v8::Isolate::Scope scope(isolate);
 
@@ -14852,8 +14858,7 @@ void SetFunctionEntryHookTest::RunTest() {
   isolate->Dispose();
 }
 
-
-TEST(SetFunctionEntryHook) {
+UNINITIALIZED_TEST(SetFunctionEntryHook) {
   // FunctionEntryHook does not work well with experimental natives.
   // Experimental natives are compiled during snapshot deserialization.
   // This test breaks because InstallGetter (function from snapshot that

test/inspector/inspector-test.cc

@@ -21,6 +21,15 @@
 #include "test/inspector/isolate-data.h"
 #include "test/inspector/task-runner.h"
 
+namespace v8 {
+namespace internal {
+
+extern void DisableEmbeddedBlobRefcounting();
+extern void FreeCurrentEmbeddedBlob();
+
+}  // namespace internal
+}  // namespace v8
+
 namespace {
 
 std::vector<TaskRunner*> task_runners;
@@ -1074,6 +1083,7 @@ int main(int argc, char* argv[]) {
   v8::V8::SetFlagsFromCommandLine(&argc, argv, true);
   v8::V8::InitializeExternalStartupData(argv[0]);
   v8::V8::Initialize();
+  i::DisableEmbeddedBlobRefcounting();
 
   v8::base::Semaphore ready_semaphore(0);
 
@@ -1087,49 +1097,56 @@ int main(int argc, char* argv[]) {
     }
   }
 
-  IsolateData::SetupGlobalTasks frontend_extensions;
-  frontend_extensions.emplace_back(new UtilsExtension());
-  TaskRunner frontend_runner(std::move(frontend_extensions), true,
-                             &ready_semaphore, nullptr, false);
-  ready_semaphore.Wait();
+  {
+    IsolateData::SetupGlobalTasks frontend_extensions;
+    frontend_extensions.emplace_back(new UtilsExtension());
+    TaskRunner frontend_runner(std::move(frontend_extensions), true,
+                               &ready_semaphore, nullptr, false);
+    ready_semaphore.Wait();
+
+    int frontend_context_group_id = 0;
+    RunSyncTask(&frontend_runner,
+                [&frontend_context_group_id](IsolateData* data) {
+                  frontend_context_group_id = data->CreateContextGroup();
+                });
 
-  int frontend_context_group_id = 0;
-  RunSyncTask(&frontend_runner,
-              [&frontend_context_group_id](IsolateData* data) {
-                frontend_context_group_id = data->CreateContextGroup();
-              });
-
-  IsolateData::SetupGlobalTasks backend_extensions;
-  backend_extensions.emplace_back(new SetTimeoutExtension());
-  backend_extensions.emplace_back(new InspectorExtension());
-  TaskRunner backend_runner(std::move(backend_extensions), false,
-                            &ready_semaphore,
-                            startup_data.data ? &startup_data : nullptr, true);
-  ready_semaphore.Wait();
-  UtilsExtension::set_backend_task_runner(&backend_runner);
+    IsolateData::SetupGlobalTasks backend_extensions;
+    backend_extensions.emplace_back(new SetTimeoutExtension());
+    backend_extensions.emplace_back(new InspectorExtension());
+    TaskRunner backend_runner(
+        std::move(backend_extensions), false, &ready_semaphore,
+        startup_data.data ? &startup_data : nullptr, true);
+    ready_semaphore.Wait();
+    UtilsExtension::set_backend_task_runner(&backend_runner);
+
+    task_runners.push_back(&frontend_runner);
+    task_runners.push_back(&backend_runner);
+
+    for (int i = 1; i < argc; ++i) {
+      // Ignore unknown flags.
+      if (argv[i] == nullptr || argv[i][0] == '-') continue;
+
+      bool exists = false;
+      std::string chars = v8::internal::ReadFile(argv[i], &exists, true);
+      if (!exists) {
+        fprintf(stderr, "Internal error: script file doesn't exists: %s\n",
+                argv[i]);
+        Exit();
+      }
+      frontend_runner.Append(
+          new ExecuteStringTask(chars, frontend_context_group_id));
+    }
 
-  task_runners.push_back(&frontend_runner);
-  task_runners.push_back(&backend_runner);
+    frontend_runner.Join();
+    backend_runner.Join();
 
-  for (int i = 1; i < argc; ++i) {
-    // Ignore unknown flags.
-    if (argv[i] == nullptr || argv[i][0] == '-') continue;
+    UtilsExtension::ClearAllSessions();
+    delete startup_data.data;
 
-    bool exists = false;
-    std::string chars = v8::internal::ReadFile(argv[i], &exists, true);
-    if (!exists) {
-      fprintf(stderr, "Internal error: script file doesn't exists: %s\n",
-              argv[i]);
-      Exit();
-    }
-    frontend_runner.Append(
-        new ExecuteStringTask(chars, frontend_context_group_id));
+    // TaskRunners go out of scope here, which causes Isolate teardown and all
+    // running background tasks to be properly joined.
   }
 
-  frontend_runner.Join();
-  backend_runner.Join();
-
-  delete startup_data.data;
-  UtilsExtension::ClearAllSessions();
+  i::FreeCurrentEmbeddedBlob();
   return 0;
 }