Skip to content

Commit 8068cda

Browse files
mmarchinimmarchini
committed
src: fix function name lookup for inferred names
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>
1 parent 82d28c5 commit 8068cda

File tree

3 files changed

+131
-80
lines changed

3 files changed

+131
-80
lines changed

‎src/llv8-inl.h‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -915,18 +915,27 @@ inline HeapObject ScopeInfo::MaybeFunctionName(Error& err) {
915915
// metadata to determine in which slot its being stored for the present
916916
// ScopeInfo, we try to find it heuristically.
917917
int tries = 3;
918+
HeapObject likely_function_name;
918919
while (tries > 0 && proper_index < Length(err).GetValue()) {
919920
err = Error();
920921

921922
HeapObject maybe_function_name =
922923
FixedArray::Get<HeapObject>(proper_index, err);
923-
if (err.Success() && String::IsString(v8(), maybe_function_name, err))
924-
return maybe_function_name;
924+
if (err.Success() && String::IsString(v8(), maybe_function_name, err)) {
925+
likely_function_name = maybe_function_name;
926+
if (*String(likely_function_name).Length(err) > 0) {
927+
return likely_function_name;
928+
}
929+
}
925930

926931
tries--;
927932
proper_index++;
928933
}
929934

935+
if (likely_function_name.Check()) {
936+
return likely_function_name;
937+
}
938+
930939
err = Error::Failure("Couldn't get FunctionName from ScopeInfo");
931940
return HeapObject();
932941
}

‎test/fixtures/frame-scenario.js‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,29 @@ const common = require('../common');
33

44
function crasher(unused) {
55
'use strict';
6-
process.abort(); // Creates an exit frame.
7-
return this; // Force definition of |this|.
6+
this.foo = arguments; // Force adaptor frame on Node.js v12+
7+
process.abort(); // Creates an exit frame.
8+
return this; // Force definition of |this|.
89
}
910

10-
function eyecatcher() {
11+
// Force V8 to use an inferred name instead of saving the variable name as
12+
// FunctionName.
13+
let fnInferredName;
14+
fnInferredName = (() => function () {
1115
crasher(); // # args < # formal parameters inserts an adaptor frame.
16+
})();
17+
18+
function Module() {
19+
this.foo = "bar";
20+
}
21+
22+
Module.prototype.fnInferredNamePrototype = function() {
23+
fnInferredName();
24+
}
25+
26+
function fnFunctionName() {
27+
(new Module()).fnInferredNamePrototype();
1228
return this; // Force definition of |this|.
1329
}
1430

15-
eyecatcher();
31+
fnFunctionName();

‎test/plugin/frame-test.js‎

Lines changed: 100 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -1,111 +1,137 @@
11
'use strict';
22

3+
const { promisify } = require('util');
4+
35
const tape = require('tape');
46

57
const common = require('../common');
68

7-
const sourceCode = [
8-
"10 function eyecatcher() {",
9-
"11 crasher(); // # args < # formal parameters inserts an adaptor frame.",
10-
"12 return this; // Force definition of |this|.",
11-
"13 }",
12-
];
13-
const lastLine = new RegExp(sourceCode[sourceCode.length - 1]);
9+
const sourceCodes = {
10+
"fnFunctionName": [
11+
"26 function fnFunctionName() {",
12+
"27 (new Module()).fnInferredNamePrototype();",
13+
"28 return this; // Force definition of |this|.",
14+
"29 }",
15+
],
16+
"fnInferredNamePrototype": [
17+
"22 Module.prototype.fnInferredNamePrototype = function() {",
18+
"23 fnInferredName();",
19+
"24 }",
20+
"25",
21+
],
22+
"fnInferredName": [
23+
"14 fnInferredName = (() => function () {",
24+
"15 crasher(); // # args < # formal parameters inserts an adaptor frame.",
25+
"16 })();",
26+
"17",
27+
],
28+
};
1429

1530
function fatalError(t, sess, err) {
1631
t.error(err);
1732
sess.quit();
1833
return t.end();
1934
}
2035

21-
function testFrameList(t, sess, frameNumber) {
36+
async function testFrameList(t, sess, frameNumber, sourceCode, cb) {
37+
const lastLine = new RegExp(sourceCode[sourceCode.length - 1]);
38+
2239
sess.send(`frame select ${frameNumber}`);
23-
sess.linesUntil(/frame/, (err, lines) => {
24-
if (err) {
25-
return fatalError(t, sess, err);
26-
}
27-
sess.send('v8 source list');
28-
29-
sess.linesUntil(/v8 source list/, (err, lines) => {
30-
sess.linesUntil(lastLine, (err, lines) => {
31-
if (err) {
32-
return fatalError(t, sess, err);
33-
}
34-
t.equal(lines.length, sourceCode.length,
35-
`v8 source list correct size`);
36-
for (let i = 0; i < lines.length; i++) {
37-
t.equal(lines[i].trim(), sourceCode[i], `v8 source list #${i}`);
38-
}
39-
40-
sess.send('v8 source list -l 2');
41-
42-
sess.linesUntil(/v8 source list/, (err, lines) => {
43-
sess.linesUntil(lastLine, (err, lines) => {
44-
if (err) {
45-
return fatalError(t, sess, err);
46-
}
47-
t.equal(lines.length, sourceCode.length - 1,
48-
`v8 source list -l 2 correct size`);
49-
for (let i = 0; i < lines.length; i++) {
50-
t.equal(lines[i].trim(), sourceCode[i + 1],
51-
`v8 source list -l 2 #${i}`);
52-
}
53-
54-
sess.quit();
55-
t.end();
56-
});
57-
});
58-
});
59-
});
60-
});
40+
await sess.linesUntil(/frame/);
41+
sess.send('v8 source list');
42+
await sess.linesUntil(/v8 source list/);
43+
44+
let lines = await sess.linesUntil(lastLine);
45+
t.equal(lines.length, sourceCode.length,
46+
`v8 source list correct size`);
47+
for (let i = 0; i < lines.length; i++) {
48+
t.equal(lines[i].trim(), sourceCode[i], `v8 source list #${i}`);
49+
}
50+
51+
sess.send('v8 source list -l 2');
52+
await sess.linesUntil(/v8 source list/);
53+
lines = await sess.linesUntil(lastLine);
54+
55+
t.equal(lines.length, sourceCode.length - 1,
56+
`v8 source list -l 2 correct size`);
57+
for (let i = 0; i < lines.length; i++) {
58+
t.equal(lines[i].trim(), sourceCode[i + 1],
59+
`v8 source list -l 2 #${i}`);
60+
}
6161
}
6262

63-
tape('v8 stack', (t) => {
63+
tape('v8 stack', async (t) => {
6464
t.timeoutAfter(15000);
6565

6666
const sess = common.Session.create('frame-scenario.js');
67-
sess.waitBreak((err) => {
68-
t.error(err);
67+
sess.waitBreak = promisify(sess.waitBreak);
68+
sess.linesUntil = promisify(sess.linesUntil);
69+
70+
try {
71+
await sess.waitBreak();
6972
sess.send('v8 bt');
70-
});
7173

72-
sess.linesUntil(/eyecatcher/, (err, lines) => {
73-
t.error(err);
74+
let lines = await sess.linesUntil(/\sfnFunctionName\(/);
75+
7476
lines.reverse();
7577
t.ok(lines.length > 4, 'frame count');
76-
// FIXME(bnoordhuis) This can fail with versions of lldb that don't
77-
// support the GetMemoryRegions() API; llnode won't be able to identify
78-
// V8 builtins stack frames, it just prints them as anonymous frames.
78+
7979
lines = lines.filter((s) => !/<builtin>|<stub>/.test(s));
80-
const eyecatcher = lines[0];
81-
const adapter = lines[1];
82-
const crasher = lines[2];
83-
const exit = lines[3];
84-
t.ok(/eyecatcher/.test(eyecatcher), 'eyecatcher frame');
85-
t.ok(/<adaptor>/.test(adapter), 'arguments adapter frame');
80+
const exit = lines[5];
81+
const crasher = lines[4];
82+
const adapter = lines[3];
83+
const fnInferredName = lines[2];
84+
const fnInferredNamePrototype = lines[1];
85+
const fnFunctionName = lines[0];
86+
t.ok(/<exit>/.test(exit), 'exit frame');
8687
t.ok(/crasher/.test(crasher), 'crasher frame');
87-
{
88-
// V8 4.5 does not use EXIT frames, only INTERNAL frames.
89-
const isv4 = /^v4\./.test(process.version);
90-
const re = isv4 ? /<internal code>/ : /<exit>/;
91-
t.ok(re.test(exit), 'exit frame');
92-
}
93-
// eyecatcher() is a sloppy mode function that should have an implicit
88+
t.ok(/<adaptor>/.test(adapter), 'arguments adapter frame');
89+
t.ok(/\sfnInferredName\(/.test(fnInferredName), 'fnInferredName frame');
90+
t.ok(/\sModule.fnInferredNamePrototype\(/.test(fnInferredNamePrototype),
91+
'fnInferredNamePrototype frame');
92+
t.ok(/\sfnFunctionName\(/.test(fnFunctionName), 'fnFunctionName frame');
93+
// fn() is a sloppy mode function that should have an implicit
9494
// |this| that is the global object. crasher() is a strict mode function
9595
// that should have a |this| that is the |undefined| value.
9696
//
9797
// Interestingly, V8 4.5 has a quirk where the |this| value is |undefined|
9898
// in both strict and sloppy mode unless the function actually uses |this|.
9999
// The test adds unreachable `return this` statements as a workaround.
100-
t.ok(/this=(0x[0-9a-f]+):<Global proxy>/.test(eyecatcher), 'global this');
100+
t.ok(/this=(0x[0-9a-f]+):<Global proxy>/.test(fnFunctionName),
101+
'global this');
101102
t.ok(/this=(0x[0-9a-f]+):<undefined>/.test(crasher), 'undefined this');
102103

103-
const eyecatcherFrame = eyecatcher.match(/frame #([0-9]+)/)[1];
104-
if (!eyecatcherFrame) {
105-
fatalError(t, sess, "Couldn't determine eyecather's frame number");
104+
// TODO(mmarchini): also test positional info (line, column)
105+
106+
const fnFunctionNameFrame = fnFunctionName.match(/frame #([0-9]+)/)[1];
107+
if (fnFunctionNameFrame) {
108+
await testFrameList(t, sess, fnFunctionNameFrame,
109+
sourceCodes['fnFunctionName']);
110+
} else {
111+
fatalError(t, sess, "Couldn't determine fnFunctionName's frame number");
112+
}
113+
114+
const fnInferredNamePrototypeFrame =
115+
fnInferredNamePrototype.match(/frame #([0-9]+)/)[1];
116+
if (fnInferredNamePrototypeFrame) {
117+
await testFrameList(t, sess, fnInferredNamePrototypeFrame,
118+
sourceCodes['fnInferredNamePrototype']);
119+
} else {
120+
fatalError(t, sess,
121+
"Couldn't determine fnInferredNamePrototype's frame number");
106122
}
107123

108-
testFrameList(t, sess, eyecatcherFrame);
124+
const fnInferredNameFrame = fnInferredName.match(/frame #([0-9]+)/)[1];
125+
if (fnInferredNameFrame) {
126+
await testFrameList(t, sess,
127+
fnInferredNameFrame, sourceCodes['fnInferredName']);
128+
} else {
129+
fatalError(t, sess, "Couldn't determine fnInferredName's frame number");
130+
}
109131

110-
});
132+
sess.quit();
133+
return t.end();
134+
} catch (err) {
135+
fatalError(t, sess, err);
136+
}
111137
});

0 commit comments

Comments
 (0)