Skip to content

Commit ce65d84

Browse files
Ben NewmanMylesBorins
Ben Newman
authored andcommitted
deps: backport a8f6869 from upstream V8
The upstream V8 commit a8f68691 was originally cherry-picked onto nodejs/node master as commit bb35752, then backported to v10.x-staging and released in Node.js v10.10.0 as 5e9ed6d. This commit cherry-picks that commit back to the v8.x-staging branch. Additionally this commit supports optional boolean argument to DisableBreak constructor. This allows a stack-allocated DisableBreak object to re-enable breakpoints temporarily, rather than always disabling them. This functionality anticipates an upstream change that will be introduced in V8 6.7.176: v8/v8@cc9736a 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 [email protected],[email protected] [email protected] Bug: v8:7230 Change-Id: Id517c873eb81cd53f7216c7efd441b956cf7f943 Reviewed-on: https://chromium-review.googlesource.com/833260 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#54902} PR-URL: #22714 Refs: v8/v8@a8f6869 Refs: #22122 Refs: bb35752 Refs: 5e9ed6d Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Yang Guo <[email protected]>
1 parent 7ab253f commit ce65d84

File tree

7 files changed

+166
-11
lines changed

7 files changed

+166
-11
lines changed

deps/v8/AUTHORS

+2
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ Yandex LLC <*@yandex-team.ru>
3030
StrongLoop, Inc. <*@strongloop.com>
3131
Facebook, Inc. <*@fb.com>
3232
Facebook, Inc. <*@oculus.com>
33+
Meteor Development Group <*@meteor.com>
3334

3435
Aaron Bieber <[email protected]>
3536
Abdulla Kamar <[email protected]>
@@ -44,6 +45,7 @@ Andrew Paprocki <[email protected]>
4445
Andrei Kashcha <[email protected]>
4546
Anna Henningsen <[email protected]>
4647
Bangfu Tao <[email protected]>
48+
Ben Newman <[email protected]>
4749
Ben Noordhuis <[email protected]>
4850
Benjamin Tan <[email protected]>
4951
Bert Belder <[email protected]>

deps/v8/include/v8-version.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
#define V8_MAJOR_VERSION 6
1212
#define V8_MINOR_VERSION 2
1313
#define V8_BUILD_NUMBER 414
14-
#define V8_PATCH_LEVEL 69
14+
#define V8_PATCH_LEVEL 70
1515

1616
// Use 1 for candidates and 0 otherwise.
1717
// (Boolean macro values are not supported by all preprocessors.)

deps/v8/src/debug/debug.cc

