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: fix function name lookup for inferred names #311

Closed mmarchini closed 4 years ago

mmarchini commented 5 years ago

Apparently, sometimes the FunctionName slot on ScopeInfo is filled with the empty string instead of not existing. This commit changes our heuristic to search for the first non-empty string on the first 3 slots after the last context info slot on the ScopeInfo. This should be enough to cover most (all?) cases.

Also updated frame-test to add frames to the stack which V8 will infer the name instead of storing it directly, and changed this particular test to use Promises instead of callbacks. We should be able to upgrade tests to primise-based API gradually with this approach. When all tests are promisified, we can change the api on test/common.js to be promise-based instead of callback-based.

mmarchini commented 5 years ago

I wonder if the failing tests are happening because I changed the test to use Primises... llnode is printing v8 source list and v8 source list -l 2 correctly, but the test suite is not receiving it sometimes...

mmarchini commented 4 years ago

cc @nodejs/llnode

codecov-io commented 4 years ago

Codecov Report

Merging #311 into master will increase coverage by 0.66%. The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
+ Coverage   78.29%   78.96%   +0.66%     
==========================================
  Files          33       33              
  Lines        4262     4278      +16     
==========================================
+ Hits         3337     3378      +41     
+ Misses        925      900      -25
Impacted Files Coverage Δ
src/llv8-inl.h 93.18% <100%> (+1.3%) :arrow_up:
test/common.js 86.26% <100%> (-0.75%) :arrow_down:
test/plugin/frame-test.js 88.88% <91.83%> (+1.38%) :arrow_up:
src/llv8-constants.cc 83.01% <0%> (+0.96%) :arrow_up:
src/llv8-constants.h 100% <0%> (+1.51%) :arrow_up:
src/llv8.h 83.33% <0%> (+2.38%) :arrow_up:
src/llv8.cc 73.05% <0%> (+2.43%) :arrow_up:

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 37b3c26...67e6e32. Read the comment docs.

mmarchini commented 4 years ago

cc @nodejs/llnode

mmarchini commented 4 years ago

Ping @nodejs/llnode, we need this for v12 support.

mmarchini commented 4 years ago

Landed in fd9d2b099093...8068cdad28f4