deps: backport a8f6869 from upstream V8 #22122

benjamn · 2018-08-04T14:05:27Z

Refs: v8/v8@a8f6869

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}

Once this PR is approved/merged, I will also submit a PR to backport it to v8.x-staging so that it can be included in Node 8.12.0, if all goes smoothly.

Thanks again to @hashseed for working with me on the V8 side.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
~~documentation is changed or added~~ (implementation detail)
commit message follows commit guidelines

mscdex · 2018-08-04T14:45:14Z

The v8 embedder string in common.gypi needs to be updated as well I believe.

targos · 2018-08-04T14:48:04Z

Yes and V8_PATCH_LEVEL should not be updated

benjamn · 2018-08-04T15:26:41Z

@mscdex Thanks, will do!

@targos I noticed that, and I did not recall making that change myself. Apparently it was introduced by the v8/tools/node/backport_node.py script here. Happy to revert.

targos · 2018-08-04T15:40:33Z

@benjamn I didn't know about that script.
Our doc about updating V8 is here: https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#maintenance-process

It is a little bit outdated because update-v8 was merged with node-core-utils, but the command is roughly the same: git node v8 backport <SHA>. It takes care of updating the embedder string and writing the commit message.

hashseed · 2018-08-04T15:46:09Z

Yeah the backport_node script is a bit outdated wrt updating embedded string.

benjamn · 2018-08-06T14:30:45Z

What's the next step here?

joyeecheung · 2018-08-07T07:59:37Z

cc @nodejs/v8-update

MylesBorins · 2018-08-07T20:54:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16259/

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

benjamn · 2018-08-08T00:13:14Z

Thanks @MylesBorins, this is useful feedback.

It looks like SetDebugDelegate(debug::DebugDelegate* delegate, bool pass_ownership) was simplified to just SetDebugDelegate(debug::DebugDelegate* delegate) in v8/v8@37dcd83, 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 v8/v8@e404670, 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).

I'm not sure why ninja -C out/Debug didn't catch these errors before. Maybe they only show up in release builds? Or maybe node/deps/v8/test/cctest/test-debug.cc just wasn't built before? 🤔

benjamn · 2018-08-08T00:59:25Z

The only test failure I'm seeing (after rebasing against master) seems to be unrelated?

not ok 1844 parallel/test-tls-server-verify --- duration_ms: 0.845 severity: fail exitcode: 1 stack: |- 0 0 connecting with agent1 0 1 connecting with agent2 0 2 connecting with agent3 0 3 connecting with nocert 1 0 connecting with agent1 1 1 connecting with agent2 1 2 connecting with agent3 1 3 connecting with nocert 2 0 connecting with agent1 2 1 connecting with agent2 2 2 connecting with agent3 2 3 connecting with nocert 3 0 connecting with agent1 3 1 connecting with agent2 3 2 connecting with agent3 3 3 connecting with nocert 4 0 connecting with agent1 4 1 connecting with agent2 4 2 connecting with agent3 4 3 connecting with nocert 5 0 connecting with agent1 5 1 connecting with agent2 5 2 connecting with agent3 5 3 connecting with agent4 5 4 connecting with nocert 0 Running 'Do not request certs. Everyone is unauthorized.' 0 - unauthed connection: null 0 - unauthed connection: null 0 - unauthed connection: null 0 0 * unauthed 0 1 * unauthed 0 2 * unauthed 0 - unauthed connection: null 0 3 * unauthed 1 Running 'Allow both authed and unauthed connections with CA1' 1 - authed connection: agent1 1 - unauthed connection: DEPTH_ZERO_SELF_SIGNED_CERT 1 1 * unauthed 1 0 * authed 1 - unauthed connection: UNABLE_TO_VERIFY_LEAF_SIGNATURE 1 2 * unauthed 1 - unauthed connection: UNABLE_TO_GET_ISSUER_CERT 1 3 * unauthed 2 Running 'Do not request certs at connection. Do that later' 2 - connected, renegotiating 2 - authed connection: agent1 2 0 * authed 2 - unauthed connection: null 2 1 * unauthed 2 - unauthed connection: null 2 2 * unauthed 2 - unauthed connection: null 2 3 * unauthed 3 Running 'Allow only authed connections with CA1' 3 - authed connection: agent1 3 0 * authed 4 Running 'Allow only authed connections with CA1 and CA2' 4 - authed connection: agent1 4 0 * authed 4 - authed connection: agent3 4 2 * authed 5 Running 'Allow only certs signed by CA2 but not in the CRL' assert.js:84 throw new AssertionError(obj); ^

