Clean up thread initialization

This CL makes ThreadManager::InitThread *the* place that's responsible
for initializing metadata for a new thread, and ensures that all new
threads actually go through there. This was previously not the case,
and e.g. test-lockers/LockerUnlocker exposed a case where some threads
were trying to use another thread's simulator instance because the
ThreadLocalTop on the Isolate was in inconsistent state.

Change-Id: I302c643f420457f6ba73897fd45eb87969e1331c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1781688
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Auto-Submit: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63527}

src/execution/isolate.cc

@@ -2649,21 +2649,12 @@ Handle<Context> Isolate::GetIncumbentContext() {
 char* Isolate::ArchiveThread(char* to) {
   MemCopy(to, reinterpret_cast<char*>(thread_local_top()),
           sizeof(ThreadLocalTop));
-  InitializeThreadLocal();
-  clear_pending_exception();
-  clear_pending_message();
-  clear_scheduled_exception();
   return to + sizeof(ThreadLocalTop);
 }
 
 char* Isolate::RestoreThread(char* from) {
   MemCopy(reinterpret_cast<char*>(thread_local_top()), from,
           sizeof(ThreadLocalTop));
-// This might be just paranoia, but it seems to be needed in case a
-// thread_local_top_ is restored on a separate OS thread.
-#ifdef USE_SIMULATOR
-  thread_local_top()->simulator_ = Simulator::current(this);
-#endif
   DCHECK(context().is_null() || context().IsContext());
   return from + sizeof(ThreadLocalTop);
 }
@@ -3148,7 +3139,12 @@ Isolate::~Isolate() {
   default_microtask_queue_ = nullptr;
 }
 
-void Isolate::InitializeThreadLocal() { thread_local_top()->Initialize(this); }
+void Isolate::InitializeThreadLocal() {
+  thread_local_top()->Initialize(this);
+  clear_pending_exception();
+  clear_pending_message();
+  clear_scheduled_exception();
+}
 
 void Isolate::SetTerminationOnExternalTryCatch() {
   if (try_catch_handler() == nullptr) return;

src/execution/isolate.h

@@ -960,6 +960,7 @@ class Isolate final : private HiddenFactory {
   void set_deoptimizer_lazy_throw(bool value) {
     deoptimizer_lazy_throw_ = value;
   }
+  void InitializeThreadLocal();
   ThreadLocalTop* thread_local_top() {
     return &isolate_data_.thread_local_top_;
   }
@@ -1633,8 +1634,6 @@ class Isolate final : private HiddenFactory {
   static void SetIsolateThreadLocals(Isolate* isolate,
                                      PerIsolateThreadData* data);
 
-  void InitializeThreadLocal();
-
   void MarkCompactPrologue(bool is_compacting,
                            ThreadLocalTop* archived_thread_data);
   void MarkCompactEpilogue(bool is_compacting,

src/execution/stack-guard.cc

@@ -194,35 +194,21 @@ void StackGuard::FreeThreadResources() {
   per_thread->set_stack_limit(thread_local_.real_climit_);
 }
 
-void StackGuard::ThreadLocal::Clear() {
-  real_jslimit_ = kIllegalLimit;
-  set_jslimit(kIllegalLimit);
-  real_climit_ = kIllegalLimit;
-  set_climit(kIllegalLimit);
+void StackGuard::ThreadLocal::Initialize(Isolate* isolate,
+                                         const ExecutionAccess& lock) {
+  const uintptr_t kLimitSize = FLAG_stack_size * KB;
+  DCHECK_GT(GetCurrentStackPosition(), kLimitSize);
+  uintptr_t limit = GetCurrentStackPosition() - kLimitSize;
+  real_jslimit_ = SimulatorStack::JsLimitFromCLimit(isolate, limit);
+  set_jslimit(SimulatorStack::JsLimitFromCLimit(isolate, limit));
+  real_climit_ = limit;
+  set_climit(limit);
   interrupt_scopes_ = nullptr;
   interrupt_flags_ = 0;
 }
 
-void StackGuard::ThreadLocal::Initialize(Isolate* isolate) {
-  if (real_climit_ == kIllegalLimit) {
-    const uintptr_t kLimitSize = FLAG_stack_size * KB;
-    DCHECK_GT(GetCurrentStackPosition(), kLimitSize);
-    uintptr_t limit = GetCurrentStackPosition() - kLimitSize;
-    real_jslimit_ = SimulatorStack::JsLimitFromCLimit(isolate, limit);
-    set_jslimit(SimulatorStack::JsLimitFromCLimit(isolate, limit));
-    real_climit_ = limit;
-    set_climit(limit);
-  }
-  interrupt_scopes_ = nullptr;
-  interrupt_flags_ = 0;
-}
-
-void StackGuard::ClearThread(const ExecutionAccess& lock) {
-  thread_local_.Clear();
-}
-
 void StackGuard::InitThread(const ExecutionAccess& lock) {
-  thread_local_.Initialize(isolate_);
+  thread_local_.Initialize(isolate_, lock);
   Isolate::PerIsolateThreadData* per_thread =
       isolate_->FindOrAllocatePerThreadDataForThisThread();
   uintptr_t stored_limit = per_thread->stack_limit();

src/execution/stack-guard.h

@@ -38,12 +38,8 @@ class V8_EXPORT_PRIVATE StackGuard final {
   char* RestoreStackGuard(char* from);
   static int ArchiveSpacePerThread() { return sizeof(ThreadLocal); }
   void FreeThreadResources();
-  // Sets up the default stack guard for this thread if it has not
-  // already been set up.
+  // Sets up the default stack guard for this thread.
   void InitThread(const ExecutionAccess& lock);
-  // Clears the stack guard for this thread so it does not look as if
-  // it has been set up.
-  void ClearThread(const ExecutionAccess& lock);
 
 #define INTERRUPT_LIST(V)                                         \
   V(TERMINATE_EXECUTION, TerminateExecution, 0)                   \
@@ -127,12 +123,9 @@ class V8_EXPORT_PRIVATE StackGuard final {
 
   class ThreadLocal final {
    public:
-    ThreadLocal() { Clear(); }
-    // You should hold the ExecutionAccess lock when you call Initialize or
-    // Clear.
-    void Clear();
+    ThreadLocal() {}
 
-    void Initialize(Isolate* isolate);
+    void Initialize(Isolate* isolate, const ExecutionAccess& lock);
 
     // The stack limit is split into a JavaScript and a C++ stack limit. These
     // two are the same except when running on a simulator where the C++ and
@@ -143,13 +136,16 @@ class V8_EXPORT_PRIVATE StackGuard final {
     // break or preemption) in which case it is lowered to make stack checks
     // fail. Both the generated code and the runtime system check against the
     // one without the real_ prefix.
-    uintptr_t real_jslimit_;  // Actual JavaScript stack limit set for the VM.
-    uintptr_t real_climit_;   // Actual C++ stack limit set for the VM.
+
+    // Actual JavaScript stack limit set for the VM.
+    uintptr_t real_jslimit_ = kIllegalLimit;
+    // Actual C++ stack limit set for the VM.
+    uintptr_t real_climit_ = kIllegalLimit;
 
     // jslimit_ and climit_ can be read without any lock.
     // Writing requires the ExecutionAccess lock.
-    base::AtomicWord jslimit_;
-    base::AtomicWord climit_;
+    base::AtomicWord jslimit_ = kIllegalLimit;
+    base::AtomicWord climit_ = kIllegalLimit;
 
     uintptr_t jslimit() {
       return bit_cast<uintptr_t>(base::Relaxed_Load(&jslimit_));
@@ -166,8 +162,8 @@ class V8_EXPORT_PRIVATE StackGuard final {
                                  static_cast<base::AtomicWord>(limit));
     }
 
-    InterruptsScope* interrupt_scopes_;
-    intptr_t interrupt_flags_;
+    InterruptsScope* interrupt_scopes_ = nullptr;
+    intptr_t interrupt_flags_ = 0;
   };
 
   // TODO(isolates): Technically this could be calculated directly from a

src/execution/v8threads.cc

@@ -40,10 +40,6 @@ void Locker::Initialize(v8::Isolate* isolate) {
     // get the saved state for this thread and restore it.
     if (isolate_->thread_manager()->RestoreThread()) {
       top_level_ = false;
-    } else {
-      internal::ExecutionAccess access(isolate_);
-      isolate_->stack_guard()->ClearThread(access);
-      isolate_->thread_manager()->InitThread(access);
     }
   }
   DCHECK(isolate_->thread_manager()->IsLockedByCurrentThread());
@@ -88,6 +84,7 @@ Unlocker::~Unlocker() {
 namespace internal {
 
 void ThreadManager::InitThread(const ExecutionAccess& lock) {
+  isolate_->InitializeThreadLocal();
   isolate_->stack_guard()->InitThread(lock);
   isolate_->debug()->InitThread(lock);
 }