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: fixes for V8 6.9 and 7.0 #247

Closed
wants to merge 2 commits into from
Closed

Conversation

mmarchini
Copy link
Contributor

@mmarchini mmarchini commented Nov 6, 2018

Several fixes for V8 6.9 (#202, #226) and 7.0 (#232). Still a work in progress though, I'm sending a PR to run CI on these changes.

  • PositionInfo removed from SFI and PreParsedData (v8/v8@39e2d97)
    • PositionInfo is now available on ScopeInfo or UncompiledData
  • DebugInfo was removed from SFI (v8/v8@c51bcd1)
    • InferredName is available on ScopeInfo or UncompiledData (v8/v8@c941f11)
  • StackLocal removed from ScopeInfo (v8/v8@467eb14)
  • ParamCount (from ScopeInfo) size decreased (v8/v8@53d4dfc)

TODO

  • Improve commit message
  • Check if there's no stalled code in the PR
  • Update Travis CI to test Node.js v11.x

@codecov-io
Copy link

codecov-io commented Feb 22, 2019

Codecov Report

Merging #247 into master will increase coverage by 11.58%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #247       +/-   ##
===========================================
+ Coverage   80.84%   92.43%   +11.58%     
===========================================
  Files          33        8       -25     
  Lines        4120      476     -3644     
===========================================
- Hits         3331      440     -2891     
+ Misses        789       36      -753
Impacted Files Coverage Δ
test/addon/jsapi-test.js 97.05% <ø> (ø) ⬆️
test/plugin/workqueue-test.js 88.88% <100%> (-11.12%) ⬇️
test/common.js 87% <0%> (-0.57%) ⬇️
test/plugin/scan-test.js 98.91% <0%> (-0.07%) ⬇️
src/llnode_module.cc
src/printer.cc
src/constants.cc
src/llnode_api.h
src/addon.cc
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74414fc...e708c92. Read the comment docs.

@mmarchini
Copy link
Contributor Author

Timer check was removed from workqueue test because of nodejs/node#25806. The test was a duplicate of the previous test anyway, so this shouldn't decrease coverage.

@mmarchini mmarchini changed the title WIP src: fixes for V8 6.9 and 7.0 src: fixes for V8 6.9 and 7.0 Feb 22, 2019
@mmarchini
Copy link
Contributor Author

While working on this I noticed we use Error::Failures a lot when trying to decode an object from the heap. The problem is those Error::Failures propagate and usually abort our command.

For example: llnode was unable to output the content of process objects in Node.js 11 because it couldn't get the script and source position for some methods in the object, but it could get everything else. llnode didn't give any output to the user because it couldn't retrieve a small piece of information.

IMO we should change the code to be more tolerant when it can't decode only a piece of information. Most users of llnode use it to analyze core dumps, which may have corrupted memory addresses, and being fault-tolerant is essential to analyze such core dumps.

We should move most occurrences of Error::Failure to Error::PrintInDebugMode and save Error::Failures for unrecoverable errors.

This would also allow llnode to work better for future Node.js versions, with fewer features affected by V8 changes.

@mmarchini
Copy link
Contributor Author

This is ready for review :)

cc @nodejs/llnode

@mmarchini mmarchini added the author ready PRs that have at least one approvals, no pending requests for changes, and a CI started. label Feb 22, 2019
@mmarchini
Copy link
Contributor Author

CI failures are coming from recent changes in core (bisected and found nodejs/node#26382 to be the reason). Since apparently we were relying on a type indirectly defined internally on core ((Object), which is an object with a non-js-function constructor according to llnode code), I dropped this type from our tests. Remaining basic types should still be enough for testing.

- PositionInfo removed from SFI and PreParsedData ((v8/v8@39e2d97)[])
  - PositionInfo is now available on ScopeInfo or UncompiledData
- DebugInfo was removed from SFI ((v8/v8@c51bcd1)[])
  - InferredName is available on ScopeInfo or UncompiledData ((v8/v8@c941f11)[])
- StackLocal removed from ScopeInfo ((v8/v8@467eb14)[])
- ParamCount (from ScopeInfo) size decreased ((v8/v8@53d4dfc)[])

[v8/v8@39e2d97]: v8/v8@39e2d97
[v8/v8@c51bcd1]: v8/v8@c51bcd1
[v8/v8@c941f11]: v8/v8@c941f11
[v8/v8@467eb14]: v8/v8@467eb14
[v8/v8@53d4dfc]: v8/v8@53d4dfc
@mmarchini
Copy link
Contributor Author

CI is green. I'll start working on fixes for Node.js v12 (V8 7.1 to 7.6) in the next few days in an attempt to have llnode working on 12 soon after it goes LTS. But it would be nice to merge this first. cc @nodejs/llnode @nodejs/diagnostics

mmarchini added a commit that referenced this pull request Sep 12, 2019
- PositionInfo removed from SFI and PreParsedData ((v8/v8@39e2d97)[])
  - PositionInfo is now available on ScopeInfo or UncompiledData
- DebugInfo was removed from SFI ((v8/v8@c51bcd1)[])
  - InferredName is available on ScopeInfo or UncompiledData
    ((v8/v8@c941f11)[])
- StackLocal removed from ScopeInfo ((v8/v8@467eb14)[])
- ParamCount (from ScopeInfo) size decreased ((v8/v8@53d4dfc)[])

[v8/v8@39e2d97]: v8/v8@39e2d97
[v8/v8@c51bcd1]: v8/v8@c51bcd1
[v8/v8@c941f11]: v8/v8@c941f11
[v8/v8@467eb14]: v8/v8@467eb14
[v8/v8@53d4dfc]: v8/v8@53d4dfc

PR-URL: #247
Reviewed-By: Colin Ihrig <[email protected]>
mmarchini added a commit that referenced this pull request Sep 12, 2019
PR-URL: #247
Reviewed-By: Colin Ihrig <[email protected]>
@mmarchini
Copy link
Contributor Author

Landed in 99d06e7...13f7034 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approvals, no pending requests for changes, and a CI started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants