Unverified Commit 5e9ed6d9 authored by Ben Newman's avatar Ben Newman Committed by Michaël Zasso
Browse files

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 <yangguo@chromium.org>
Reviewed-by: default avatarYang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54902}

Refs: https://github.com/v8/v8/commit/a8f6869177685cfb9c199c454a86f4698c260515

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).

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.

PR-URL: https://github.com/nodejs/node/pull/22122


Reviewed-By: default avatarFranziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: default avatarMichaël Zasso <targos@protonmail.com>
parent 4c34302b
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment