From 7b6fbcd0a6700db498ad55db046ecda92c8ee8c1 Mon Sep 17 00:00:00 2001 From: Nikolaos Papaspyrou Date: Sun, 29 Jan 2023 17:18:08 +0100 Subject: [PATCH] Merge: [heap] Move the Stack object from ThreadLocalTop to Isolate This is just for nodejs, do not backmerge to 11.0. (cherry picked from commit 1e4b71d99fea5ea6bb4bf6420585a7819872bb0f) > Change-Id: I026a35af3bc6999a09b21f277756d4454c086343 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4152476 > Reviewed-by: Michael Lippautz > Reviewed-by: Omer Katz > Commit-Queue: Nikolaos Papaspyrou > Cr-Commit-Position: refs/heads/main@{#85445} Stack information is thread-specific and, until now, it was stored in a field in ThreadLocalTop. This CL moves stack information to the isolate and makes sure to update the stack start whenever a main thread enters the isolate. At the same time, the Stack object is refactored and simplified. As a side effect, after removing the Stack object, ThreadLocalTop satisfies the std::standard_layout trait; this fixes some issues observed with different C++ compilers. Bug: v8:13630 Bug: v8:13257 Change-Id: I4be1f04fe90699e1a6e456dad3e0dd623851acce --- src/execution/isolate.cc | 36 +++++++++++++++---------------- src/execution/isolate.h | 6 ++++++ src/execution/thread-local-top.cc | 2 -- src/execution/thread-local-top.h | 6 +----- src/heap/heap.cc | 4 +--- 5 files changed, 25 insertions(+), 29 deletions(-) diff --git a/src/execution/isolate.cc b/src/execution/isolate.cc index 4edf364e0a..be4fd400d2 100644 --- a/src/execution/isolate.cc +++ b/src/execution/isolate.cc @@ -3074,22 +3074,23 @@ void Isolate::AddSharedWasmMemory(Handle memory_object) { void Isolate::RecordStackSwitchForScanning() { Object current = root(RootIndex::kActiveContinuation); DCHECK(!current.IsUndefined()); - thread_local_top()->stack_.ClearStackSegments(); - wasm::StackMemory* stack = Managed::cast( - WasmContinuationObject::cast(current).stack()) - .get() - .get(); + stack().ClearStackSegments(); + wasm::StackMemory* wasm_stack = + Managed::cast( + WasmContinuationObject::cast(current).stack()) + .get() + .get(); current = WasmContinuationObject::cast(current).parent(); - thread_local_top()->stack_.SetStackStart( - reinterpret_cast(stack->base())); + heap()->SetStackStart(reinterpret_cast(wasm_stack->base())); // We don't need to add all inactive stacks. Only the ones in the active chain // may contain cpp heap pointers. while (!current.IsUndefined()) { auto cont = WasmContinuationObject::cast(current); - auto* stack = Managed::cast(cont.stack()).get().get(); - thread_local_top()->stack_.AddStackSegment( - reinterpret_cast(stack->base()), - reinterpret_cast(stack->jmpbuf()->sp)); + auto* wasm_stack = + Managed::cast(cont.stack()).get().get(); + stack().AddStackSegment( + reinterpret_cast(wasm_stack->base()), + reinterpret_cast(wasm_stack->jmpbuf()->sp)); current = cont.parent(); } } @@ -3377,20 +3378,13 @@ void Isolate::Delete(Isolate* isolate) { Isolate* saved_isolate = isolate->TryGetCurrent(); SetIsolateThreadLocals(isolate, nullptr); isolate->set_thread_id(ThreadId::Current()); - isolate->thread_local_top()->stack_ = - saved_isolate ? std::move(saved_isolate->thread_local_top()->stack_) - : ::heap::base::Stack(base::Stack::GetStackStart()); + isolate->heap()->SetStackStart(base::Stack::GetStackStart()); bool owns_shared_isolate = isolate->owns_shared_isolate_; Isolate* maybe_shared_isolate = isolate->shared_isolate_; isolate->Deinit(); - // Restore the saved isolate's stack. - if (saved_isolate) - saved_isolate->thread_local_top()->stack_ = - std::move(isolate->thread_local_top()->stack_); - #ifdef DEBUG non_disposed_isolates_--; #endif // DEBUG @@ -4647,6 +4641,10 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data, void Isolate::Enter() { Isolate* current_isolate = nullptr; PerIsolateThreadData* current_data = CurrentPerIsolateThreadData(); + + // Set the stack start for the main thread that enters the isolate. + heap()->SetStackStart(base::Stack::GetStackStart()); + if (current_data != nullptr) { current_isolate = current_data->isolate_; DCHECK_NOT_NULL(current_isolate); diff --git a/src/execution/isolate.h b/src/execution/isolate.h index a32f999fe5..1cb6e10661 100644 --- a/src/execution/isolate.h +++ b/src/execution/isolate.h @@ -32,6 +32,7 @@ #include "src/execution/stack-guard.h" #include "src/handles/handles.h" #include "src/handles/traced-handles.h" +#include "src/heap/base/stack.h" #include "src/heap/factory.h" #include "src/heap/heap.h" #include "src/heap/read-only-heap.h" @@ -2022,6 +2023,8 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory { SimulatorData* simulator_data() { return simulator_data_; } #endif + ::heap::base::Stack& stack() { return stack_; } + #ifdef V8_ENABLE_WEBASSEMBLY wasm::StackMemory*& wasm_stacks() { return wasm_stacks_; } // Update the thread local's Stack object so that it is aware of the new stack @@ -2520,6 +2523,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory { // The mutex only guards adding pages, the retrieval is signal safe. base::Mutex code_pages_mutex_; + // Stack information for the main thread. + ::heap::base::Stack stack_; + #ifdef V8_ENABLE_WEBASSEMBLY wasm::StackMemory* wasm_stacks_; #endif diff --git a/src/execution/thread-local-top.cc b/src/execution/thread-local-top.cc index 0d7071ddda..05cc20b8e4 100644 --- a/src/execution/thread-local-top.cc +++ b/src/execution/thread-local-top.cc @@ -37,14 +37,12 @@ void ThreadLocalTop::Clear() { current_embedder_state_ = nullptr; failed_access_check_callback_ = nullptr; thread_in_wasm_flag_address_ = kNullAddress; - stack_ = ::heap::base::Stack(); } void ThreadLocalTop::Initialize(Isolate* isolate) { Clear(); isolate_ = isolate; thread_id_ = ThreadId::Current(); - stack_.SetStackStart(base::Stack::GetStackStart()); #if V8_ENABLE_WEBASSEMBLY thread_in_wasm_flag_address_ = reinterpret_cast
( trap_handler::GetThreadInWasmThreadLocalAddress()); diff --git a/src/execution/thread-local-top.h b/src/execution/thread-local-top.h index 43fec0a7df..989c817f31 100644 --- a/src/execution/thread-local-top.h +++ b/src/execution/thread-local-top.h @@ -10,7 +10,6 @@ #include "include/v8-unwinder.h" #include "src/common/globals.h" #include "src/execution/thread-id.h" -#include "src/heap/base/stack.h" #include "src/objects/contexts.h" #include "src/utils/utils.h" @@ -30,7 +29,7 @@ class ThreadLocalTop { // TODO(all): This is not particularly beautiful. We should probably // refactor this to really consist of just Addresses and 32-bit // integer fields. - static constexpr uint32_t kSizeInBytes = 30 * kSystemPointerSize; + static constexpr uint32_t kSizeInBytes = 25 * kSystemPointerSize; // Does early low-level initialization that does not depend on the // isolate being present. @@ -147,9 +146,6 @@ class ThreadLocalTop { // Address of the thread-local "thread in wasm" flag. Address thread_in_wasm_flag_address_; - - // Stack information. - ::heap::base::Stack stack_; }; } // namespace internal diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 51a90ddcab..b5722ab6ec 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -5851,9 +5851,7 @@ void Heap::SetStackStart(void* stack_start) { stack().SetStackStart(stack_start); } -::heap::base::Stack& Heap::stack() { - return isolate_->thread_local_top()->stack_; -} +::heap::base::Stack& Heap::stack() { return isolate_->stack(); } void Heap::RegisterExternallyReferencedObject(Address* location) { Object object = TracedHandles::Mark(location, TracedHandles::MarkMode::kAll);