v8: backport bb357524 to v8.x-staging (a8f68691 from upstream v8)

[benjamn]

The upstream V8 commit v8/v8@a8f6869 was originally cherry-picked onto 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.

Refs: v8/v8@a8f6869
Refs: #22122
Refs: bb35752
Refs: 5e9ed6d

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

[richardlau]

Patch level in v8-version.h should be bumped as per https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#backporting-to-abandoned-branches.

[benjamn]

@richardlau Thanks for the reminder about the patch level. Mind kicking off a test run for this?

[richardlau]

Node CI: https://ci.nodejs.org/job/node-test-pull-request/17061/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1667/

[richardlau]

@benjamn Looks like there's an issue with test/cctest/test-debug.cc:

https://ci.nodejs.org/job/node-test-commit-v8-linux/1667/nodes=ppcle-ubuntu1404,v8test=v8test/console

12:17:51 ../test/cctest/test-debug.cc: In member function 'virtual void ArchiveRestoreThread::Run()':
12:17:51 ../test/cctest/test-debug.cc:6236:58: error: no matching function for call to 'v8::internal::DisableBreak::DisableBreak(v8::internal::Debug*&, bool)'
12:17:51      v8::internal::DisableBreak enable_break(debug_, false);
12:17:51                                                           ^
12:17:51 ../test/cctest/test-debug.cc:6236:58: note: candidate is:
12:17:51 In file included from ../test/cctest/test-debug.cc:36:0:
12:17:51 .././src/debug/debug.h:694:12: note: v8::internal::DisableBreak::DisableBreak(v8::internal::Debug*)
12:17:51    explicit DisableBreak(Debug* debug)
12:17:51             ^
12:17:51 .././src/debug/debug.h:694:12: note:   candidate expects 1 argument, 2 provided

[benjamn]

Ok, I've backported a small change from upstream V8 (6.7.176) that should fix that compilation error.

[richardlau]

I'm having trouble accessing the CI at the moment (gateway timeouts) so I haven't been able to start a new CI run. May not be around much this weekend, but maybe another collaborator can start one when the CI is more responsive.

[richardlau]

New V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1677/

[benjamn]

Fixed a couple more trivial compilation errors in test-debug.cc: a0e5d6f66e0069a729b210cb8d80f5702e990df5 and df50aab3d6add07b719944e6a794a31b13c1b87c. I say "trivial" because some internal APIs have changed between Node 8 and Node 10 (and upstream V8), but not any of the exercised behavior.

[richardlau]

Another V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1680/

[richardlau]

(This will obviously need a clean Node CI run too, but lets get a green V8 CI first).

[benjamn]

@richardlau Thanks for your patience!

[benjamn]

V8 CI looking green! 😌

[benjamn]

Can we do a Node CI run? I'm assuming that will auto-rebase, but I'm happy to rebase these changes if that's not the case.

[benjamn]

@richardlau Just resurfacing this since it's been a minute. Thanks for your help so far.

[richardlau]

Node CI: https://ci.nodejs.org/job/node-test-pull-request/17451/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1716/

[benjamn]

This seems ready to land—do we need any more approvals?

[MylesBorins]

@benjamn I'd like sign off from someone from the @nodejs/v8-update team... Generally we do not float changes onto V8 that do not exist upstream in some capacity. This PR is made up of 5 different commits, not all of which appear to have upstream equivalence.

FWIW it is fine for a change to require modification to work when backported, but generally those changes are squashed into the original commit.

What I would like to see before this lands would be the following

• Each commit be 1:1 with an upstream commit on V8
• Patch level bump in 9d6f70c squashed into the commit that is bumped the patch level
• Explicit documentation about what the extra commits are, why they are neccessary
• Sign off from someone in @nodejs/v8-update on the changes
• Bonus: commits to all have the proper style + meta data (I can do this when landing)

[MylesBorins]

Please do not land this until the request I've made above are responded to

[benjamn]

First let me explain why the additional commits should be uncontroversial, and then I'll explain the difference between the main commit and its upstream equivalent.

If you look at the commit message for ad3b8e76c6e47204535b5504738c12e6348a3c60, it explains that the change comes from a commit first included in V8 6.7.176: v8/v8@cc9736a. When this code is updated to V8 6.7.176 or later, there will be no merge conflicts. I didn't want to backport anything else from that commit, because the disable = false default parameter was all I needed. It's an isolated, backwards-compatible change, since call sites that don't pass a second argument use the default value of false, as before.

Commit a0e5d6f66e0069a729b210cb8d80f5702e990df5 is a V8 cctest-only commit to add a parameter to the BreakProgramRequested method that I don't actually use, but that is necessary to avoid a compiler error. When V8 is next updated to a version that contains this test, these changes can be safely obliterated by the new test code.

Commit df50aab3d6add07b719944e6a794a31b13c1b87c might as well be squashed into a0e5d6f66e0069a729b210cb8d80f5702e990df5, as I forgot the v8:: namespace in one of the other parameters to BreakProgramRequested. Again, it's a V8 cctest-only commit, so I believe there is very little risk to Node.

Commit 9d6f70c3771677f5c20ff5bb03380a3c5a810ccb bumps the V8_PATCH_LEVEL.

[ofrobots]

The second commit (ad3b8e76c6e47204535b5504738c12e6348a3c60) is a subset of an upstream commit (v8/v8@cc9736a). Can the entire patch be back-ported? (an additional patch version bump should be included in this commit)

The third and forth (a0e5d6f66e0069a729b210cb8d80f5702e990df5, df50aab3d6add07b719944e6a794a31b13c1b87c) could probably be squashed. I am not sure of their pedigree. If the responsible commit upstream is back-portable, we should at least document what commit the patch ends up being a subset of.

The fifth commit should be squashed into the first as @MylesBorins pointed out.

EDIT: @benjamn I didn't see your comment before this comment.

[benjamn]

(Thanks @ofrobots!)

That leaves commit 9faf12f2b18b8a63ec0827d8cdf9011712b453ed, which is identical (outside of test code) to the version of this PR that landed on nodejs/master (bb35752) and the commit that is currently included in Node 10.10.0 (5e9ed6d). In other words, the difference between these commits and the original V8 commit has already been discussed (in #22122), so there's nothing new in this PR except some tweaks to make the tests Node 8-compatible.

As for why we avoid creating a new DebugScope in this version, the tl;dr is that the addition of the DebugScope was a last-minute change in the V8 version of this commit (see https://chromium-review.googlesource.com/c/v8/v8/+/833260), and it turns out we do not need to restore debugging state unless the thread we're switching back to was in a DebugScope when it was suspended. That's good news, because the DebugScope class is not fully reentrant in the versions of V8 used by Node 8 and 10, though it has become (more) reentrant in the most recent versions of V8, which is why the original upstream commit is safe. In that sense, this is a more conservative change than the original change, which I think should make it easier to accept (and indeed it has already been accepted in Node master and 10.10.0+).

Hope that helps! Happy to make any squashing/reformatting changes you like, though I would love direct guidance about that.

[benjamn]

One more note: the two test-only commits (a0e5d6f66e0069a729b210cb8d80f5702e990df5 and df50aab3d6add07b719944e6a794a31b13c1b87c) are confined to the very code that I added in v8/v8@a8f6869, so that's their pedigree. The changes were necessary to make the test I added compile in Node 8 and Node 10. In other words, the only upstream commit that could correspond to those two changes is the commit that I'm already backporting in this PR!

[benjamn]

@MylesBorins (&al.) I've squashed everything down to two commits. The non-test parts of the main commit (c204389777622fc00050d3ddad07c543f4180662) are identical to what already landed on master and v10.x, and that commit now includes the V8_PATCH_LEVEL bump. I also cleaned up the formatting of that commit so it includes a note about the backporting story, with all relevant Refs:, and the original commit message is less cluttered. To my eyes, it now looks exactly like what's mandated in maintaining-V8.md.

I've left 1134dcaefc83d931d66098cd74d5f6b21db34429 as a separate commit because it corresponds to a different upstream commit (v8/v8@cc9736a). I don't know how to evaluate whether the other changes in that commit are worth cherry-picking, but I'm confident the bool default = false change is isolated and backwards compatible. My gut tells me it's not worth the risk to include those other changes. Everything should merge cleanly when V8 is next updated to 6.7.176 or later, since the hunk in question is identical between the commits.

[MylesBorins]

@benjamn I think we can just squash 1134dca into c204389 if we don't plan to backport the entire V8 change. The change by itself looks unlikely to break anything, but I see no advantage to keeping it separate, in fact it will only make an instance of the tree with a broken test (bad for bisecting).

Assuming the below CI is green and nobody from @nodejs/v8-update objects, I'll go ahead and land this as a single commit.

CI: https://ci.nodejs.org/job/node-test-pull-request/17702/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1746/

[benjamn]

Totally agree with the point about bisection! I'll squash everything in a moment.

I suspect those test failures are unrelated. Windows:

not ok 226 parallel/test-http2-client-upload
not ok 522 sequential/test-http2-ping-flood # TODO : Fix flaky test

and Ubuntu:

not ok 2028 sequential/test-fs-watch

[benjamn]

I think this is ready to go, pending @nodejs/v8-update approval and possibly another test run, since I rebased and squashed everything after the last CI run.

[benjamn]

@MylesBorins I just rebased and bumped V8_PATCH_LEVEL again. Not sure if this needs another test run, given the time that has passed. Can we get this into v8.x-staging soon?

[MylesBorins]

One more CI: https://ci.nodejs.org/job/node-test-pull-request/18084/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1782/

I pinged the V8 team in their chat to get someone to review

[MylesBorins]

Rebuilding failed linux tests: https://ci.nodejs.org/job/node-test-commit-linux-containered/8201/

edit: one more time

https://ci.nodejs.org/job/node-test-commit-linux-containered/8223/

[MylesBorins]

landed in ce65d84

Thanks for sticking around!