llnode icon indicating copy to clipboard operation
llnode copied to clipboard

Upcoming metadata changes in V8 7.7

Open cjihrig opened this issue 6 years ago • 4 comments

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: https://github.com/nodejs/node/pull/28918

cjihrig avatar Aug 07 '19 17:08 cjihrig

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 avatar Oct 08 '19 21:10 mmarchini

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

mmarchini avatar Oct 08 '19 22:10 mmarchini

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 avatar Oct 08 '19 23:10 mmarchini

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 :)

mmarchini avatar Oct 08 '19 23:10 mmarchini

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

No9 avatar Sep 17 '22 15:09 No9

There used to be a test in core that verified the existence of the metadata used by llnode. However, that test was removed in https://github.com/nodejs/node/commit/9a0aaa610706bfe7aeb2234b085cdcb912403c90. 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.

cjihrig avatar Sep 17 '22 15:09 cjihrig

Cheers @cjihrig - I'll close this and captured your recommendation here https://github.com/nodejs/llnode/issues/412

No9 avatar Sep 17 '22 15:09 No9