src: fix function name lookup for inferred names #311
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
I wonder if the failing tests are happening because I changed the test to use Primises... llnode is printing |
mmarchini
force-pushed
the
fix-inferred-name-fn-lookup
branch
from
fb87eec to
c547181
Compare
mmarchini
commented
Oct 3, 2019
| let fnInferredName; | ||
| fnInferredName = (() => function () { | ||
| crasher(); // # args < # formal parameters inserts an adaptor frame. | ||
| })(); |
Contributor
Author
There was a problem hiding this comment.
V8 is not inferring this on Node.js v12... We'll probably need to skip the check for this function on 12.
Upstream bug: https://bugs.chromium.org/p/v8/issues/detail?id=9807
mmarchini
added
author ready
Contributor
Author
|
cc @nodejs/llnode |
If a test case would use Promises for linesUntil, sometimes we would emit lines events while there was no listener attached to it. This commit changes SessionOutput to make it a little more resilient: now line events will only be emitted when the SessionOutput is waiting. When wait is called (via linesUntil, for example), it'll retrieve all lines (via line events) on the buffer until it reaches a line matching the regexp. If no lines match the regexp, we'll continue as before waiting for data from lldb and buffering it.
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
force-pushed
the
fix-inferred-name-fn-lookup
branch
from
c547181 to
67e6e32
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Contributor
Author
|
cc @nodejs/llnode |
Contributor
Author
|
Ping @nodejs/llnode, we need this for v12 support. |
cjihrig
approved these changes
Jan 14, 2020
mmarchini
added a commit
that referenced
this pull request
Jan 14, 2020
If a test case would use Promises for linesUntil, sometimes we would emit lines events while there was no listener attached to it. This commit changes SessionOutput to make it a little more resilient: now line events will only be emitted when the SessionOutput is waiting. When wait is called (via linesUntil, for example), it'll retrieve all lines (via line events) on the buffer until it reaches a line matching the regexp. If no lines match the regexp, we'll continue as before waiting for data from lldb and buffering it. PR-URL: #311 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
mmarchini
added a commit
that referenced
this pull request
Jan 14, 2020
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. PR-URL: #311 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Contributor
Author
|
Landed in fd9d2b0...8068cda |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.