From a28a51d40b8cf3f1feeb5a045842ba81181e1502 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Sat, 4 Aug 2018 11:28:29 -0400 Subject: [PATCH 1/4] deps: backport a8f6869 from upstream V8 Original commit message: [debug] Fully implement Debug::ArchiveDebug and Debug::RestoreDebug. I have a project that embeds V8 and uses a single `Isolate` from multiple threads. The program runs just fine, but sometimes the inspector doesn't stop on the correct line after stepping over a statement that switches threads behind the scenes, even though the original thread is restored by the time the next statement is executed. After some digging, I discovered that the `Debug::ArchiveDebug` and `Debug::RestoreDebug` methods, which should be responsible for saving/restoring this `ThreadLocal` information when switching threads, currently don't do anything. This commit implements those methods using MemCopy, in the style of other Archive/Restore methods in the V8 codebase. Related: https://groups.google.com/forum/#!topic/v8-users/_Qf2rwljRk8 R=yangguo@chromium.org,jgruber@chromium.org CC=info@bnoordhuis.nl Bug: v8:7230 Change-Id: Id517c873eb81cd53f7216c7efd441b956cf7f943 Reviewed-on: https://chromium-review.googlesource.com/833260 Commit-Queue: Yang Guo Reviewed-by: Yang Guo Cr-Commit-Position: refs/heads/master@{#54902} --- deps/v8/AUTHORS | 2 + deps/v8/src/debug/debug.cc | 24 ++++-- deps/v8/src/debug/debug.h | 1 + deps/v8/src/v8threads.cc | 8 +- deps/v8/src/v8threads.h | 1 + deps/v8/test/cctest/test-debug.cc | 128 ++++++++++++++++++++++++++++++ 6 files changed, 156 insertions(+), 8 deletions(-) diff --git a/deps/v8/AUTHORS b/deps/v8/AUTHORS index e920bbf42bd3d8..afad3a50414858 100644 --- a/deps/v8/AUTHORS +++ b/deps/v8/AUTHORS @@ -32,6 +32,7 @@ Facebook, Inc. <*@fb.com> Facebook, Inc. <*@oculus.com> Vewd Software AS <*@vewd.com> Groupon <*@groupon.com> +Meteor Development Group <*@meteor.com> Cloudflare, Inc. <*@cloudflare.com> Aaron Bieber @@ -49,6 +50,7 @@ Andrei Kashcha Anna Henningsen Bangfu Tao Ben Coe +Ben Newman Ben Noordhuis Benjamin Tan Bert Belder diff --git a/deps/v8/src/debug/debug.cc b/deps/v8/src/debug/debug.cc index b5dbbf8412e57d..6e72a7e09fa646 100644 --- a/deps/v8/src/debug/debug.cc +++ b/deps/v8/src/debug/debug.cc @@ -355,19 +355,31 @@ void Debug::ThreadInit() { char* Debug::ArchiveDebug(char* storage) { - // Simply reset state. Don't archive anything. - ThreadInit(); + MemCopy(storage, reinterpret_cast(&thread_local_), + ArchiveSpacePerThread()); return storage + ArchiveSpacePerThread(); } - char* Debug::RestoreDebug(char* storage) { - // Simply reset state. Don't restore anything. - ThreadInit(); + MemCopy(reinterpret_cast(&thread_local_), storage, + ArchiveSpacePerThread()); + + // Enter the debugger. + DebugScope debug_scope(this); + + // Clear any one-shot breakpoints that may have been set by the other + // thread, and reapply breakpoints for this thread. + ClearOneShot(); + + if (thread_local_.last_step_action_ != StepNone) { + // Reset the previous step action for this thread. + PrepareStep(thread_local_.last_step_action_); + } + return storage + ArchiveSpacePerThread(); } -int Debug::ArchiveSpacePerThread() { return 0; } +int Debug::ArchiveSpacePerThread() { return sizeof(ThreadLocal); } void Debug::Iterate(RootVisitor* v) { v->VisitRootPointer(Root::kDebug, nullptr, &thread_local_.return_value_); diff --git a/deps/v8/src/debug/debug.h b/deps/v8/src/debug/debug.h index 2c83d9855cf477..17bdfc03680a07 100644 --- a/deps/v8/src/debug/debug.h +++ b/deps/v8/src/debug/debug.h @@ -327,6 +327,7 @@ class Debug { static int ArchiveSpacePerThread(); void FreeThreadResources() { } void Iterate(RootVisitor* v); + void InitThread(const ExecutionAccess& lock) { ThreadInit(); } bool CheckExecutionState(int id) { return CheckExecutionState() && break_id() == id; diff --git a/deps/v8/src/v8threads.cc b/deps/v8/src/v8threads.cc index db927010ef1749..0fb333c1f37572 100644 --- a/deps/v8/src/v8threads.cc +++ b/deps/v8/src/v8threads.cc @@ -45,7 +45,7 @@ void Locker::Initialize(v8::Isolate* isolate) { } else { internal::ExecutionAccess access(isolate_); isolate_->stack_guard()->ClearThread(access); - isolate_->stack_guard()->InitThread(access); + isolate_->thread_manager()->InitThread(access); } } DCHECK(isolate_->thread_manager()->IsLockedByCurrentThread()); @@ -95,6 +95,10 @@ Unlocker::~Unlocker() { namespace internal { +void ThreadManager::InitThread(const ExecutionAccess& lock) { + isolate_->stack_guard()->InitThread(lock); + isolate_->debug()->InitThread(lock); +} bool ThreadManager::RestoreThread() { DCHECK(IsLockedByCurrentThread()); @@ -127,7 +131,7 @@ bool ThreadManager::RestoreThread() { isolate_->FindPerThreadDataForThisThread(); if (per_thread == nullptr || per_thread->thread_state() == nullptr) { // This is a new thread. - isolate_->stack_guard()->InitThread(access); + InitThread(access); return false; } ThreadState* state = per_thread->thread_state(); diff --git a/deps/v8/src/v8threads.h b/deps/v8/src/v8threads.h index bb87afea7d8497..7fde0c9ec494e7 100644 --- a/deps/v8/src/v8threads.h +++ b/deps/v8/src/v8threads.h @@ -67,6 +67,7 @@ class ThreadManager { void Lock(); void Unlock(); + void InitThread(const ExecutionAccess&); void ArchiveThread(); bool RestoreThread(); void FreeThreadResources(); diff --git a/deps/v8/test/cctest/test-debug.cc b/deps/v8/test/cctest/test-debug.cc index f3daf05a9ee53f..b39196478d69ac 100644 --- a/deps/v8/test/cctest/test-debug.cc +++ b/deps/v8/test/cctest/test-debug.cc @@ -6221,6 +6221,134 @@ TEST(DebugBreakOffThreadTerminate) { } +class ArchiveRestoreThread : public v8::base::Thread, + public v8::debug::DebugDelegate { + public: + ArchiveRestoreThread(v8::Isolate* isolate, int spawn_count) + : Thread(Options("ArchiveRestoreThread")), + isolate_(isolate), + debug_(reinterpret_cast(isolate_)->debug()), + spawn_count_(spawn_count), + break_count_(0) {} + + virtual void Run() { + v8::Locker locker(isolate_); + isolate_->Enter(); + + v8::HandleScope scope(isolate_); + v8::Local context = v8::Context::New(isolate_); + v8::Context::Scope context_scope(context); + + v8::Local test = CompileFunction(isolate_, + "function test(n) {\n" + " debugger;\n" + " return n + 1;\n" + "}\n", + "test"); + + debug_->SetDebugDelegate(this); + v8::internal::DisableBreak enable_break(debug_, false); + + v8::Local args[1] = {v8::Integer::New(isolate_, spawn_count_)}; + + int result = test->Call(context, context->Global(), 1, args) + .ToLocalChecked() + ->Int32Value(context) + .FromJust(); + + // Verify that test(spawn_count_) returned spawn_count_ + 1. + CHECK_EQ(spawn_count_ + 1, result); + + isolate_->Exit(); + } + + void BreakProgramRequested(v8::Local context, + const std::vector&) { + auto stack_traces = v8::debug::StackTraceIterator::Create(isolate_); + if (!stack_traces->Done()) { + v8::debug::Location location = stack_traces->GetSourceLocation(); + + i::PrintF("ArchiveRestoreThread #%d hit breakpoint at line %d\n", + spawn_count_, location.GetLineNumber()); + + switch (location.GetLineNumber()) { + case 1: // debugger; + CHECK_EQ(break_count_, 0); + + // Attempt to stop on the next line after the first debugger + // statement. If debug->{Archive,Restore}Debug() improperly reset + // thread-local debug information, the debugger will fail to stop + // before the test function returns. + debug_->PrepareStep(StepNext); + + // Spawning threads while handling the current breakpoint verifies + // that the parent thread correctly archived and restored the + // state necessary to stop on the next line. If not, then control + // will simply continue past the `return n + 1` statement. + MaybeSpawnChildThread(); + + break; + + case 2: // return n + 1; + CHECK_EQ(break_count_, 1); + break; + + default: + CHECK(false); + } + } + + ++break_count_; + } + + void MaybeSpawnChildThread() { + if (spawn_count_ > 1) { + v8::Unlocker unlocker(isolate_); + + // Spawn a thread that spawns a thread that spawns a thread (and so + // on) so that the ThreadManager is forced to archive and restore + // the current thread. + ArchiveRestoreThread child(isolate_, spawn_count_ - 1); + child.Start(); + child.Join(); + + // The child thread sets itself as the debug delegate, so we need to + // usurp it after the child finishes, or else future breakpoints + // will be delegated to a destroyed ArchiveRestoreThread object. + debug_->SetDebugDelegate(this); + + // This is the most important check in this test, since + // child.GetBreakCount() will return 1 if the debugger fails to stop + // on the `return n + 1` line after the grandchild thread returns. + CHECK_EQ(child.GetBreakCount(), 2); + } + } + + int GetBreakCount() { return break_count_; } + + private: + v8::Isolate* isolate_; + v8::internal::Debug* debug_; + const int spawn_count_; + int break_count_; +}; + +TEST(DebugArchiveRestore) { + v8::Isolate::CreateParams create_params; + create_params.array_buffer_allocator = CcTest::array_buffer_allocator(); + v8::Isolate* isolate = v8::Isolate::New(create_params); + + ArchiveRestoreThread thread(isolate, 5); + // Instead of calling thread.Start() and thread.Join() here, we call + // thread.Run() directly, to make sure we exercise archive/restore + // logic on the *current* thread as well as other threads. + thread.Run(); + CHECK_EQ(thread.GetBreakCount(), 2); + + isolate->Dispose(); +} + + static void DebugEventExpectNoException( const v8::Debug::EventDetails& event_details) { v8::DebugEvent event = event_details.GetEvent(); From 3a2d17522c4888d31aabfefc20df452798095799 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 7 Aug 2018 20:15:25 -0400 Subject: [PATCH 2/4] Fix build errors by matching older V8 APIs used by Node. It looks like SetDebugDelegate(debug::DebugDelegate* delegate, bool pass_ownership) was simplified to just SetDebugDelegate(debug::DebugDelegate* delegate) in https://github.com/v8/v8/commit/37dcd837dbafa7f1175be5f01f0def013437c7e7, but the extra `pass_ownership` parameter is still there in the current version of `node/deps/v8`. I should be able to fix those tests by passing `false` for `pass_ownership`. Also, the `DebugDelegate::BreakProgramRequested` method lost a parameter in https://github.com/v8/v8/commit/e404670696b4c49d7f8adcdb075b98acab9967dd, but it's not a parameter I was using in my test, so there shouldn't be any harm in adding the `exec_state` parameter back to `BreakProgramRequested` (and continuing to ignore it). I'm not sure why `ninja -C out/Debug` didn't catch these errors before. Maybe they only show up in release builds? --- deps/v8/test/cctest/test-debug.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/deps/v8/test/cctest/test-debug.cc b/deps/v8/test/cctest/test-debug.cc index b39196478d69ac..92abcae16e817b 100644 --- a/deps/v8/test/cctest/test-debug.cc +++ b/deps/v8/test/cctest/test-debug.cc @@ -6246,7 +6246,7 @@ class ArchiveRestoreThread : public v8::base::Thread, "}\n", "test"); - debug_->SetDebugDelegate(this); + debug_->SetDebugDelegate(this, false); v8::internal::DisableBreak enable_break(debug_, false); v8::Local args[1] = {v8::Integer::New(isolate_, spawn_count_)}; @@ -6263,6 +6263,7 @@ class ArchiveRestoreThread : public v8::base::Thread, } void BreakProgramRequested(v8::Local context, + v8::Local exec_state, const std::vector&) { auto stack_traces = v8::debug::StackTraceIterator::Create(isolate_); if (!stack_traces->Done()) { @@ -6315,7 +6316,7 @@ class ArchiveRestoreThread : public v8::base::Thread, // The child thread sets itself as the debug delegate, so we need to // usurp it after the child finishes, or else future breakpoints // will be delegated to a destroyed ArchiveRestoreThread object. - debug_->SetDebugDelegate(this); + debug_->SetDebugDelegate(this, false); // This is the most important check in this test, since // child.GetBreakCount() will return 1 if the debugger fails to stop From 023cea7352c23c618445d08f662f398a5ec44f9c Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 14 Aug 2018 17:17:39 -0400 Subject: [PATCH 3/4] Skip restoring debug state unless thread previously in DebugScope. A simpler version of the changes I proposed upstream in this V8 change request: https://chromium-review.googlesource.com/c/v8/v8/+/1168449 In this version, Debug::RestoreDebug never attempts to enter a new DebugScope, but merely reuses the previous one, if we're returning to a thread that was previously in a DebugScope. If the thread was not previously in a DebugScope, I believe it does not need to have any debugging state restored with ClearOneShot and PrepareStep. The tests from https://chromium-review.googlesource.com/c/v8/v8/+/833260 still pass, and the failing V8-CI tests are now passing locally for me: https://ci.nodejs.org/job/node-test-commit-v8-linux/1601/ --- deps/v8/src/debug/debug.cc | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/deps/v8/src/debug/debug.cc b/deps/v8/src/debug/debug.cc index 6e72a7e09fa646..d25c2598467e6c 100644 --- a/deps/v8/src/debug/debug.cc +++ b/deps/v8/src/debug/debug.cc @@ -364,16 +364,21 @@ char* Debug::RestoreDebug(char* storage) { MemCopy(reinterpret_cast(&thread_local_), storage, ArchiveSpacePerThread()); - // Enter the debugger. - DebugScope debug_scope(this); - - // Clear any one-shot breakpoints that may have been set by the other - // thread, and reapply breakpoints for this thread. - ClearOneShot(); + if (in_debug_scope()) { + // If this thread was in a DebugScope when we archived it, restore the + // previous debugging state now. Note that in_debug_scope() returns + // true when thread_local_.current_debug_scope_ (restored by MemCopy + // above) is non-null. + + // Clear any one-shot breakpoints that may have been set by the other + // thread, and reapply breakpoints for this thread. + HandleScope scope(isolate_); + ClearOneShot(); - if (thread_local_.last_step_action_ != StepNone) { - // Reset the previous step action for this thread. - PrepareStep(thread_local_.last_step_action_); + if (thread_local_.last_step_action_ != StepNone) { + // Reset the previous step action for this thread. + PrepareStep(thread_local_.last_step_action_); + } } return storage + ArchiveSpacePerThread(); From 8d66ed7286f554c7897cd034c941c44d071ace42 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 20 Aug 2018 10:14:05 -0400 Subject: [PATCH 4/4] Bump v8_embedder_string to '-node.17'. --- common.gypi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common.gypi b/common.gypi index a085594786e46f..c2f4b1778a12d0 100644 --- a/common.gypi +++ b/common.gypi @@ -29,7 +29,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.23', + 'v8_embedder_string': '-node.24', # Enable disassembler for `--print-code` v8 options 'v8_enable_disassembler': 1,