Make Function source locations start at the start of the function, no…

…t its body.

Summary:
IR Functions' source spans were being set to that of their body node,
rather than the function node itself. By fixing this, we also happen
to be able to use the PreParsed function span in lieu of debug
info when finding source locations for lazily compiled functions
in heap snapshots.

Reviewed By: dulinriley

Differential Revision: D19384569

fbshipit-source-id: 6e7e827d6d6cfb6188b432f92f8ed295250201b3

lib/IRGen/ESTreeIRGen-func.cpp

@@ -178,13 +178,13 @@ Function *ESTreeIRGen::genES5Function(
             originalName,
             Function::DefinitionKind::ES5Function,
             ESTree::isStrict(functionNode->strictness),
-            body->getSourceRange(),
+            functionNode->getSourceRange(),
             /* insertBefore */ nullptr)
       : Builder.createFunction(
             originalName,
             Function::DefinitionKind::ES5Function,
             ESTree::isStrict(functionNode->strictness),
-            body->getSourceRange(),
+            functionNode->getSourceRange(),
             /* isGlobal */ false,
             /* insertBefore */ nullptr);
 

test/BCGen/HBC/new-array-debug.js

@@ -24,5 +24,5 @@ function foo() {
 //CHECK-NEXT:[@ 28] Ret 0<Reg8>
 
 //CHECK-LABEL:Debug source table:
-//CHECK:  0x{{[0-9a-f]+}}  function idx 1, starts at line 11 col 16
+//CHECK:  0x{{[0-9a-f]+}}  function idx 1, starts at line 11 col 1
 //CHECK:  0x{{[0-9a-f]+}}  end of debug source table

test/BCGen/HBC/sourcepath.js

@@ -12,4 +12,4 @@ function func() {
 }
 
 //CHECK:   {{.*}}/sourcepath.js[10:1]
-//CHECK:   {{.*}}/sourcepath.js[10:17]
+//CHECK:   {{.*}}/sourcepath.js[10:1]

test/IRGen/source_loc.js

@@ -37,11 +37,11 @@ function foo(a,b) {
 
 //CHECK-LABEL: function foo(a, b)
 //CHECK-NEXT: frame = [a, b]
-//CHECK-NEXT: source location: [<stdin>:10:19 ... <stdin>:18:2)
+//CHECK-NEXT: source location: [<stdin>:10:1 ... <stdin>:18:2)
 //CHECK-NEXT: %BB0:
-//CHECK-NEXT: ; <stdin>:10:19
+//CHECK-NEXT: ; <stdin>:10:1
 //CHECK-NEXT:   %0 = StoreFrameInst %a, [a]
-//CHECK-NEXT: ; <stdin>:10:19
+//CHECK-NEXT: ; <stdin>:10:1
 //CHECK-NEXT:   %1 = StoreFrameInst %b, [b]
 //CHECK-NEXT: ; <stdin>:11:9
 //CHECK-NEXT:   %2 = LoadFrameInst [a]

test/debugger/forEach-step-out.js

@@ -28,13 +28,13 @@ print("finish");
 //CHECK: Stepped to anonymous: {{.*}}:18:7
 //CHECK: Stepped to anonymous: {{.*}}:18:3
 //CHECK: Stepped to anonymous: {{.*}}:21:3
-//CHECK: Stepped to anonymous: {{.*}}:17:23
+//CHECK: Stepped to anonymous: {{.*}}:17:11
 //CHECK: Stepped to anonymous: {{.*}}:18:7
 //CHECK: Stepped to anonymous: {{.*}}:19:5
 //CHECK: Stepped to funky: {{.*}}:12:3
 //CHECK: funky
 //CHECK: Stepped to anonymous: {{.*}}:21:3
-//CHECK: Stepped to anonymous: {{.*}}:17:23
+//CHECK: Stepped to anonymous: {{.*}}:17:11
 //CHECK: Stepped to anonymous: {{.*}}:18:7
 //CHECK: Stepped to global: {{.*}}:17:10
 //CHECK: Stepped to global: {{.*}}:23:1

test/debugger/step-in.js

@@ -49,8 +49,8 @@ x.forEach(function(i) {
 // CHECK-NEXT: Stepped to global: {{.*}}:43:9
 // CHECK-NEXT: Stepped to global: {{.*}}:44:1
 // CHECK-NEXT: Stepped to anonymous: {{.*}}:45:3
-// CHECK-NEXT: Stepped to anonymous: {{.*}}:44:23
+// CHECK-NEXT: Stepped to anonymous: {{.*}}:44:11
 // CHECK-NEXT: Stepped to anonymous: {{.*}}:45:3
-// CHECK-NEXT: Stepped to anonymous: {{.*}}:44:23
+// CHECK-NEXT: Stepped to anonymous: {{.*}}:44:11
 // CHECK-NEXT: Stepped to anonymous: {{.*}}:45:3
 // CHECK-NEXT: Continuing execution

test/hermes/hermes-internal-get-function-location.js

@@ -20,7 +20,7 @@ print(loc(fn1).lineNumber);
 // JS: [[@LINE-5]]
 // BC: undefined
 print(loc(fn1).columnNumber);
-// JS-NEXT: 16
+// JS-NEXT: 1
 // BC-NEXT: undefined
 print(loc(fn1).cjsModuleOffset);
 // JS-NEXT: undefined
@@ -40,7 +40,7 @@ print(loc(fn1Bound).lineNumber);
 // JS: [[@LINE-25]]
 // BC-NEXT: undefined
 print(loc(fn1Bound).columnNumber);
-// JS-NEXT: 16
+// JS-NEXT: 1
 // BC-NEXT: undefined
 print(loc(fn1Bound).cjsModuleOffset);
 // JS-NEXT: undefined

unittests/VMRuntime/HeapSnapshotTest.cpp

@@ -643,7 +643,8 @@ TEST(HeapSnapshotTest, TestNodesAndEdgesForDummyObjects) {
 
 using HeapSnapshotRuntimeTest = RuntimeTestFixture;
 
-TEST_F(HeapSnapshotRuntimeTest, SnapshotLazyCodeDoesNotAssert) {
+TEST_F(HeapSnapshotRuntimeTest, FunctionLocationForLazyCode) {
+  // Similar test to the above, but for lazy-compiled source.
   JSONFactory::Allocator alloc;
   JSONFactory jsonFactory{alloc};
 
@@ -652,16 +653,41 @@ TEST_F(HeapSnapshotRuntimeTest, SnapshotLazyCodeDoesNotAssert) {
   flags.lazy = true;
 
   // Build a function that will be lazily compiled.
-  std::string source = "function myGlobal() { ";
+  std::string source = "    function myGlobal() { ";
   for (int i = 0; i < 100; i++)
     source += " Math.random(); ";
-  source += "};\n";
+  source += "};\nmyGlobal;";
 
   CallResult<HermesValue> res = runtime->run(source, "file:///fake.js", flags);
   ASSERT_FALSE(isException(res));
+  Handle<JSFunction> func = runtime->makeHandle(vmcast<JSFunction>(*res));
+  const auto funcID = runtime->getHeap().getObjectID(func.get());
+
+  JSONObject *root = TAKE_SNAPSHOT(runtime->getHeap(), jsonFactory);
+  ASSERT_NE(root, nullptr);
+
+  const JSONArray &nodes = *llvm::cast<JSONArray>(root->at("nodes"));
+  const JSONArray &strings = *llvm::cast<JSONArray>(root->at("strings"));
+
+  // This test requires a location to be emitted.
+  auto node = FIND_NODE_FOR_ID(funcID, nodes, strings);
+  Node expected{HeapSnapshot::NodeType::Closure,
+                "myGlobal",
+                funcID,
+                func->getAllocatedSize(),
+                6};
+  EXPECT_EQ(node, expected);
+  // Edges aren't tested in this test.
 
-  // Make sure we can capture a snapshot without asserting.
-  TAKE_SNAPSHOT(runtime->getHeap(), jsonFactory);
+#ifdef HERMES_ENABLE_DEBUGGER
+  // The location isn't emitted in fully optimized builds.
+  const JSONArray &locations = *llvm::cast<JSONArray>(root->at("locations"));
+  Location loc = FIND_LOCATION_FOR_ID(funcID, locations, nodes, strings);
+  // The location should be the first file, at line 1 column 5 with indenting
+  EXPECT_EQ(loc, Location(expected, 1, 1, 5));
+#else
+  (void)findLocationForID;
+#endif
 }
 
 TEST_F(HeapSnapshotRuntimeTest, FunctionLocationAndNameTest) {
@@ -671,8 +697,9 @@ TEST_F(HeapSnapshotRuntimeTest, FunctionLocationAndNameTest) {
   // Make sure that debug info is emitted for this source file when it's
   // compiled.
   flags.debug = true;
+  // Indent the function slightly to test that the source location is correct
   CallResult<HermesValue> res =
-      runtime->run("function foo() {}; foo;", "file:///fake.js", flags);
+      runtime->run("\n  function foo() {}; foo;", "file:///fake.js", flags);
   ASSERT_FALSE(isException(res));
   Handle<JSFunction> func = runtime->makeHandle(vmcast<JSFunction>(*res));
   const auto funcID = runtime->getHeap().getObjectID(func.get());
@@ -697,9 +724,8 @@ TEST_F(HeapSnapshotRuntimeTest, FunctionLocationAndNameTest) {
   // The location isn't emitted in fully optimized builds.
   const JSONArray &locations = *llvm::cast<JSONArray>(root->at("locations"));
   Location loc = FIND_LOCATION_FOR_ID(funcID, locations, nodes, strings);
-  // The location should be the first file, at line 1 column 16 where the
-  // function's opening curly brace is.
-  EXPECT_EQ(loc, Location(expected, 1, 1, 16));
+  // The location should be the first file, second line, third column
+  EXPECT_EQ(loc, Location(expected, 1, 2, 3));
 #else
   (void)findLocationForID;
 #endif