FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v12.x] deps: V8: backport postmortem metadata fixes by mmarchini · Pull Request #30870 · nodejs/node · GitHub

/ node Public

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

Closed
mmarchini wants to merge 4 commits into
nodejs:v12.x-stagingfrom
mmarchini:v12.x-backport-postmortem-metadata
ConversationCommits (4)ChecksFiles changed
Closed

[v12.x] deps: V8: backport postmortem metadata fixes#30870
mmarchini wants to merge 4 commits into
nodejs:v12.x-stagingfrom
mmarchini:v12.x-backport-postmortem-metadata

Conversation

mmarchini commented Dec 9, 2019

Copy link
Copy Markdown
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 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 approved these changes Dec 9, 2019
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

nodejs-github-bot commented Dec 9, 2019

Copy link
Copy Markdown
Collaborator

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

targos reviewed Dec 10, 2019
View reviewed changes
Comment thread deps/v8/BUILD.gn Outdated

targos Dec 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mmarchini Dec 10, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

targos Dec 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

nodejs-github-bot commented Dec 12, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Dec 12, 2019

Copy link
Copy Markdown
Collaborator

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

mmarchini commented Dec 12, 2019

Copy link
Copy Markdown
Contributor Author

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

mmarchini force-pushed the v12.x-backport-postmortem-metadata branch from 0c80610 to 1730fc1 Compare December 27, 2019 18:56

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

Copy link
Copy Markdown
Collaborator

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 commented Dec 27, 2019

Copy link
Copy Markdown
Contributor Author

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

nodejs-github-bot commented Dec 27, 2019

Copy link
Copy Markdown
Collaborator

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

mmarchini added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 27, 2019

mmarchini commented Jan 2, 2020

Copy link
Copy Markdown
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
deps: V8: cherry-pick cc5016e1b702
2badc61 
Original commit message:

    [postmortem] add metadata for the new DescriptorArray layout

    R=yangguo@chromium.org

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

Refs: v8/v8@cc5016e
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).

    R=bmeurer@chromium.org, hpayer@chromium.org, verwaest@chromium.org, yangguo@chromium.org

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

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

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

Copy link
Copy Markdown
Collaborator

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

MylesBorins commented Jan 8, 2020

Copy link
Copy Markdown
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 force-pushed the v12.x-staging branch 2 times, most recently from 3900f4a to 32e5c39 Compare January 8, 2020 16:34
mmarchini mentioned this pull request Jan 13, 2020
Closed

mmarchini commented Jan 13, 2020

Copy link
Copy Markdown
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 commented Jan 14, 2020 via email

Copy link
Copy Markdown
Contributor

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

    [postmortem] add metadata for the new DescriptorArray layout

    R=yangguo@chromium.org

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

Refs: v8/v8@cc5016e
PR-URL: #30870
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Jan 14, 2020
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).

    R=bmeurer@chromium.org, hpayer@chromium.org, verwaest@chromium.org, yangguo@chromium.org

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

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

targos commented Jan 14, 2020

Copy link
Copy Markdown
Member

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

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

    [postmortem] add metadata for the new DescriptorArray layout

    R=yangguo@chromium.org

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

Refs: v8/v8@cc5016e
PR-URL: #30870
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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).

    R=bmeurer@chromium.org, hpayer@chromium.org, verwaest@chromium.org, yangguo@chromium.org

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

Refs: v8/v8@b38dfaf
PR-URL: #30870
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Revert "src: update v8abbr.h for V8 7.7"
29608be 
This reverts commit 59b4640.
PR-URL: #30870
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Revert "test: update postmortem metadata test for V8 7.7"
023a802 
This reverts commit f056d55.
PR-URL: #30870
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
targos targos left review comments
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL