Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v12.x] deps: V8: backport postmortem metadata fixes #30870

Closed
wants to merge 4 commits into from
Closed

[v12.x] deps: V8: backport postmortem metadata fixes #30870

wants to merge 4 commits into from

Conversation

mmarchini
Copy link
Contributor

V8 7.7 branch is closed, I tried backporting these commits upstream but it was not approved. These commits are necessary for llnode to work with Node.js v12.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. v12.x v8 engine Issues and PRs related to the V8 dependency. labels Dec 9, 2019
mhdawson
mhdawson approved these changes Dec 9, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nodejs-github-bot
Copy link
Collaborator

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

targos
targos reviewed Dec 10, 2019
View reviewed changes
deps/v8/BUILD.gn
@@ -870,6 +870,8 @@ action("postmortem-metadata") {
"src/objects/code.h",
"src/objects/data-handler.h",
"src/objects/data-handler-inl.h",
"src/objects/descriptor-array.h",
"src/objects/descriptor-array-inl.h",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need the same change in v8.gyp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so, but it worked on my machine when I built it (metadata was correct in the final binary). I also couldn't find where to change on V8.gyp when I searched using grep. I think we're inferring this parameter from BUILD.gn now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes I'm sorry, it must be one of the lists that are scraped automatically from build.gn

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@mmarchini
Copy link
Contributor Author

SmartOS failure is legit. I'll send a commit to fix it.

@mmarchini mmarchini force-pushed the v12.x-backport-postmortem-metadata branch from 0c80610 to 1730fc1 Compare December 27, 2019 18:56
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 27, 2019 •
edited by mmarchini
Loading

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

@mmarchini
Copy link
Contributor Author

Reverted 59b4640 and f056d55, since string metadata name changes were reverted in v8/v8@b38dfaf3a633. cc @cjihrig

@nodejs-github-bot
Copy link
Collaborator

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

@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2019
@mmarchini
Copy link
Contributor Author

Should I land this or should I wait for someone from @nodejs/releasers to do it? I couldn't find this information on https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md or https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md

mmarchini and others added 4 commits January 7, 2020 20:00
@mmarchini @MylesBorins
deps: V8: cherry-pick cc5016e1b702
2badc61 
Original commit message:

    [postmortem] add metadata for the new DescriptorArray layout

    [email protected]

    Ref: nodejs/llnode#255
    Change-Id: Icda271123375db5c381fe1d1bba13dcc26f26d7c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1832311
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64169}

Refs: v8/v8@cc5016e
@mmarchini @MylesBorins
deps: V8: cherry-pick b38dfaf3a633
d86bcd7 
Original commit message:

    [postmortem] update Symbol and *String metadata

    Symbol and *String classes are now declared on Torque with
    generateCppClass, which means they don't use macro accessors anymore. As
    such, the gen-postmortem-metadata script is not able to automatically
    detect fields for those classes. Define metadata for those fields
    manually for now. In the future we might want to generate it from Torque
    for consistency.

    Also renamed a few *String fields metadata to match the expected format
    (className__fieldName__fieldType). For more context:
    nodejs/llnode#287 (comment).

    [email protected], [email protected], [email protected], [email protected]

    Change-Id: I82fe8315cdbfd1b8c64c6a8d5dc011b1edaec39e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1847783
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64313}

Refs: v8/v8@b38dfaf
@mmarchini @MylesBorins
Revert "src: update v8abbr.h for V8 7.7"
0a42e03 
This reverts commit 59b4640.
@mmarchini @MylesBorins
Revert "test: update postmortem metadata test for V8 7.7"
15a2ca7 
This reverts commit f056d55.
@MylesBorins MylesBorins force-pushed the v12.x-backport-postmortem-metadata branch from 1730fc1 to 15a2ca7 Compare January 8, 2020 01:01
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 8, 2020 •
edited by MylesBorins
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/28280/
https://ci.nodejs.org/job/node-test-commit-v8-linux/2773/

@MylesBorins
Copy link
Contributor

@mmarchini for LTS lines it should be left to backporters to land. I've gone ahead and rebase the PR against the 12.x-staging branch as we just updated to 7.8

@targos targos force-pushed the v12.x-staging branch 2 times, most recently from 3900f4a to 32e5c39 Compare January 8, 2020 16:34
@mmarchini mmarchini mentioned this pull request Jan 13, 2020
Closed
@mmarchini
Copy link
Contributor Author

@MylesBorins there is a small conflict on common.gypi, do you mind if I rebase to fix it, or do you want to do it?

@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 14, 2020 via email

targos pushed a commit that referenced this pull request Jan 14, 2020
@mmarchini @targos
deps: V8: cherry-pick cc5016e1b702
8c5e951 
Original commit message:

    [postmortem] add metadata for the new DescriptorArray layout

    [email protected]

    Ref: nodejs/llnode#255
    Change-Id: Icda271123375db5c381fe1d1bba13dcc26f26d7c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1832311
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64169}

Refs: v8/v8@cc5016e
PR-URL: #30870
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
@mmarchini @targos
deps: V8: cherry-pick b38dfaf3a633
015671d 
Original commit message:

    [postmortem] update Symbol and *String metadata

    Symbol and *String classes are now declared on Torque with
    generateCppClass, which means they don't use macro accessors anymore. As
    such, the gen-postmortem-metadata script is not able to automatically
    detect fields for those classes. Define metadata for those fields
    manually for now. In the future we might want to generate it from Torque
    for consistency.

    Also renamed a few *String fields metadata to match the expected format
    (className__fieldName__fieldType). For more context:
    nodejs/llnode#287 (comment).

    [email protected], [email protected], [email protected], [email protected]

    Change-Id: I82fe8315cdbfd1b8c64c6a8d5dc011b1edaec39e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1847783
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64313}

Refs: v8/v8@b38dfaf
PR-URL: #30870
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
@mmarchini @targos
Revert "src: update v8abbr.h for V8 7.7"
6bc2ce2 
This reverts commit 59b4640.
PR-URL: #30870
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Jan 14, 2020
@mmarchini @targos
Revert "test: update postmortem metadata test for V8 7.7"
4fd4a73 
This reverts commit f056d55.
PR-URL: #30870
Reviewed-By: Michael Dawson <[email protected]>
@targos
Copy link
Member

targos commented Jan 14, 2020

Went ahead and landed this in 8c5e951, 015671d, 6bc2ce2, and 4fd4a73. Thanks for the PR!

@targos targos closed this Jan 14, 2020
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@mmarchini @BethGriggs
deps: V8: cherry-pick cc5016e1b702
554c7c2 
Original commit message:

    [postmortem] add metadata for the new DescriptorArray layout

    [email protected]

    Ref: nodejs/llnode#255
    Change-Id: Icda271123375db5c381fe1d1bba13dcc26f26d7c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1832311
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Yang Guo <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64169}

Refs: v8/v8@cc5016e
PR-URL: #30870
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@mmarchini @BethGriggs
deps: V8: cherry-pick b38dfaf3a633
317c3df 
Original commit message:

    [postmortem] update Symbol and *String metadata

    Symbol and *String classes are now declared on Torque with
    generateCppClass, which means they don't use macro accessors anymore. As
    such, the gen-postmortem-metadata script is not able to automatically
    detect fields for those classes. Define metadata for those fields
    manually for now. In the future we might want to generate it from Torque
    for consistency.

    Also renamed a few *String fields metadata to match the expected format
    (className__fieldName__fieldType). For more context:
    nodejs/llnode#287 (comment).

    [email protected], [email protected], [email protected], [email protected]

    Change-Id: I82fe8315cdbfd1b8c64c6a8d5dc011b1edaec39e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1847783
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64313}

Refs: v8/v8@b38dfaf
PR-URL: #30870
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@mmarchini @BethGriggs
Revert "src: update v8abbr.h for V8 7.7"
29608be 
This reverts commit 59b4640.
PR-URL: #30870
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@mmarchini @BethGriggs
Revert "test: update postmortem metadata test for V8 7.7"
023a802 
This reverts commit f056d55.
PR-URL: #30870
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mmarchini @nodejs-github-bot @MylesBorins @targos @mhdawson