Skip to content

Commit a3f258c

Browse files
committed
deps: cherry-pick 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 Note: I believe my employer, Meteor Development Group, has previously signed the CLA using the group email address google-contrib@meteor.com. 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 <yangguo@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#54902} Refs: v8/v8@a8f6869 PR-URL: #21983 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent fc1770b commit a3f258c

File tree

7 files changed

+156
-9
lines changed

7 files changed

+156
-9
lines changed

common.gypi

+1-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929

3030
# Reset this number to 0 on major V8 upgrades.
3131
# Increment by one for each non-official patch applied to deps/v8.
32-
'v8_embedder_string': '-node.7',
32+
'v8_embedder_string': '-node.8',
3333

3434
# Enable disassembler for `--print-code` v8 options
3535
'v8_enable_disassembler': 1,

deps/v8/AUTHORS

+2
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ Facebook, Inc. <*@fb.com>
3232
Facebook, Inc. <*@oculus.com>
3333
Vewd Software AS <*@vewd.com>
3434
Groupon <*@groupon.com>
35+
Meteor Development Group <*@meteor.com>
3536
Cloudflare, Inc. <*@cloudflare.com>
3637

3738
Aaron Bieber <deftly@gmail.com>
@@ -49,6 +50,7 @@ Andrei Kashcha <anvaka@gmail.com>
4950
Anna Henningsen <anna@addaleax.net>
5051
Bangfu Tao <bangfu.tao@samsung.com>
5152
Ben Coe <ben@npmjs.com>
53+
Ben Newman <ben@meteor.com>
5254
Ben Noordhuis <info@bnoordhuis.nl>
5355
Benjamin Tan <demoneaux@gmail.com>
5456
Bert Belder <bertbelder@gmail.com>

deps/v8/src/debug/debug.cc

+18-6
Original file line numberDiff line numberDiff line change
@@ -355,19 +355,31 @@ void Debug::ThreadInit() {
355355

356356

357357
char* Debug::ArchiveDebug(char* storage) {
358-
// Simply reset state. Don't archive anything.
359-
ThreadInit();
358+
MemCopy(storage, reinterpret_cast<char*>(&thread_local_),
359+
ArchiveSpacePerThread());
360360
return storage + ArchiveSpacePerThread();
361361
}
362362

363-
364363
char* Debug::RestoreDebug(char* storage) {
365-
// Simply reset state. Don't restore anything.
366-
ThreadInit();
364+
MemCopy(reinterpret_cast<char*>(&thread_local_), storage,
365+
ArchiveSpacePerThread());
366+
367+
// Enter the debugger.
368+
DebugScope debug_scope(this);
369+
370+
// Clear any one-shot breakpoints that may have been set by the other
371+
// thread, and reapply breakpoints for this thread.
372+
ClearOneShot();
373+
374+
if (thread_local_.last_step_action_ != StepNone) {
375+
// Reset the previous step action for this thread.
376+
PrepareStep(thread_local_.last_step_action_);
377+
}
378+
367379
return storage + ArchiveSpacePerThread();
368380
}
369381

370-
int Debug::ArchiveSpacePerThread() { return 0; }
382+
int Debug::ArchiveSpacePerThread() { return sizeof(ThreadLocal); }
371383

372384
void Debug::Iterate(RootVisitor* v) {
373385
v->VisitRootPointer(Root::kDebug, nullptr, &thread_local_.return_value_);

deps/v8/src/debug/debug.h

+1
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,7 @@ class Debug {
314314
static int ArchiveSpacePerThread();
315315
void FreeThreadResources() { }
316316
void Iterate(RootVisitor* v);
317+
void InitThread(const ExecutionAccess& lock) { ThreadInit(); }
317318

318319
bool CheckExecutionState() { return is_active() && break_id() != 0; }
319320

deps/v8/src/v8threads.cc

+6-2
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ void Locker::Initialize(v8::Isolate* isolate) {
4545
} else {
4646
internal::ExecutionAccess access(isolate_);
4747
isolate_->stack_guard()->ClearThread(access);
48-
isolate_->stack_guard()->InitThread(access);
48+
isolate_->thread_manager()->InitThread(access);
4949
}
5050
}
5151
DCHECK(isolate_->thread_manager()->IsLockedByCurrentThread());
@@ -95,6 +95,10 @@ Unlocker::~Unlocker() {
9595

9696
namespace internal {
9797

98+
void ThreadManager::InitThread(const ExecutionAccess& lock) {
99+
isolate_->stack_guard()->InitThread(lock);
100+
isolate_->debug()->InitThread(lock);
101+
}
98102

99103
bool ThreadManager::RestoreThread() {
100104
DCHECK(IsLockedByCurrentThread());
@@ -127,7 +131,7 @@ bool ThreadManager::RestoreThread() {
127131
isolate_->FindPerThreadDataForThisThread();
128132
if (per_thread == nullptr || per_thread->thread_state() == nullptr) {
129133
// This is a new thread.
130-
isolate_->stack_guard()->InitThread(access);
134+
InitThread(access);
131135
return false;
132136
}
133137
ThreadState* state = per_thread->thread_state();

deps/v8/src/v8threads.h

+1
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ class ThreadManager {
6767
void Lock();
6868
void Unlock();
6969

70+
void InitThread(const ExecutionAccess&);
7071
void ArchiveThread();
7172
bool RestoreThread();
7273
void FreeThreadResources();

deps/v8/test/cctest/test-debug.cc

+127
Original file line numberDiff line numberDiff line change
@@ -3717,6 +3717,133 @@ TEST(DebugBreakOffThreadTerminate) {
37173717
CHECK(try_catch.HasTerminated());
37183718
}
37193719

3720+
class ArchiveRestoreThread : public v8::base::Thread,
3721+
public v8::debug::DebugDelegate {
3722+
public:
3723+
ArchiveRestoreThread(v8::Isolate* isolate, int spawn_count)
3724+
: Thread(Options("ArchiveRestoreThread")),
3725+
isolate_(isolate),
3726+
debug_(reinterpret_cast<i::Isolate*>(isolate_)->debug()),
3727+
spawn_count_(spawn_count),
3728+
break_count_(0) {}
3729+
3730+
virtual void Run() {
3731+
v8::Locker locker(isolate_);
3732+
isolate_->Enter();
3733+
3734+
v8::HandleScope scope(isolate_);
3735+
v8::Local<v8::Context> context = v8::Context::New(isolate_);
3736+
v8::Context::Scope context_scope(context);
3737+
3738+
v8::Local<v8::Function> test = CompileFunction(isolate_,
3739+
"function test(n) {\n"
3740+
" debugger;\n"
3741+
" return n + 1;\n"
3742+
"}\n",
3743+
"test");
3744+
3745+
debug_->SetDebugDelegate(this);
3746+
v8::internal::DisableBreak enable_break(debug_, false);
3747+
3748+
v8::Local<v8::Value> args[1] = {v8::Integer::New(isolate_, spawn_count_)};
3749+
3750+
int result = test->Call(context, context->Global(), 1, args)
3751+
.ToLocalChecked()
3752+
->Int32Value(context)
3753+
.FromJust();
3754+
3755+
// Verify that test(spawn_count_) returned spawn_count_ + 1.
3756+
CHECK_EQ(spawn_count_ + 1, result);
3757+
3758+
isolate_->Exit();
3759+
}
3760+
3761+
void BreakProgramRequested(v8::Local<v8::Context> context,
3762+
const std::vector<v8::debug::BreakpointId>&) {
3763+
auto stack_traces = v8::debug::StackTraceIterator::Create(isolate_);
3764+
if (!stack_traces->Done()) {
3765+
v8::debug::Location location = stack_traces->GetSourceLocation();
3766+
3767+
i::PrintF("ArchiveRestoreThread #%d hit breakpoint at line %d\n",
3768+
spawn_count_, location.GetLineNumber());
3769+
3770+
switch (location.GetLineNumber()) {
3771+
case 1: // debugger;
3772+
CHECK_EQ(break_count_, 0);
3773+
3774+
// Attempt to stop on the next line after the first debugger
3775+
// statement. If debug->{Archive,Restore}Debug() improperly reset
3776+
// thread-local debug information, the debugger will fail to stop
3777+
// before the test function returns.
3778+
debug_->PrepareStep(StepNext);
3779+
3780+
// Spawning threads while handling the current breakpoint verifies
3781+
// that the parent thread correctly archived and restored the
3782+
// state necessary to stop on the next line. If not, then control
3783+
// will simply continue past the `return n + 1` statement.
3784+
MaybeSpawnChildThread();
3785+
3786+
break;
3787+
3788+
case 2: // return n + 1;
3789+
CHECK_EQ(break_count_, 1);
3790+
break;
3791+
3792+
default:
3793+
CHECK(false);
3794+
}
3795+
}
3796+
3797+
++break_count_;
3798+
}
3799+
3800+
void MaybeSpawnChildThread() {
3801+
if (spawn_count_ > 1) {
3802+
v8::Unlocker unlocker(isolate_);
3803+
3804+
// Spawn a thread that spawns a thread that spawns a thread (and so
3805+
// on) so that the ThreadManager is forced to archive and restore
3806+
// the current thread.
3807+
ArchiveRestoreThread child(isolate_, spawn_count_ - 1);
3808+
child.Start();
3809+
child.Join();
3810+
3811+
// The child thread sets itself as the debug delegate, so we need to
3812+
// usurp it after the child finishes, or else future breakpoints
3813+
// will be delegated to a destroyed ArchiveRestoreThread object.
3814+
debug_->SetDebugDelegate(this);
3815+
3816+
// This is the most important check in this test, since
3817+
// child.GetBreakCount() will return 1 if the debugger fails to stop
3818+
// on the `return n + 1` line after the grandchild thread returns.
3819+
CHECK_EQ(child.GetBreakCount(), 2);
3820+
}
3821+
}
3822+
3823+
int GetBreakCount() { return break_count_; }
3824+
3825+
private:
3826+
v8::Isolate* isolate_;
3827+
v8::internal::Debug* debug_;
3828+
const int spawn_count_;
3829+
int break_count_;
3830+
};
3831+
3832+
TEST(DebugArchiveRestore) {
3833+
v8::Isolate::CreateParams create_params;
3834+
create_params.array_buffer_allocator = CcTest::array_buffer_allocator();
3835+
v8::Isolate* isolate = v8::Isolate::New(create_params);
3836+
3837+
ArchiveRestoreThread thread(isolate, 5);
3838+
// Instead of calling thread.Start() and thread.Join() here, we call
3839+
// thread.Run() directly, to make sure we exercise archive/restore
3840+
// logic on the *current* thread as well as other threads.
3841+
thread.Run();
3842+
CHECK_EQ(thread.GetBreakCount(), 2);
3843+
3844+
isolate->Dispose();
3845+
}
3846+
37203847
class DebugEventExpectNoException : public v8::debug::DebugDelegate {
37213848
public:
37223849
void ExceptionThrown(v8::Local<v8::Context> paused_context,

0 commit comments

Comments
 (0)