Skip to content

Commit

Permalink
deps: backport a8f6869 from upstream V8
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
Ben Newman authored and MylesBorins committed Oct 31, 2018
1 parent 7ab253f commit ce65d84
Show file tree
Hide file tree
Showing 7 changed files with 166 additions and 11 deletions.
2 changes: 2 additions & 0 deletions deps/v8/AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Yandex LLC <*@yandex-team.ru>
StrongLoop, Inc. <*@strongloop.com>
Facebook, Inc. <*@fb.com>
Facebook, Inc. <*@oculus.com>
Meteor Development Group <*@meteor.com>

Aaron Bieber <deftly@gmail.com>
Abdulla Kamar <abdulla.kamar@gmail.com>
Expand All @@ -44,6 +45,7 @@ Andrew Paprocki <andrew@ishiboo.com>
Andrei Kashcha <anvaka@gmail.com>
Anna Henningsen <anna@addaleax.net>
Bangfu Tao <bangfu.tao@samsung.com>
Ben Newman <ben@meteor.com>
Ben Noordhuis <info@bnoordhuis.nl>
Benjamin Tan <demoneaux@gmail.com>
Bert Belder <bertbelder@gmail.com>
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/include/v8-version.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#define V8_MAJOR_VERSION 6
#define V8_MINOR_VERSION 2
#define V8_BUILD_NUMBER 414
#define V8_PATCH_LEVEL 69
#define V8_PATCH_LEVEL 70

// Use 1 for candidates and 0 otherwise.
// (Boolean macro values are not supported by all preprocessors.)
Expand Down
29 changes: 23 additions & 6 deletions deps/v8/src/debug/debug.cc
Original file line number Diff line number Diff line change
Expand Up @@ -264,19 +264,36 @@ void Debug::ThreadInit() {


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


char* Debug::RestoreDebug(char* storage) {
// Simply reset state. Don't restore anything.
ThreadInit();
MemCopy(reinterpret_cast<char*>(&thread_local_), storage,
ArchiveSpacePerThread());

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_);
}
}

return storage + ArchiveSpacePerThread();
}

int Debug::ArchiveSpacePerThread() { return 0; }
int Debug::ArchiveSpacePerThread() { return sizeof(ThreadLocal); }

void Debug::Iterate(RootVisitor* v) {
v->VisitRootPointer(Root::kDebug, &thread_local_.return_value_);
Expand Down
5 changes: 3 additions & 2 deletions deps/v8/src/debug/debug.h
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,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;
Expand Down Expand Up @@ -690,9 +691,9 @@ class ReturnValueScope {
// Stack allocated class for disabling break.
class DisableBreak BASE_EMBEDDED {
public:
explicit DisableBreak(Debug* debug)
explicit DisableBreak(Debug* debug, bool disable = true)
: debug_(debug), previous_break_disabled_(debug->break_disabled_) {
debug_->break_disabled_ = true;
debug_->break_disabled_ = disable;
}
~DisableBreak() {
debug_->break_disabled_ = previous_break_disabled_;
Expand Down
8 changes: 6 additions & 2 deletions deps/v8/src/v8threads.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -127,7 +131,7 @@ bool ThreadManager::RestoreThread() {
isolate_->FindPerThreadDataForThisThread();
if (per_thread == NULL || per_thread->thread_state() == NULL) {
// This is a new thread.
isolate_->stack_guard()->InitThread(access);
InitThread(access);
return false;
}
ThreadState* state = per_thread->thread_state();
Expand Down
1 change: 1 addition & 0 deletions deps/v8/src/v8threads.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ class ThreadManager {
void Lock();
void Unlock();

void InitThread(const ExecutionAccess&);
void ArchiveThread();
bool RestoreThread();
void FreeThreadResources();
Expand Down
130 changes: 130 additions & 0 deletions deps/v8/test/cctest/test-debug.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6207,6 +6207,136 @@ 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<i::Isolate*>(isolate_)->debug()),
spawn_count_(spawn_count),
break_count_(0) {}

virtual void Run() {
v8::Locker locker(isolate_);
isolate_->Enter();

v8::HandleScope scope(isolate_);
v8::Local<v8::Context> context = v8::Context::New(isolate_);
v8::Context::Scope context_scope(context);

v8::Local<v8::Function> test = CompileFunction(isolate_,
"function test(n) {\n"
" debugger;\n"
" return n + 1;\n"
"}\n",
"test");

debug_->SetDebugDelegate(this, false);
v8::internal::DisableBreak enable_break(debug_, false);

v8::Local<v8::Value> 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<v8::Context> context,
v8::Local<v8::Object> exec_state,
v8::Local<v8::Value> break_points_hit,
const std::vector<v8::debug::BreakpointId>&) {
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, false);

// 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();
Expand Down

0 comments on commit ce65d84

Please sign in to comment.