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

src,test: support V8 6.1 postmortem data #130

Merged
merged 2 commits into from
Sep 8, 2017
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Sep 1, 2017

This is still a work in progress, but adds postmortem debugging support for V8 6.1. The following issues still need to be resolved (all marked with TODO in the code):

  • Properly handle the end offset of function bodies. It previously needed to be shifted, but that is no longer the case. This just needs version detection logic.
  • Properly load the ThinStringTag constant. V8DBG_THINSTRINGTAG appears to be defined, but the constant isn't loading. Right now, the value of 0x5 is hard coded.
  • Support Uint8Arrays. Those tests are currently broken and commented out. The problem is that the backing store of the JSArrayBufferView's JSArrayBuffer is coming back as all zeros.

UPDATE: The issue with the Uint8Arrays is that the backing stores are not materialized. If the test is updated to reference the .buffer property, the backing store is materialized and shows up properly in llnode.

indutny

This comment was marked as off-topic.

indutny

This comment was marked as off-topic.

@indutny
Copy link
Member

indutny commented Sep 1, 2017

Yikes, work in progress. Sorry for premature review!

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 1, 2017

No worries, but yes, your two comments correspond to a couple of the remaining problems :-)

@cjihrig cjihrig mentioned this pull request Sep 2, 2017
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 4, 2017

@indutny thanks for the help on IRC. I was able to get all of the tests passing on V8 6.1. Apparently, the backing stores of typed arrays are not necessarily materialized like they were in older versions of V8. If you update the inspect-scenario to simply access c.hashmap['uint8-array'].buffer, then it is materialized and works properly in llnode.

Currently, I'm reporting unmaterialized backing stores as being unmaterialized. I think I can materialize the backing store in llnode, but it will take more work.

@cjihrig cjihrig force-pushed the turbofan branch 2 times, most recently from 125f7ec to 8a8e0f1 Compare September 5, 2017 00:18
@cjihrig cjihrig changed the title WIP: src,test: support V8 6.1 postmortem data src,test: support V8 6.1 postmortem data Sep 5, 2017
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 5, 2017

This is ready for review.

I've done regression testing locally, and all tests are passing with Node 4.8.4 (latest Argon), 6.11.2 (latest Boron), 8.2.1 (latest Node 8 with Crankshaft).

I also tested the V8 6.1 branch, with these changes to the postmortem metadata.

bnoordhuis

This comment was marked as off-topic.

@indutny
Copy link
Member

indutny commented Sep 5, 2017

make format is your friend. Will take a deeper look after that 😉 Thanks for the changes!

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 5, 2017

@indutny I ran make format. Sorry I missed that. It might be good to incorporate that as part of npm test.

@cjihrig cjihrig mentioned this pull request Sep 6, 2017
5 tasks
@hhellyer
Copy link
Contributor

hhellyer commented Sep 8, 2017

@cjihrig - Would you like me to merge this now or wait until 6.1 is landed in Node.js? (We've a few approved PR's on llnode atm. They do seem to merge cleanly when I test locally.)

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 8, 2017

@hhellyer it should be fine to land this before 6.1 lands in core.

@hhellyer hhellyer merged commit c14adb4 into nodejs:master Sep 8, 2017
@cjihrig cjihrig deleted the turbofan branch September 8, 2017 15:18
addaleax pushed a commit to nodejs/node that referenced this pull request Sep 13, 2017
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
PR-URL: #14730
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos added a commit to targos/node that referenced this pull request Sep 14, 2017
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
kisg pushed a commit to paul99/v8mips that referenced this pull request Sep 15, 2017
See: nodejs/llnode#130
Change-Id: Ibce294f7620cd6ab0db4408a8c2b457c3a5aebcd
Reviewed-on: https://chromium-review.googlesource.com/650746
Commit-Queue: Yang Guo <[email protected]>
Reviewed-by: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#48021}
targos added a commit to targos/node that referenced this pull request Sep 21, 2017
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Sep 28, 2017
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Sep 29, 2017
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Oct 3, 2017
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
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.

4 participants