Skip to content

Commit

Permalink
src: fix function name lookup for inferred names
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@mmarchini
mmarchini committed Jan 14, 2020
1 parent 82d28c5 commit 8068cda
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 80 deletions.
13 changes: 11 additions & 2 deletions src/llv8-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -915,18 +915,27 @@ inline HeapObject ScopeInfo::MaybeFunctionName(Error& err) {
// metadata to determine in which slot its being stored for the present
// ScopeInfo, we try to find it heuristically.
int tries = 3;
HeapObject likely_function_name;
while (tries > 0 && proper_index < Length(err).GetValue()) {
err = Error();

HeapObject maybe_function_name =
FixedArray::Get<HeapObject>(proper_index, err);
if (err.Success() && String::IsString(v8(), maybe_function_name, err))
return maybe_function_name;
if (err.Success() && String::IsString(v8(), maybe_function_name, err)) {
likely_function_name = maybe_function_name;
if (*String(likely_function_name).Length(err) > 0) {
return likely_function_name;
}
}

tries--;
proper_index++;
}

if (likely_function_name.Check()) {
return likely_function_name;
}

err = Error::Failure("Couldn't get FunctionName from ScopeInfo");
return HeapObject();
}
Expand Down
24 changes: 20 additions & 4 deletions test/fixtures/frame-scenario.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,29 @@ const common = require('../common');

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

function eyecatcher() {
// Force V8 to use an inferred name instead of saving the variable name as
// FunctionName.
let fnInferredName;
fnInferredName = (() => function () {
crasher(); // # args < # formal parameters inserts an adaptor frame.
})();

function Module() {
this.foo = "bar";
}

Module.prototype.fnInferredNamePrototype = function() {
fnInferredName();
}

function fnFunctionName() {
(new Module()).fnInferredNamePrototype();
return this; // Force definition of |this|.
}

eyecatcher();
fnFunctionName();
174 changes: 100 additions & 74 deletions test/plugin/frame-test.js
View file
Original file line number Diff line number Diff line change
@@ -1,111 +1,137 @@
'use strict';

const { promisify } = require('util');

const tape = require('tape');

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

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

function fatalError(t, sess, err) {
t.error(err);
sess.quit();
return t.end();
}

function testFrameList(t, sess, frameNumber) {
async function testFrameList(t, sess, frameNumber, sourceCode, cb) {
const lastLine = new RegExp(sourceCode[sourceCode.length - 1]);

sess.send(`frame select ${frameNumber}`);
sess.linesUntil(/frame/, (err, lines) => {
if (err) {
return fatalError(t, sess, err);
}
sess.send('v8 source list');

sess.linesUntil(/v8 source list/, (err, lines) => {
sess.linesUntil(lastLine, (err, lines) => {
if (err) {
return fatalError(t, sess, err);
}
t.equal(lines.length, sourceCode.length,
`v8 source list correct size`);
for (let i = 0; i < lines.length; i++) {
t.equal(lines[i].trim(), sourceCode[i], `v8 source list #${i}`);
}

sess.send('v8 source list -l 2');

sess.linesUntil(/v8 source list/, (err, lines) => {
sess.linesUntil(lastLine, (err, lines) => {
if (err) {
return fatalError(t, sess, err);
}
t.equal(lines.length, sourceCode.length - 1,
`v8 source list -l 2 correct size`);
for (let i = 0; i < lines.length; i++) {
t.equal(lines[i].trim(), sourceCode[i + 1],
`v8 source list -l 2 #${i}`);
}

sess.quit();
t.end();
});
});
});
});
});
await sess.linesUntil(/frame/);
sess.send('v8 source list');
await sess.linesUntil(/v8 source list/);

let lines = await sess.linesUntil(lastLine);
t.equal(lines.length, sourceCode.length,
`v8 source list correct size`);
for (let i = 0; i < lines.length; i++) {
t.equal(lines[i].trim(), sourceCode[i], `v8 source list #${i}`);
}

sess.send('v8 source list -l 2');
await sess.linesUntil(/v8 source list/);
lines = await sess.linesUntil(lastLine);

t.equal(lines.length, sourceCode.length - 1,
`v8 source list -l 2 correct size`);
for (let i = 0; i < lines.length; i++) {
t.equal(lines[i].trim(), sourceCode[i + 1],
`v8 source list -l 2 #${i}`);
}
}

tape('v8 stack', (t) => {
tape('v8 stack', async (t) => {
t.timeoutAfter(15000);

const sess = common.Session.create('frame-scenario.js');
sess.waitBreak((err) => {
t.error(err);
sess.waitBreak = promisify(sess.waitBreak);
sess.linesUntil = promisify(sess.linesUntil);

try {
await sess.waitBreak();
sess.send('v8 bt');
});

sess.linesUntil(/eyecatcher/, (err, lines) => {
t.error(err);
let lines = await sess.linesUntil(/\sfnFunctionName\(/);

lines.reverse();
t.ok(lines.length > 4, 'frame count');
// FIXME(bnoordhuis) This can fail with versions of lldb that don't
// support the GetMemoryRegions() API; llnode won't be able to identify
// V8 builtins stack frames, it just prints them as anonymous frames.

lines = lines.filter((s) => !/<builtin>|<stub>/.test(s));
const eyecatcher = lines[0];
const adapter = lines[1];
const crasher = lines[2];
const exit = lines[3];
t.ok(/eyecatcher/.test(eyecatcher), 'eyecatcher frame');
t.ok(/<adaptor>/.test(adapter), 'arguments adapter frame');
const exit = lines[5];
const crasher = lines[4];
const adapter = lines[3];
const fnInferredName = lines[2];
const fnInferredNamePrototype = lines[1];
const fnFunctionName = lines[0];
t.ok(/<exit>/.test(exit), 'exit frame');
t.ok(/crasher/.test(crasher), 'crasher frame');
{
// V8 4.5 does not use EXIT frames, only INTERNAL frames.
const isv4 = /^v4\./.test(process.version);
const re = isv4 ? /<internal code>/ : /<exit>/;
t.ok(re.test(exit), 'exit frame');
}
// eyecatcher() is a sloppy mode function that should have an implicit
t.ok(/<adaptor>/.test(adapter), 'arguments adapter frame');
t.ok(/\sfnInferredName\(/.test(fnInferredName), 'fnInferredName frame');
t.ok(/\sModule.fnInferredNamePrototype\(/.test(fnInferredNamePrototype),
'fnInferredNamePrototype frame');
t.ok(/\sfnFunctionName\(/.test(fnFunctionName), 'fnFunctionName frame');
// fn() is a sloppy mode function that should have an implicit
// |this| that is the global object. crasher() is a strict mode function
// that should have a |this| that is the |undefined| value.
//
// Interestingly, V8 4.5 has a quirk where the |this| value is |undefined|
// in both strict and sloppy mode unless the function actually uses |this|.
// The test adds unreachable `return this` statements as a workaround.
t.ok(/this=(0x[0-9a-f]+):<Global proxy>/.test(eyecatcher), 'global this');
t.ok(/this=(0x[0-9a-f]+):<Global proxy>/.test(fnFunctionName),
'global this');
t.ok(/this=(0x[0-9a-f]+):<undefined>/.test(crasher), 'undefined this');

const eyecatcherFrame = eyecatcher.match(/frame #([0-9]+)/)[1];
if (!eyecatcherFrame) {
fatalError(t, sess, "Couldn't determine eyecather's frame number");
// TODO(mmarchini): also test positional info (line, column)

const fnFunctionNameFrame = fnFunctionName.match(/frame #([0-9]+)/)[1];
if (fnFunctionNameFrame) {
await testFrameList(t, sess, fnFunctionNameFrame,
sourceCodes['fnFunctionName']);
} else {
fatalError(t, sess, "Couldn't determine fnFunctionName's frame number");
}

const fnInferredNamePrototypeFrame =
fnInferredNamePrototype.match(/frame #([0-9]+)/)[1];
if (fnInferredNamePrototypeFrame) {
await testFrameList(t, sess, fnInferredNamePrototypeFrame,
sourceCodes['fnInferredNamePrototype']);
} else {
fatalError(t, sess,
"Couldn't determine fnInferredNamePrototype's frame number");
}

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

});
sess.quit();
return t.end();
} catch (err) {
fatalError(t, sess, err);
}
});

0 comments on commit 8068cda

Please sign in to comment.