joyent/mdb_v8#23 support V8 4.4.x and 4.5.x

[misterdjules]

This PR updates mdb_v8 to support V8 4.4.x and 4.5.x.

• It uses v8dbg_propindex_mask instead of v8dbg_fieldindex_mask, and v8dbg_propindex_shift instead of v8dbg_fieldindex_shift. The v8dbg_propindex* values were already present when the v8dbg_fieldindex values were added, and they represented the same values. So instead of going back to naming these values fieldindex_* and having to usptream these changes to V8, I chose to change what values we use in mdb_v8.c instead.
• It adds a V8_CONSTANT_ADDED_SINCE macro and a function named v8_version_at_least to mark and determine when a constant or an offset was added by a given V8 version. It's used to support the new OneByteStringTag constant, and Maps' constructor offset that has been renamed to constructor_or_backpointer.
• struct v8_offset now also has a flags member so that we can determine when Maps' constructor offset was renamed to constructor_or_backpointer using V8_CONSTANT_ADDED_SINCE.
• Finally tst.postmortem_details.js and tst.postmortem_findjsobjects.js were failing because of a race condition between the child process' exit event and the stdin's end event.

These changes have been tested with core files generated from 32 and 64 bits versions of nodejs/node master that use V8 4.4.x and 4.5.x, as well as with node v0.12.7 64 bits. All tests pass.

I will perform more testing, specifically with node v0.10.x 32bits and 64bits, and with node v0.12.7 32 bits. In the meantime, any feedback would be very much appreciated.

[davepacheco]

This looks great! My only issue is in src/v8dbg.h, lines 52-53 (the definition of V8_STRENC_ASCII()). My concern is that we're always checking both cases, even though we know which one it should be (based on whether V8_AsciiStringTag or V8_OneByteStringTag is uninitialized). The case I'm worried about is where a garbage object's tag accidentally matches whichever of these is uninitialized. I think it would be better to check whether the corresponding tag is actually initialized before using it, as we do at for V8_IS_FAILURE() at https://github.com/misterdjules/mdb_v8/blob/update-for-v8-4.5/src/v8dbg.h#L31-L33.

[misterdjules]

@davepacheco Absolutely, sorry for missing that in the first place. I updated this PR with a separate commit that hopefully does what you had in mind.

Please let me know what you think.

[misterdjules]

Tested these changes with node v0.10.40, v0.12.7, nodejs/node master with V8 4.4.x and 4.5.x, and both 32bits and 64bits for all these versions and all mdb_v8 tests passed. I also verified manually that the output from ::jsstack and ::findjsobjects was sane for all these versions.

[misterdjules]

Actually, I had run only the tests that used to be included in node when mdb_v8 was in deps/mdb_v8, so I had missed tst.jsclosure.js, which fails. So there's still some work to do. My apologies for the confusion.

I'll get to that on Tuesday, since Monday is a holiday.

[misterdjules]

@davepacheco The latest commit in this PR fixes ::v8scopeinfo, and thus tst.jsclosure.js. However, in its current form, it relies on constants being added to V8's postmortem metadata generation process, which is a change that has not been submitted upstream.

I'd like to get your feedback on these latest changes before moving forward. One of the things I'm not a big fan of is having a constant hardcoded in gen-postmortem-metadata.py. Should it be added in as a constant in src/objects.h instead?

[davepacheco]

The change looks good overall. But we can't wait for postmortem metadata to show up in the binary before support works. We've always provided fallback values to handle that situation. Can we not do that here?

As for the constant value: is it a constant in the V8 source? If so, I wouldn't bother adding metadata for it. Metadata is only useful to the extent that it will update automatically when V8 changes. If there's no way to do that, then we may as well hardcode it in mdb_v8 (with a comment explaining that). We could update the V8 change so that it is a constant...but I think that gets a little hairy if the V8 team isn't that excited about it.

Nitty notes on a few comments:

lines 389 - 403: this comment seems a little redundant with mdb_v8_dbg.h lines 86 - 94. My worry is that if someone updates one in the future, they'll end up forgetting to update the other one. Maybe consolidate these notes into one place, or add a reference from one to the other?

line 428: Should probably start with "If". Also, this sentence is a comma splice. I think you want a period before "we're done."

The comment at 435-441 has an extra line break.

[misterdjules]

But we can't wait for postmortem metadata to show up in the binary before support works. We've always provided fallback values to handle that situation. Can we not do that here?

If we consider the case of V8_SCOPEINFO_IDX_FIRST_VARS: it appeared in V8 3.7.x, and then changed in V8 4.5.x. So the fallback value would need to change depending on the version of V8. If I understand correctly, this is not something that is supported by the current V8_CONSTANT_FALLBACK macro. We could add more declarative, static info to the v8_constants array to represent these differences between V8 versions, or just have special cases for initializing this value in autoconfigure. Do you have any preference?

The same applies to V8_SCOPEINFO_OFFSET_STACK_LOCALS. It's not a constant in V8's source, so I agree with you that adding metadata for it doesn't seem to be the best way forward. However, we can't rely on the current V8_CONSTANT_FALLBACK macro because the offset may change in the future, and we need to handle different V8 versions. For now, another solution could be to test in v8scopeinfo_iter_vars if the first slot of the StackLocals ScopeInfo's data is not a string (or a SMI), and in this case skip that slot. Do you have any preference?

All other constants (V8_SCOPEINFO_IDX_NPARAMS, V8_SCOPEINFO_IDX_NSTACKLOCALS and V8_SCOPEINFO_IDX_NCONTEXTLOCALS) are already present as constants in V8's source so in my opinion it makes sense to add them to gen-postmortem-metadata.py.

Nitty notes on a few comments:

Thank you! I updated these comments and will push the changes shortly.

[misterdjules]

With the latest changes added to this PR, all tests pass for node (32 and 64 bits) v0.10, v0.12, nodejs/node master and nodejs/node master with metadata added.

V8_SCOPEINFO_OFFSET_STACK_LOCALS is defined as having the value of v8dbg_scopeinfo_offset_stack_locals, even though we know that symbol is not (and will not be in the near future) defined in V8. I thought it was a good compromise to leverage the V8_CONSTANT_FALLBACK macro and not have to hardcode things even more, but I'm open to other suggestions if you'd like to handle things differently.

[davepacheco]

LGTM!

[misterdjules]

@davepacheco Thank you! Squashed all commits into one. Let me know if you need anything else.