From 916641c08de44e0a915df223d55781c6c912717c Mon Sep 17 00:00:00 2001 From: Nikolaos Papaspyrou Date: Thu, 19 Jan 2023 13:43:10 +0100 Subject: [PATCH] [heap] Move the Stack object from ThreadLocalTop to Isolate 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: I026a35af3bc6999a09b21f277756d4454c086343 (stripped tests) --- diff --git a/src/execution/isolate.cc b/src/execution/isolate.cc index 31536a1..22b8492 100644 --- a/v8/src/execution/isolate.cc +++ b/v8/src/execution/isolate.cc @@ -3069,21 +3069,23 @@ 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(); - heap()->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(); } } @@ -3371,23 +3373,13 @@ Isolate* saved_isolate = isolate->TryGetCurrent(); SetIsolateThreadLocals(isolate, nullptr); isolate->set_thread_id(ThreadId::Current()); - if (saved_isolate) { - isolate->thread_local_top()->stack_ = - std::move(saved_isolate->thread_local_top()->stack_); - } else { - isolate->heap()->SetStackStart(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 @@ -4652,6 +4644,10 @@ 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 afc3d0b..415e476 100644 --- a/v8/src/execution/isolate.h +++ b/v8/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" @@ -2028,6 +2029,8 @@ 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 @@ -2526,6 +2529,9 @@ // 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 c115ae0..05cc20b 100644 --- a/v8/src/execution/thread-local-top.cc +++ b/v8/src/execution/thread-local-top.cc @@ -37,7 +37,6 @@ current_embedder_state_ = nullptr; failed_access_check_callback_ = nullptr; thread_in_wasm_flag_address_ = kNullAddress; - stack_ = ::heap::base::Stack(); } void ThreadLocalTop::Initialize(Isolate* isolate) { @@ -45,12 +44,8 @@ isolate_ = isolate; thread_id_ = ThreadId::Current(); #if V8_ENABLE_WEBASSEMBLY - stack_.SetStackStart(base::Stack::GetStackStart(), - v8_flags.experimental_wasm_stack_switching); thread_in_wasm_flag_address_ = reinterpret_cast
( trap_handler::GetThreadInWasmThreadLocalAddress()); -#else - stack_.SetStackStart(base::Stack::GetStackStart(), false); #endif // V8_ENABLE_WEBASSEMBLY #ifdef USE_SIMULATOR simulator_ = Simulator::current(isolate); diff --git a/src/execution/thread-local-top.h b/src/execution/thread-local-top.h index 43fec0a..989c817 100644 --- a/v8/src/execution/thread-local-top.h +++ b/v8/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 @@ // 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 @@ // 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 f4b7da0..6efd486 100644 --- a/v8/src/heap/heap.cc +++ b/v8/src/heap/heap.cc @@ -5830,9 +5830,7 @@ #endif // V8_ENABLE_WEBASSEMBLY } -::heap::base::Stack& Heap::stack() { - return isolate_->thread_local_top()->stack_; -} +::heap::base::Stack& Heap::stack() { return isolate_->stack(); } void Heap::StartTearDown() { // Finish any ongoing sweeping to avoid stray background tasks still accessing