[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
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4152476
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <nikolaos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85445}

src/execution/isolate.cc

@@ -3069,21 +3069,23 @@ void Isolate::AddSharedWasmMemory(Handle<WasmMemoryObject> memory_object) {
 void Isolate::RecordStackSwitchForScanning() {
   Object current = root(RootIndex::kActiveContinuation);
   DCHECK(!current.IsUndefined());
-  thread_local_top()->stack_.ClearStackSegments();
-  wasm::StackMemory* stack = Managed<wasm::StackMemory>::cast(
-                                 WasmContinuationObject::cast(current).stack())
-                                 .get()
-                                 .get();
+  stack().ClearStackSegments();
+  wasm::StackMemory* wasm_stack =
+      Managed<wasm::StackMemory>::cast(
+          WasmContinuationObject::cast(current).stack())
+          .get()
+          .get();
   current = WasmContinuationObject::cast(current).parent();
-  heap()->SetStackStart(reinterpret_cast<void*>(stack->base()));
+  heap()->SetStackStart(reinterpret_cast<void*>(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<wasm::StackMemory>::cast(cont.stack()).get().get();
-    thread_local_top()->stack_.AddStackSegment(
-        reinterpret_cast<const void*>(stack->base()),
-        reinterpret_cast<const void*>(stack->jmpbuf()->sp));
+    auto* wasm_stack =
+        Managed<wasm::StackMemory>::cast(cont.stack()).get().get();
+    stack().AddStackSegment(
+        reinterpret_cast<const void*>(wasm_stack->base()),
+        reinterpret_cast<const void*>(wasm_stack->jmpbuf()->sp));
     current = cont.parent();
   }
 }
@@ -3371,23 +3373,13 @@ void Isolate::Delete(Isolate* isolate) {
   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
@@ -4618,6 +4610,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);

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"
@@ -2029,6 +2030,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
@@ -2527,6 +2530,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

src/execution/thread-local-top.cc

@@ -37,7 +37,6 @@ 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) {
@@ -45,12 +44,8 @@ void ThreadLocalTop::Initialize(Isolate* isolate) {
   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<Address>(
       trap_handler::GetThreadInWasmThreadLocalAddress());
-#else
-  stack_.SetStackStart(base::Stack::GetStackStart(), false);
 #endif  // V8_ENABLE_WEBASSEMBLY
 #ifdef USE_SIMULATOR
   simulator_ = Simulator::current(isolate);

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

src/heap/heap.cc

@@ -5813,9 +5813,7 @@ void Heap::SetStackStart(void* stack_start) {
 #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

test/cctest/test-api.cc

@@ -13043,12 +13043,13 @@ bool ApiTestFuzzer::NextThread() {
 
 
 void ApiTestFuzzer::Run() {
-  // When it is our turn...
+  // Wait until it is our turn.
   gate_.Wait();
   {
-    // ... get the V8 lock
+    // Get the V8 lock.
     v8::Locker locker(CcTest::isolate());
-    // ... and start running the test.
+    // Start running the test, which will enter the isolate and exit it when it
+    // finishes.
     CallTest();
   }
   // This test finished.
@@ -13089,11 +13090,18 @@ static void CallTestNumber(int test_number) {
 
 
 void ApiTestFuzzer::RunAllTests() {
+  // This method is called when running each THREADING_TEST, which is an
+  // initialized test and has entered the isolate at this point. We need to exit
+  // the isolate, so that the fuzzer threads can enter it in turn, while running
+  // their tests.
+  CcTest::isolate()->Exit();
   // Set off the first test.
   current_ = -1;
   NextThread();
   // Wait till they are all done.
   all_tests_done_.Wait();
+  // We enter the isolate again, to prepare for teardown.
+  CcTest::isolate()->Enter();
 }
 
 
@@ -13111,10 +13119,16 @@ int ApiTestFuzzer::GetNextTestNumber() {
 void ApiTestFuzzer::ContextSwitch() {
   // If the new thread is the same as the current thread there is nothing to do.
   if (NextThread()) {
-    // Now it can start.
-    v8::Unlocker unlocker(CcTest::isolate());
-    // Wait till someone starts us again.
-    gate_.Wait();
+    // Exit the isolate from this thread.
+    CcTest::i_isolate()->Exit();
+    {
+      // Now the new thread can start.
+      v8::Unlocker unlocker(CcTest::isolate());
+      // Wait till someone starts us again.
+      gate_.Wait();
+    }
+    // Enter the isolate from this thread again.
+    CcTest::i_isolate()->Enter();
     // And we're off.
   }
 }

test/cctest/test-cpu-profiler.cc

@@ -3457,8 +3457,12 @@ TEST(MultipleThreadsSingleIsolate) {
       env, "YieldIsolate", [](const v8::FunctionCallbackInfo<v8::Value>& info) {
         v8::Isolate* isolate = info.GetIsolate();
         if (!info[0]->IsTrue()) return;
-        v8::Unlocker unlocker(isolate);
-        v8::base::OS::Sleep(v8::base::TimeDelta::FromMilliseconds(1));
+        isolate->Exit();
+        {
+          v8::Unlocker unlocker(isolate);
+          v8::base::OS::Sleep(v8::base::TimeDelta::FromMilliseconds(1));
+        }
+        isolate->Enter();
       });
 
   CompileRun(varying_frame_size_script);
@@ -3470,11 +3474,13 @@ TEST(MultipleThreadsSingleIsolate) {
 
   // For good measure, profile on our own thread
   UnlockingThread::Profile(env, 0);
+  isolate->Exit();
   {
     v8::Unlocker unlocker(isolate);
     thread1.Join();
     thread2.Join();
   }
+  isolate->Enter();
 }
 
 // Tests that StopProfiling doesn't wait for the next sample tick in order to

test/cctest/test-debug.cc

@@ -4211,6 +4211,12 @@ class ArchiveRestoreThread : public v8::base::Thread,
       // child.GetBreakCount() will return 1 if the debugger fails to stop
       // on the `next()` line after the grandchild thread returns.
       CHECK_EQ(child.GetBreakCount(), 5);
+
+      // This test on purpose unlocks the isolate without exiting and
+      // re-entering. It must however update the stack start, which would have
+      // been done automatically if the isolate was properly re-entered.
+      reinterpret_cast<i::Isolate*>(isolate_)->heap()->SetStackStart(
+          v8::base::Stack::GetStackStart());
     }
   }
 

test/cctest/test-lockers.cc

@@ -871,7 +871,9 @@ TEST(LockUnlockLockDefaultIsolateMultithreaded) {
       threads.push_back(new LockUnlockLockDefaultIsolateThread(context));
     }
   }
+  CcTest::isolate()->Exit();
   StartJoinAndDeleteThreads(threads);
+  CcTest::isolate()->Enter();
 }