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

joyent/mdb_v8#32: fix ::findjsobjects for V8 4.5.x #33

Merged

Conversation

misterdjules
Copy link
Contributor

This fixes ::findjsobjects for core dumps generated by Node.js 4.0.x
(and possibly later).

This change fixes how mdb_v8 retrieves the constructor of a JavaScript
object given its Map.

It also fixes the problem of accessing objects' properties that was
caused by a typo in "v8db_propindex_mask" that should have been
"v8db_prop_index_mask" (note the underscore in "prop_index") in my
previous changes.

It adds a "fallback" member for v8_offsets so that we can have a
fallback for typed arrays' length's offset.

Finally, this change allows ::findjsobjects to find Buffer instances and
inspect them. However, due to how Buffer instances are implemented in
node v4.x and later, they are currently seen by mdb_v8 as having a
constructor named "Uint8Array" instead of "Buffer", so ::findjsobjects
-c Buffer won't find any actual Buffer instances, instead one has to use
::findjsobjects -c Uint8Array.

@misterdjules
Copy link
Contributor Author

/cc @davepacheco

@misterdjules
Copy link
Contributor Author

Also, one thing I failed to mention in the commit message is that now Buffer instances don't have a property named length. Instead, the length is a generated property on typed arrays. The current changes print this generated property when we ::jsprint a Buffer instance (really a typed array) to make the tests pass without having to change them too much.

However, I'm still undecided on whether we would want to display it in the final set of changes.

@@ -706,6 +721,11 @@ autoconfigure(v8_cfg_t *cfgp)
failed++;
}

if (V8_TYPE_JSTYPEDARRAY == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it definitely a problem for there to be no JSTypedArray type? Even on older versions of Node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's OK for older versions of node not to have JSTypedArray. Will fix too.

@davepacheco
Copy link
Contributor

I've made a few inline comments. In general, the debugger should present the facts, however ugly. For that reason I prefer reporting the constructor as "Uint8Array", or maybe "Uint8Array (with Buffer prototype)". Thanks!

@misterdjules
Copy link
Contributor Author

@davepacheco Thanks for the review, I replied to your comments and I will update this PR shortly.

I'll do a bit more research on how to get the actual prototype set on Buffer instances, and I'll keep you posted.

@misterdjules
Copy link
Contributor Author

@davepacheco I believe I have addressed all your comments.

I still need to do more research around how Object.setPrototypeOf affects V8's internal data structures, but I believe the current state of this PR already significantly improves support for node v4.x [1], and would be worth considering for a new release (pending the second review, obviously).

[1] Or rather for the next release of node v4.x, as some metadata is missing in node v4.0 and v4.1. See #34 for more details.

@misterdjules
Copy link
Contributor Author

@davepacheco Regarding [1] above, I was mostly confused by the fact that properties of functions' prototypes get a constructor property by default which is the function itself. It all makes sense now.

Now, regarding:

In general, the debugger should present the facts, however ugly. For that reason I prefer reporting the constructor as "Uint8Array", or maybe "Uint8Array (with Buffer prototype)"

Even though I generally agree with the fact that the debugger should represent the facts, I still think it could be confusing for users who are used to use ::findjsobjects -c Buffer, and who don't necessarily know that Buffer instances are now typed arrays, to have to use ::findjsobjects -c Uint8Array.

The confusion might be made worse because var buf = new Buffer(); console.log(buf.constructor.name); outputs 'Buffer'.

Thinking out loud: one potential solution would be to add another parameter to ::findjsobjects, maybe "-C constructor_name" (capital c) to search for objects that have a constructor property whose name is constructor_name. Thus, we could use ::findjsobjects -C Buffer with all versions of node to find Buffer instances.

At least, we should probably document/communicate around this change.

Do you mind if I open a separate issue to discuss potential solutions for this specific use case?

@misterdjules
Copy link
Contributor Author

Squashed commits into a single one and updated commit message + original PR description to match the commit message.

@misterdjules
Copy link
Contributor Author

@davepacheco I've actually ran the full tests suite again against all v0.10.40, v0.12.7, v4.0.0 and v4.1.0 node versions, 32 and 64 bits and I added another commit that fixes what I believe is the last remaining issues.

If your second review is OK I'll squash these two commits into one.

@davepacheco
Copy link
Contributor

LGTM!

@misterdjules
Copy link
Contributor Author

@davepacheco Thank you! Squashed all commits into one, and made two minor changes:

  1. Updated comment to not mention node and only V8 here: https://github.com/joyent/mdb_v8/pull/33/files#diff-38a6513060f0dadc2b8c2ea5c48a57d9R1052. Also updated V8 version number to reflect where the proper metadata landed (4.7): https://github.com/joyent/mdb_v8/pull/33/files#diff-38a6513060f0dadc2b8c2ea5c48a57d9R1063.
  2. Use actual numbers to compare node version numbers: https://github.com/joyent/mdb_v8/pull/33/files#diff-1f7f4b61869c469eb7471735f5412183R123 and https://github.com/joyent/mdb_v8/pull/33/files#diff-1f7f4b61869c469eb7471735f5412183R197.

@misterdjules misterdjules mentioned this pull request Sep 25, 2015
@davepacheco davepacheco merged commit c4d540f into TritonDataCenter:master Oct 2, 2015
@davepacheco
Copy link
Contributor

Merged. Thanks! I'll wait for #37 and then issue a new release.

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

Successfully merging this pull request may close these issues.

2 participants