src: fix function name lookup for inferred names

mmarchini · mmarchini · commit 8068cdad28f4 · 2020-01-14T08:41:32.000-08:00
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>
diff --git a/src/llv8-inl.h b/src/llv8-inl.h
@@ -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();
 }
diff --git a/test/fixtures/frame-scenario.js b/test/fixtures/frame-scenario.js
@@ -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();
diff --git a/test/plugin/frame-test.js b/test/plugin/frame-test.js
@@ -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);
+  }
 });
