search

nodejs / llnode

An lldb plugin for Node.js and V8, which enables inspection of JavaScript states for insights into Node.js processes and their core dumps.
Other
1.15k stars 100 forks source link

src: fixes for V8 6.9 and 7.0 #247

Closed mmarchini closed 5 years ago

mmarchini commented 5 years ago

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.

TODO

codecov-io commented 5 years ago

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% <ø> (ø) :arrow_up:
test/plugin/workqueue-test.js 88.88% <100%> (-11.12%) :arrow_down:
test/common.js 87% <0%> (-0.57%) :arrow_down:
test/plugin/scan-test.js 98.91% <0%> (-0.07%) :arrow_down:
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 commented 5 years ago

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

mmarchini commented 5 years ago

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 commented 5 years ago

This is ready for review :)

cc @nodejs/llnode

mmarchini commented 5 years ago

CI failures are coming from recent changes in core (bisected and found https://github.com/nodejs/node/pull/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.

mmarchini commented 5 years ago

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 commented 5 years ago

Landed in 99d06e7...13f7034 :smile: