GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [v12.x] deps: V8: backport postmortem metadata fixes by mmarchini · Pull Request #30870 · nodejs/node

Conversation

@mmarchini

@mmarchini 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. post-mortem Issues and PRs related to Node.js postmortem diagnostics. 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

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Dec 9, 2019

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos
targos reviewed Dec 10, 2019
View reviewed changes
Comment thread deps/v8/BUILD.gn Outdated

@targos targos Dec 10, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mmarchini mmarchini Dec 10, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@targos targos Dec 10, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Dec 12, 2019

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 12, 2019

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mmarchini

mmarchini commented Dec 12, 2019

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins
MylesBorins force-pushed the v12.x-staging branch from ac6ff77 to f3c3b1d Compare December 17, 2019 22:07
@mmarchini
mmarchini force-pushed the v12.x-backport-postmortem-metadata branch from 0c80610 to 1730fc1 Compare December 27, 2019 18:56
@nodejs-github-bot

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

Uh oh!

There was an error while loading. Please reload this page.

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mmarchini

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 27, 2019

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mmarchini mmarchini added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 27, 2019
@BethGriggs
BethGriggs force-pushed the v12.x-staging branch from d96c765 to b08601b Compare December 31, 2019 01:00
@mmarchini

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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@{nodejs#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@{nodejs#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

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

Uh oh!

There was an error while loading. Please reload this page.

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins

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
mmarchini reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos
targos force-pushed the v12.x-staging branch 2 times, most recently from 3900f4a to 32e5c39 Compare January 8, 2020 16:34
@MylesBorins
MylesBorins force-pushed the v12.x-staging branch from 10b7951 to 0a4cfef Compare January 12, 2020 09:38
@mmarchini mmarchini mentioned this pull request Jan 13, 2020
llnode don't support nodejs v12 nodejs/llnode#328
Closed
@mmarchini

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins

MylesBorins commented Jan 14, 2020 via email

Copy link
Copy Markdown
Contributor
Go for it!
…
On Mon, Jan 13, 2020, 2:01 PM Matheus Marchini ***@***.***> wrote: @MylesBorins <https://github.com/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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30870?email_source=notifications&email_token=AADZYV4LMPAIWKWWHEKUD7DQ5S3ABA5CNFSM4JYTT2Q2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIZ4J7Y#issuecomment-573818111>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZYV4VNBL5LLP2QZMVBWDQ5S3ABANCNFSM4JYTT2QQ> .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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

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!
mmarchini reacted with hooray emoji mmarchini reacted with heart emoji
All reactions
  • 🎉 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos closed this Jan 14, 2020
@targos targos mentioned this pull request Jan 15, 2020
v12.15.0 release proposal #31368
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
v12.16.0 proposal #31691
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure. post-mortem Issues and PRs related to Node.js postmortem diagnostics. 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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@mmarchini @nodejs-github-bot @MylesBorins @targos @mhdawson
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.