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.

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

Upcoming metadata changes #158

Closed
cjihrig opened this issue Dec 19, 2017 · 3 comments
Closed

Upcoming metadata changes #158

cjihrig opened this issue Dec 19, 2017 · 3 comments

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Dec 19, 2017

The following changes are coming:

  • v8dbg_class_Map__inobject_properties_or_constructor_function_index__int
    • This is now v8dbg_class_Map__inobject_properties_start_or_constructor_function_index__char as of v8/v8@61bf2cc
  • v8dbg_class_Map__instance_attributes__int
  • v8dbg_class_Map__instance_size__int
    • This is now v8dbg_class_Map__instance_size_in_words__char as of v8/v8@61bf2cc
  • v8dbg_bit_field3_dictionary_map_shift
    • This is now v8dbg_bit_field3_is_dictionary_map_shift as of v8/v8@7a159da

See nodejs/node-v8#34

@mmarchini
Copy link
Contributor

These metadata changes landed with nodejs/node#17489, thus llnode is not working with node compiled from source. There's also one more missing: v8dbg_prop_type_mask.

@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 6, 2018

There's also one more missing: v8dbg_prop_type_mask.

That one is expected. It's used here for Node.js version detection.

Drieger added a commit to Drieger/llnode that referenced this issue Apr 16, 2018
Drieger added a commit to Drieger/llnode that referenced this issue Apr 17, 2018
Drieger added a commit to Drieger/llnode that referenced this issue Apr 19, 2018
joyeecheung pushed a commit that referenced this issue Apr 22, 2018
@mmarchini
Copy link
Contributor

I think we should reopen this Issue, we updated the metadata names, but we didn't fix semantic changes. npm test have six failing tests on Node.js v10.0.0.

I started to investigate them, and I think at least three failures are happening because of v8/v8@61bf2cc. If I understood the patch correctly, it moved the internal properties of Map, confusing llnode once it tries to inspect objects. For example, in our inspect-test, Class.x should be 1 and Class.hashmap should be an object, but instead, they both are pointing to the internal Map:

(lldb) v8 findjsinstances -d Class
0x000030c00446cd11:<Object: Class properties {
    .x=0x000030c077382341:<Map own_descriptors=0 in_object=0 instance_size=8 descriptors=0x000030c0b6e02231>,
    .y=0.000000,
    .hashmap=0x000030c077382341:<Map own_descriptors=0 in_object=0 instance_size=8 descriptors=0x000030c0b6e02231>}
  internal fields {
    0x0000000100000000,
      0x405edd2f1a9fbe77,
      0x000030c00446ce09,
      0x000030c077382341,
      0x000030c077382341,
      0x000030c077382341,
      0x000030c077382341,
      0x000030c077382341}>

FWIW all failures from npm test can be reproduced with v10.0.0-v8-canary20171123e31a70aef2 from https://nodejs.org/download/v8-canary/, which means changes made on v8/v8@7a159da should already be fixed in llnode 😄

@joyeecheung joyeecheung reopened this May 2, 2018
mmarchini pushed a commit to mmarchini/llnode that referenced this issue May 3, 2018
V8 6.4 "replaces the in-object properties count byte in the map
with the byte that stores the start offset of in-object properties".
Object inspection on llnode relies on in-object properties being count
bytes, so these are minimal changes to make object inspection work again
with V8 6.4 while keeping compatibility with previous versions.

Ref: https://chromium-review.googlesource.com/c/v8/v8/+/776720
Fixes: nodejs#158
mmarchini pushed a commit that referenced this issue May 15, 2018
V8 6.4 "replaces the in-object properties count byte in the map
with the byte that stores the start offset of in-object properties".
Object inspection on llnode relies on in-object properties being count
bytes, so these are minimal changes to make object inspection work again
with V8 6.4 while keeping compatibility with previous versions.

Ref: https://chromium-review.googlesource.com/c/v8/v8/+/776720
Fixes: #158
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants