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

Upcoming metadata changes in V8 7.7 #287

Closed
cjihrig opened this issue Aug 7, 2019 · 7 comments
Closed

Upcoming metadata changes in V8 7.7 #287

cjihrig opened this issue Aug 7, 2019 · 7 comments
Labels
V8 Changes

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Aug 7, 2019

The V8 7.7 update requires the following adjustments to the postmortem debugging metadata constants:

  • v8dbg_class_ConsString__first__String

    • Class is now generated via torque.
    • Postmortem tools should use v8dbg_class_ConsString__first_offset__int
    • Refs: v8/v8@14274bb

  • v8dbg_class_ConsString__second__String

    • Class is now generated via torque.
    • Postmortem tools should use v8dbg_class_ConsString__second_offset__int
    • Refs: v8/v8@14274bb

  • v8dbg_class_SlicedString__offset__SMI

    • Class is now generated via torque.
    • Postmortem tools should use v8dbg_class_SlicedString__offset_offset__int
    • Refs: v8/v8@14274bb

  • v8dbg_class_ThinString__actual__String

    • Class is now generated via torque.
    • Postmortem tools should use v8dbg_class_ThinString__actual_offset__int
    • Refs: v8/v8@14274bb

Refs: nodejs/node#28918
@mmarchini
Copy link
Contributor

Interesting, the type of those fields didn't change, but they are all prefixed by __int. Is the metadata generated for Torque classes prefixed by the the metadata type instead of the field type? That's probably not a good idea, if a field type changes it wouldn't reflect on the metadata name, and we wouldn't have a way to check the type on llnode...

@mmarchini
Copy link
Contributor

class_Symbol__name__Object was removed as well, I'm guessing in this version. Still investigating.

@mmarchini
Copy link
Contributor

So I renamed the String metadata back to their previous names in: https://chromium-review.googlesource.com/c/v8/v8/+/1847783. I also added the missing metadata for symbols. Will update the tests once this is merged to core.

@mmarchini
Copy link
Contributor

If the above gets merged, no changes will be necessary on llnode for 7.7 (and then we can close this issue). I already have patches to fix 7.2, 7.4 and 7.6 (I haven't opened PRs with all of them yet because some are blocked by #303), so once the metadata lands upstream and is backported to core, we'll have llnode working on Node.js v12 :)

MylesBorins pushed a commit to mmarchini/node that referenced this issue Jan 8, 2020
@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
targos pushed a commit to nodejs/node that referenced this issue 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]>
BethGriggs pushed a commit to nodejs/node that referenced this issue 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]>
@No9
Copy link
Member

No9 commented Sep 17, 2022

@cjihrig We discussed the issue of breaking changes in a recent diagnostics meeting.
Is there a process around how these breaking changes are identified and flagged to llnode or do we just rely on the good will of folks like yourself?

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 17, 2022

There used to be a test in core that verified the existence of the metadata used by llnode. However, that test was removed in nodejs/node@9a0aaa6. After that, I think @mmarchini had her own way of detecting the breaking changes. You'll definitely want some automated way to detect the breaking changes because they happen frequently with V8 updates.

@No9 No9 mentioned this issue Sep 17, 2022
Open
@No9
Copy link
Member

No9 commented Sep 17, 2022

Cheers @cjihrig - I'll close this and captured your recommendation here #412

@No9 No9 closed this as completed Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
V8 Changes
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@No9 @cjihrig @mmarchini