AssertionError [ERR_ASSERTION]: 5 2 agent3 rejected, but should NOT have been
    at ChildProcess.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-tls-server-verify.js:232:14)
    at ChildProcess.emit (events.js:182:13)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:240:12)

MylesBorins · 2018-08-08T01:10:17Z

@benjamn that's a known failure right now 😄

#22182

MylesBorins · 2018-08-08T17:52:38Z

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

MylesBorins · 2018-08-08T18:41:52Z

Lots of failures, please dig in to the results

richardlau · 2018-08-08T19:03:01Z

test-trace-event-promises.js failures will be fixed by #22200.

MylesBorins · 2018-08-08T19:08:22Z

Lot's of V8 Failures as well

https://ci.nodejs.org/job/node-test-commit-v8-linux/1586/testReport/

V8-Ci on master for comparison: https://ci.nodejs.org/job/node-test-commit-v8-linux/1587/

benjamn · 2018-08-08T19:48:30Z

Lots of test suite failures, but not many different tests failing. As far as I can tell, all the node-test-* failures should be fixed by #22200.

Regarding the V8 failures, those same tests are passing on V8 master with my change applied, so I would guess these failures are specific to the back-porting. I'll keep digging into that!

benjamn · 2018-08-08T19:55:43Z

Just rebased to include #22200 (d7e1847). I have not done anything to address the V8 failures, yet.

https://ci.nodejs.org/job/node-test-commit-linux/nodes=ubuntu1604-32/20546/ seems to be from the previous test run (old revision, test-trace-event-promises still failing), fwiw. Perhaps I should have merged master into my branch instead of force-pushing a new revision?

MylesBorins · 2018-08-08T20:03:47Z

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

MylesBorins · 2018-08-08T20:04:59Z

@benjamingr our CI will auto rebase against master when you run the job 😄

benjamn · 2018-08-09T02:33:30Z

I think I've tracked down the cause of the V8 test failures, and I've submitted a possible solution as another V8 change request: https://chromium-review.googlesource.com/c/v8/v8/+/1168449

I could make those changes locally, though it might be better to get them landed in V8 first?

benjamn · 2018-08-09T13:48:59Z

Ok, I've provisionally cherry-picked my local commit to fix the V8 test failures, even though it has not landed upstream yet (I submitted it for review just last night). If the tests pass here, that would be a good sign that it's a safe and useful change. I will keep working on https://chromium-review.googlesource.com/c/v8/v8/+/1168449 with @hashseed and anyone else who's interested.

benjamn · 2018-08-13T19:40:36Z

It looks like my follow-up fix may not be necessary for recent versions of V8 (according to @hashseed here), but is definitely necessary for Node's current version of V8, so I'm unsure how to proceed.

Should we land an unnecessary change upstream just so Node's copy of V8 does not diverge from upstream, or should we just make the change locally, as I did above? Has this kind of situation ever happened before, @MylesBorins?

It looks like SetDebugDelegate(debug::DebugDelegate* delegate, bool pass_ownership) was simplified to just SetDebugDelegate(debug::DebugDelegate* delegate) in v8/v8@37dcd83, 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 v8/v8@e404670, 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). I'm not sure why `ninja -C out/Debug` didn't catch these errors before. Maybe they only show up in release builds?

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: https://ci.nodejs.org/job/node-test-commit-v8-linux/1601/

targos · 2018-09-03T18:01:34Z

I rebased the PR on top of master.
New CI: https://ci.nodejs.org/job/node-test-pull-request/16993/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1649/

targos · 2018-09-04T06:38:59Z

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

targos · 2018-09-04T12:39:20Z

Landed in bb35752

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} Refs: v8/v8@a8f6869 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 v8/v8@37dcd83, 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 v8/v8@e404670, 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: #22122 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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. Refs: v8/v8@a8f6869 Refs: nodejs#22122 Refs: nodejs@bb35752 Refs: nodejs@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@{nodejs#54902} PR-URL: nodejs#22714 ADDITIONAL CHANGES: Support 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 More immediately, these changes should fix a cctest compile error in this backport PR: nodejs#22714 (comment)

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>

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 4, 2018