+23-6
Original file line numberDiff line numberDiff line change
@@ -264,19 +264,36 @@ void Debug::ThreadInit() {
264264

265265

266266
char* Debug::ArchiveDebug(char* storage) {
267-
// Simply reset state. Don't archive anything.
268-
ThreadInit();
267+
MemCopy(storage, reinterpret_cast<char*>(&thread_local_),
268+
ArchiveSpacePerThread());
269269
return storage + ArchiveSpacePerThread();
270270
}
271271

272-
273272
char* Debug::RestoreDebug(char* storage) {
274-
// Simply reset state. Don't restore anything.
275-
ThreadInit();
273+
MemCopy(reinterpret_cast<char*>(&thread_local_), storage,
274+
ArchiveSpacePerThread());
275+
276+
if (in_debug_scope()) {
277+
// If this thread was in a DebugScope when we archived it, restore the
278+
// previous debugging state now. Note that in_debug_scope() returns
279+
// true when thread_local_.current_debug_scope_ (restored by MemCopy
280+
// above) is non-null.
281+
282+
// Clear any one-shot breakpoints that may have been set by the other
283+
// thread, and reapply breakpoints for this thread.
284+
HandleScope scope(isolate_);
285+
ClearOneShot();
286+
287+
if (thread_local_.last_step_action_ != StepNone) {
288+
// Reset the previous step action for this thread.
289+
PrepareStep(thread_local_.last_step_action_);
290+
}
291+
}
292+
276293
return storage + ArchiveSpacePerThread();
277294
}
278295

279-
int Debug::ArchiveSpacePerThread() { return 0; }
296+
int Debug::ArchiveSpacePerThread() { return sizeof(ThreadLocal); }
280297

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

deps/v8/src/debug/debug.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,7 @@ class Debug {
309309
static int ArchiveSpacePerThread();
310310
void FreeThreadResources() { }
311311
void Iterate(RootVisitor* v);
312+
void InitThread(const ExecutionAccess& lock) { ThreadInit(); }
312313

313314
bool CheckExecutionState(int id) {
314315
return CheckExecutionState() && break_id() == id;
@@ -690,9 +691,9 @@ class ReturnValueScope {
690691
// Stack allocated class for disabling break.
691692
class DisableBreak BASE_EMBEDDED {
692693
public:
693-
explicit DisableBreak(Debug* debug)
694+
explicit DisableBreak(Debug* debug, bool disable = true)
694695
: debug_(debug), previous_break_disabled_(debug->break_disabled_) {
695-
debug_->break_disabled_ = true;
696+
debug_->break_disabled_ = disable;
696697
}
697698
~DisableBreak() {
698699
debug_->break_disabled_ = previous_break_disabled_;

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 == NULL || per_thread->thread_state() == NULL) {
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

+130
Original file line numberDiff line numberDiff line change
@@ -6207,6 +6207,136 @@ TEST(DebugBreakOffThreadTerminate) {
62076207
}
62086208

62096209

6210+
class ArchiveRestoreThread : public v8::base::Thread,
6211+
public v8::debug::DebugDelegate {
6212+
public:
6213+
ArchiveRestoreThread(v8::Isolate* isolate, int spawn_count)
6214+
: Thread(Options("ArchiveRestoreThread")),
6215+
isolate_(isolate),
6216+
debug_(reinterpret_cast<i::Isolate*>(isolate_)->debug()),
6217+
spawn_count_(spawn_count),
6218+
break_count_(0) {}
6219+
6220+
virtual void Run() {
6221+
v8::Locker locker(isolate_);
6222+
isolate_->Enter();
6223+
6224+
v8::HandleScope scope(isolate_);
6225+
v8::Local<v8::Context> context = v8::Context::New(isolate_);
6226+
v8::Context::Scope context_scope(context);
6227+
6228+
v8::Local<v8::Function> test = CompileFunction(isolate_,
6229+
"function test(n) {\n"
6230+
" debugger;\n"
6231+
" return n + 1;\n"
6232+
"}\n",
6233+
"test");
6234+
6235+
debug_->SetDebugDelegate(this, false);
6236+
v8::internal::DisableBreak enable_break(debug_, false);
6237+
6238+
v8::Local<v8::Value> args[1] = {v8::Integer::New(isolate_, spawn_count_)};
6239+
6240+
int result = test->Call(context, context->Global(), 1, args)
6241+
.ToLocalChecked()
6242+
->Int32Value(context)
6243+
.FromJust();
6244+
6245+
// Verify that test(spawn_count_) returned spawn_count_ + 1.
6246+
CHECK_EQ(spawn_count_ + 1, result);
6247+
6248+
isolate_->Exit();
6249+
}
6250+
6251+
void BreakProgramRequested(v8::Local<v8::Context> context,
6252+
v8::Local<v8::Object> exec_state,
6253+
v8::Local<v8::Value> break_points_hit,
6254+
const std::vector<v8::debug::BreakpointId>&) {
6255+
auto stack_traces = v8::debug::StackTraceIterator::Create(isolate_);
6256+
if (!stack_traces->Done()) {
6257+
v8::debug::Location location = stack_traces->GetSourceLocation();
6258+
6259+
i::PrintF("ArchiveRestoreThread #%d hit breakpoint at line %d\n",
6260+
spawn_count_, location.GetLineNumber());
6261+
6262+
switch (location.GetLineNumber()) {
6263+
case 1: // debugger;
6264+
CHECK_EQ(break_count_, 0);
6265+
6266+
// Attempt to stop on the next line after the first debugger
6267+
// statement. If debug->{Archive,Restore}Debug() improperly reset
6268+
// thread-local debug information, the debugger will fail to stop
6269+
// before the test function returns.
6270+
debug_->PrepareStep(StepNext);
6271+
6272+
// Spawning threads while handling the current breakpoint verifies
6273+
// that the parent thread correctly archived and restored the
6274+
// state necessary to stop on the next line. If not, then control
6275+
// will simply continue past the `return n + 1` statement.
6276+
MaybeSpawnChildThread();
6277+
6278+
break;
6279+
6280+
case 2: // return n + 1;
6281+
CHECK_EQ(break_count_, 1);
6282+
break;
6283+
6284+
default:
6285+
CHECK(false);
6286+
}
6287+
}
6288+
6289+
++break_count_;
6290+
}
6291+
6292+
void MaybeSpawnChildThread() {
6293+
if (spawn_count_ > 1) {
6294+
v8::Unlocker unlocker(isolate_);
6295+
6296+
// Spawn a thread that spawns a thread that spawns a thread (and so
6297+
// on) so that the ThreadManager is forced to archive and restore
6298+
// the current thread.
6299+
ArchiveRestoreThread child(isolate_, spawn_count_ - 1);
6300+
child.Start();
6301+
child.Join();
6302+
6303+
// The child thread sets itself as the debug delegate, so we need to
6304+
// usurp it after the child finishes, or else future breakpoints
6305+
// will be delegated to a destroyed ArchiveRestoreThread object.
6306+
debug_->SetDebugDelegate(this, false);
6307+
6308+
// This is the most important check in this test, since
6309+
// child.GetBreakCount() will return 1 if the debugger fails to stop
6310+
// on the `return n + 1` line after the grandchild thread returns.
6311+
CHECK_EQ(child.GetBreakCount(), 2);
6312+
}
6313+
}
6314+
6315+
int GetBreakCount() { return break_count_; }
6316+
6317+
private:
6318+
v8::Isolate* isolate_;
6319+
v8::internal::Debug* debug_;
6320+
const int spawn_count_;
6321+
int break_count_;
6322+
};
6323+
6324+
TEST(DebugArchiveRestore) {
6325+
v8::Isolate::CreateParams create_params;
6326+
create_params.array_buffer_allocator = CcTest::array_buffer_allocator();
6327+
v8::Isolate* isolate = v8::Isolate::New(create_params);
6328+
6329+
ArchiveRestoreThread thread(isolate, 5);
6330+
// Instead of calling thread.Start() and thread.Join() here, we call
6331+
// thread.Run() directly, to make sure we exercise archive/restore
6332+
// logic on the *current* thread as well as other threads.
6333+
thread.Run();
6334+
CHECK_EQ(thread.GetBreakCount(), 2);
6335+
6336+
isolate->Dispose();
6337+
}
6338+
6339+
62106340
static void DebugEventExpectNoException(
62116341
const v8::Debug::EventDetails& event_details) {
62126342
v8::DebugEvent event = event_details.GetEvent();

0 commit comments

Comments
 (0)