Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit ce65d84

Browse filesBrowse files
Ben NewmanMylesBorins
Ben Newman
authored and
MylesBorins
committed
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 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} PR-URL: #22714 Refs: v8/v8@a8f6869 Refs: #22122 Refs: bb35752 Refs: 5e9ed6d Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org>
1 parent 7ab253f commit ce65d84
Copy full SHA for ce65d84

File tree

Expand file treeCollapse file tree

7 files changed

+166
-11
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+166
-11
lines changed
Open diff view settings
Collapse file

‎deps/v8/AUTHORS‎

Copy file name to clipboardExpand all lines: deps/v8/AUTHORS
+2Lines changed: 2 additions & 0 deletions
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 <deftly@gmail.com>
3536
Abdulla Kamar <abdulla.kamar@gmail.com>
@@ -44,6 +45,7 @@ Andrew Paprocki <andrew@ishiboo.com>
4445
Andrei Kashcha <anvaka@gmail.com>
4546
Anna Henningsen <anna@addaleax.net>
4647
Bangfu Tao <bangfu.tao@samsung.com>
48+
Ben Newman <ben@meteor.com>
4749
Ben Noordhuis <info@bnoordhuis.nl>
4850
Benjamin Tan <demoneaux@gmail.com>
4951
Bert Belder <bertbelder@gmail.com>
Collapse file

‎deps/v8/include/v8-version.h‎

Copy file name to clipboardExpand all lines: deps/v8/include/v8-version.h
+1-1Lines changed: 1 addition & 1 deletion
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.)
Collapse file

‎deps/v8/src/debug/debug.cc‎

Copy file name to clipboardExpand all lines: deps/v8/src/debug/debug.cc
+23-6Lines changed: 23 additions & 6 deletions
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_);
Collapse file

‎deps/v8/src/debug/debug.h‎

Copy file name to clipboardExpand all lines: deps/v8/src/debug/debug.h
+3-2Lines changed: 3 additions & 2 deletions
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_;
Collapse file

‎deps/v8/src/v8threads.cc‎

Copy file name to clipboardExpand all lines: deps/v8/src/v8threads.cc
+6-2Lines changed: 6 additions & 2 deletions
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();
Collapse file

‎deps/v8/src/v8threads.h‎

Copy file name to clipboardExpand all lines: deps/v8/src/v8threads.h
+1Lines changed: 1 addition & 0 deletions
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();
Collapse file

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

Copy file name to clipboardExpand all lines: deps/v8/test/cctest/test-debug.cc
+130Lines changed: 130 additions & 0 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.