benjamn force-pushed the backport_v8_a8f6869 branch from 1a5cb4e to 39fda8e Compare August 4, 2018 15:29

benjamn force-pushed the backport_v8_a8f6869 branch from 8724158 to af55577 Compare August 8, 2018 00:37

fhinkel approved these changes Aug 8, 2018

View reviewed changes

MylesBorins self-assigned this Aug 8, 2018

benjamn force-pushed the backport_v8_a8f6869 branch from af55577 to 115f04f Compare August 8, 2018 19:53

benjamn force-pushed the backport_v8_a8f6869 branch from 115f04f to ed69b68 Compare August 9, 2018 13:45

Ben Newman added 3 commits September 3, 2018 19:59

Bump v8_embedder_string to '-node.17'.

8d66ed7

targos force-pushed the backport_v8_a8f6869 branch from 77567b7 to 8d66ed7 Compare September 3, 2018 18:00

targos approved these changes Sep 4, 2018

View reviewed changes

targos closed this Sep 4, 2018

benjamn mentioned this pull request Sep 5, 2018

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

Closed

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

aliceUnhinged613 mentioned this pull request May 16, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 aliceUnhinged613/node#58

Open

This was referenced May 16, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 137717unity/node#57

Open

[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 Rednickle/node-1#49

Open

kissofire mentioned this pull request May 16, 2022

[Snyk] Security upgrade gatsby from 2.13.73 to 2.19.36 kissofire/node#67

Open

wedataintelligence mentioned this pull request May 16, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 wedataintelligence/node#33

Open

ron-a20 mentioned this pull request May 16, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 ron-a20/node#66

Open

molllyn1 mentioned this pull request May 17, 2022

[Snyk] Security upgrade gatsby from 2.18.18 to 2.19.36 molllyn1/node#48

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: backport a8f6869 from upstream V8 #22122

deps: backport a8f6869 from upstream V8 #22122

benjamn commented Aug 4, 2018 •

edited

Loading

mscdex commented Aug 4, 2018

targos commented Aug 4, 2018

benjamn commented Aug 4, 2018 •

edited

Loading

targos commented Aug 4, 2018

hashseed commented Aug 4, 2018 •

edited

Loading

benjamn commented Aug 6, 2018

joyeecheung commented Aug 7, 2018

MylesBorins commented Aug 7, 2018

benjamn commented Aug 8, 2018 •

edited

Loading

benjamn commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

richardlau commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

benjamn commented Aug 8, 2018

benjamn commented Aug 8, 2018 •

edited

Loading

MylesBorins commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

benjamn commented Aug 9, 2018

benjamn commented Aug 9, 2018

benjamn commented Aug 13, 2018 •

edited

Loading

targos commented Sep 3, 2018

targos commented Sep 4, 2018

targos commented Sep 4, 2018

deps: backport a8f6869 from upstream V8 #22122

deps: backport a8f6869 from upstream V8 #22122

Conversation

benjamn commented Aug 4, 2018 • edited Loading

Checklist

mscdex commented Aug 4, 2018

targos commented Aug 4, 2018

benjamn commented Aug 4, 2018 • edited Loading

targos commented Aug 4, 2018

hashseed commented Aug 4, 2018 • edited Loading

benjamn commented Aug 6, 2018

joyeecheung commented Aug 7, 2018

MylesBorins commented Aug 7, 2018

benjamn commented Aug 8, 2018 • edited Loading

benjamn commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

richardlau commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

benjamn commented Aug 8, 2018

benjamn commented Aug 8, 2018 • edited Loading

MylesBorins commented Aug 8, 2018

MylesBorins commented Aug 8, 2018

benjamn commented Aug 9, 2018

benjamn commented Aug 9, 2018

benjamn commented Aug 13, 2018 • edited Loading

targos commented Sep 3, 2018

targos commented Sep 4, 2018

targos commented Sep 4, 2018

benjamn commented Aug 4, 2018 •

edited

Loading

benjamn commented Aug 4, 2018 •

edited

Loading

hashseed commented Aug 4, 2018 •

edited

Loading

benjamn commented Aug 8, 2018 •

edited

Loading

benjamn commented Aug 8, 2018 •

edited

Loading

benjamn commented Aug 13, 2018 •

edited

Loading