[wasm] Differentiate unnamed and empty names

Empty function names are allowed and are output as such, unnamed
functions or functions with no valid UTF-8 name are output as
"<WASM UNNAMED>", while the CallSite object returns null as the
function name.

R=titzer@chromium.org, yangguo@chromium.org

Review-Url: https://codereview.chromium.org/1970503004
Cr-Commit-Position: refs/heads/master@{#36348}

src/frames.cc

@@ -16,7 +16,6 @@
 #include "src/safepoint-table.h"
 #include "src/string-stream.h"
 #include "src/vm-state-inl.h"
-#include "src/wasm/wasm-module.h"
 
 namespace v8 {
 namespace internal {
@@ -1355,13 +1354,6 @@ uint32_t WasmFrame::function_index() {
   return val;
 }
 
-Object* WasmFrame::function_name() {
-  Object* wasm_object = wasm_obj();
-  if (wasm_object->IsUndefined()) return wasm_object;
-  Handle<JSObject> wasm = handle(JSObject::cast(wasm_object));
-  return *wasm::GetWasmFunctionName(wasm, function_index());
-}
-
 namespace {
 
 

src/frames.h

@@ -971,8 +971,6 @@ class WasmFrame : public StandardFrame {
   Object* wasm_obj();
   uint32_t function_index();
 
-  Object* function_name();
-
   static WasmFrame* cast(StackFrame* frame) {
     DCHECK(frame->is_wasm());
     return static_cast<WasmFrame*>(frame);

src/isolate.cc

@@ -39,7 +39,7 @@
 #include "src/v8.h"
 #include "src/version.h"
 #include "src/vm-state-inl.h"
-
+#include "src/wasm/wasm-module.h"
 
 namespace v8 {
 namespace internal {
@@ -616,11 +616,17 @@ class CaptureStackTraceHelper {
         factory()->NewJSObject(isolate_->object_function());
 
     if (!function_key_.is_null()) {
-      Handle<Object> fun_name = handle(frame->function_name(), isolate_);
-      if (fun_name->IsUndefined())
-        fun_name = isolate_->factory()->InternalizeUtf8String(
-            Vector<const char>("<WASM>"));
-      JSObject::AddProperty(stack_frame, function_key_, fun_name, NONE);
+      Object* wasm_object = frame->wasm_obj();
+      Handle<String> name;
+      if (!wasm_object->IsUndefined()) {
+        Handle<JSObject> wasm = handle(JSObject::cast(wasm_object));
+        wasm::GetWasmFunctionName(wasm, frame->function_index())
+            .ToHandle(&name);
+      }
+      if (name.is_null()) {
+        name = isolate_->factory()->NewStringFromStaticChars("<WASM UNNAMED>");
+      }
+      JSObject::AddProperty(stack_frame, function_key_, name, NONE);
     }
     // Encode the function index as line number.
     if (!line_key_.is_null()) {

src/js/messages.js

@@ -630,12 +630,6 @@ function CallSiteGetScriptNameOrSourceURL() {
 function CallSiteGetFunctionName() {
   // See if the function knows its own name
   CheckCallSite(this, "getFunctionName");
-  if (HAS_PRIVATE(this, callSiteWasmObjectSymbol)) {
-    var wasm = GET_PRIVATE(this, callSiteWasmObjectSymbol);
-    var func_index = GET_PRIVATE(this, callSiteWasmFunctionIndexSymbol);
-    if (IS_UNDEFINED(wasm)) return "<WASM>";
-    return %WasmGetFunctionName(wasm, func_index);
-  }
   return %CallSiteGetFunctionNameRT(this);
 }
 
@@ -679,7 +673,8 @@ function CallSiteToString() {
     var funName = this.getFunctionName();
     var funcIndex = GET_PRIVATE(this, callSiteWasmFunctionIndexSymbol);
     var pos = this.getPosition();
-    return funName + " (<WASM>:" + funcIndex + ":" + pos + ")";
+    if (IS_NULL(funName)) funName = "<WASM UNNAMED>";
+    return funName + " (<WASM>[" + funcIndex + "]+" + pos + ")";
   }
 
   var fileName;

src/messages.cc

@@ -205,12 +205,13 @@ Handle<Object> CallSite::GetFileName() {
 
 Handle<Object> CallSite::GetFunctionName() {
   if (IsWasm()) {
-    if (wasm_obj_->IsUndefined()) return isolate_->factory()->null_value();
-    // wasm_obj_ can be a String if we generate WASM code directly in a test
-    // case.
-    if (wasm_obj_->IsString()) return wasm_obj_;
-    return wasm::GetWasmFunctionName(Handle<JSObject>::cast(wasm_obj_),
-                                     wasm_func_index_);
+    MaybeHandle<String> name;
+    if (!wasm_obj_->IsUndefined()) {
+      name = wasm::GetWasmFunctionName(Handle<JSObject>::cast(wasm_obj_),
+                                       wasm_func_index_);
+    }
+    if (name.is_null()) return isolate_->factory()->null_value();
+    return name.ToHandleChecked();
   }
   Handle<String> result = JSFunction::GetName(fun_);
   if (result->length() != 0) return result;

src/runtime/runtime-function.cc

@@ -311,15 +311,5 @@ RUNTIME_FUNCTION(Runtime_FunctionToString) {
              : *JSFunction::ToString(Handle<JSFunction>::cast(function));
 }
 
-RUNTIME_FUNCTION(Runtime_WasmGetFunctionName) {
-  HandleScope scope(isolate);
-  DCHECK_EQ(2, args.length());
-
-  CONVERT_ARG_HANDLE_CHECKED(JSObject, wasm, 0);
-  CONVERT_SMI_ARG_CHECKED(func_index, 1);
-
-  return *wasm::GetWasmFunctionName(wasm, func_index);
-}
-
 }  // namespace internal
 }  // namespace v8

src/runtime/runtime.h

@@ -320,7 +320,6 @@ namespace internal {
   F(GetAndResetRuntimeCallStats, -1 /* <= 2 */, 1)  \
   F(EnqueueMicrotask, 1, 1)                         \
   F(RunMicrotasks, 0, 1)                            \
-  F(WasmGetFunctionName, 2, 1)                      \
   F(OrdinaryHasInstance, 2, 1)
 
 #define FOR_EACH_INTRINSIC_JSON(F) \

src/wasm/wasm-function-name-table.cc

@@ -14,10 +14,12 @@ namespace wasm {
 // module, then the first (kIntSize * (N+1)) bytes are integer entries.
 // The first integer entry encodes the number of functions in the module.
 // The entries 1 to N contain offsets into the second part of this array.
+// If a function is unnamed (not to be confused with an empty name), then the
+// integer entry is the negative offset of the next function name.
 // After these N+1 integer entries, the second part begins, which holds a
 // concatenation of all function names.
 //
-// Returns undefined if the array length would not fit in an integer value
+// Returns undefined if the array length would not fit in an integer value.
 Handle<Object> BuildFunctionNamesTable(Isolate* isolate, WasmModule* module) {
   uint64_t func_names_length = 0;
   for (auto& func : module->functions) func_names_length += func.name_length;
@@ -36,37 +38,35 @@ Handle<Object> BuildFunctionNamesTable(Isolate* isolate, WasmModule* module) {
   int func_index = 0;
   for (WasmFunction& fun : module->functions) {
     WasmName name = module->GetNameOrNull(&fun);
-    func_names_array->copy_in(current_offset,
-                              reinterpret_cast<const byte*>(name.start()),
-                              name.length());
-    func_names_array->set_int(func_index + 1, current_offset);
-    current_offset += name.length();
+    if (name.start() == nullptr) {
+      func_names_array->set_int(func_index + 1, -current_offset);
+    } else {
+      func_names_array->copy_in(current_offset,
+                                reinterpret_cast<const byte*>(name.start()),
+                                name.length());
+      func_names_array->set_int(func_index + 1, current_offset);
+      current_offset += name.length();
+    }
     ++func_index;
   }
   return func_names_array;
 }
 
-Handle<Object> GetWasmFunctionNameFromTable(Handle<ByteArray> func_names_array,
-                                            uint32_t func_index) {
+MaybeHandle<String> GetWasmFunctionNameFromTable(
+    Handle<ByteArray> func_names_array, uint32_t func_index) {
   uint32_t num_funcs = static_cast<uint32_t>(func_names_array->get_int(0));
   DCHECK(static_cast<int>(num_funcs) >= 0);
-  auto undefined = [&func_names_array]() -> Handle<Object> {
-    return func_names_array->GetIsolate()->factory()->undefined_value();
-  };
-  if (func_index >= num_funcs) return undefined();
+  Factory* factory = func_names_array->GetIsolate()->factory();
+  DCHECK(func_index < num_funcs);
   int offset = func_names_array->get_int(func_index + 1);
+  if (offset < 0) return {};
   int next_offset = func_index == num_funcs - 1
                         ? func_names_array->length()
-                        : func_names_array->get_int(func_index + 2);
+                        : abs(func_names_array->get_int(func_index + 2));
   ScopedVector<byte> buffer(next_offset - offset);
   func_names_array->copy_out(offset, buffer.start(), next_offset - offset);
-  if (!unibrow::Utf8::Validate(buffer.start(), buffer.length())) {
-    return undefined();
-  }
-  MaybeHandle<Object> maybe_name =
-      func_names_array->GetIsolate()->factory()->NewStringFromUtf8(
-          Vector<const char>::cast(buffer));
-  return maybe_name.is_null() ? undefined() : maybe_name.ToHandleChecked();
+  if (!unibrow::Utf8::Validate(buffer.start(), buffer.length())) return {};
+  return factory->NewStringFromUtf8(Vector<const char>::cast(buffer));
 }
 
 }  // namespace wasm

src/wasm/wasm-function-name-table.h

@@ -16,12 +16,15 @@ namespace wasm {
 struct WasmModule;
 
 // Encode all function names of the WasmModule into one ByteArray.
+// Returns undefined if the array length would not fit in an integer value.
 Handle<Object> BuildFunctionNamesTable(Isolate* isolate, WasmModule* module);
 
-// Extract the function name for the given func_index from the wasm module.
-// Returns undefined if the function index is invalid.
-Handle<Object> GetWasmFunctionNameFromTable(Handle<ByteArray> wasm_names_table,
-                                            uint32_t func_index);
+// Extract the function name for the given func_index from the function name
+// table.
+// Returns a null handle if the respective function is unnamed (not to be
+// confused with empty names) or the function name is not a valid UTF-8 string.
+MaybeHandle<String> GetWasmFunctionNameFromTable(
+    Handle<ByteArray> wasm_names_table, uint32_t func_index);
 
 }  // namespace wasm
 }  // namespace internal

src/wasm/wasm-module.cc

@@ -332,7 +332,7 @@ static MaybeHandle<JSFunction> ReportFFIError(ErrorThrower& thrower,
                                               const char* error, uint32_t index,
                                               wasm::WasmName module_name,
                                               wasm::WasmName function_name) {
-  if (function_name.start()) {
+  if (!function_name.is_empty()) {
     thrower.Error("Import #%d module=\"%.*s\" function=\"%.*s\" error: %s",
                   index, module_name.length(), module_name.start(),
                   function_name.length(), function_name.start(), error);
@@ -368,7 +368,7 @@ static MaybeHandle<JSFunction> LookupFunction(
   }
 
   Handle<Object> function;
-  if (function_name.start()) {
+  if (!function_name.is_empty()) {
     // Look up the function in the module.
     Handle<String> name = factory->InternalizeUtf8String(function_name);
     MaybeHandle<Object> result = Object::GetProperty(module, name);
@@ -977,13 +977,12 @@ int32_t CompileAndRunWasmModule(Isolate* isolate, WasmModule* module) {
   return -1;
 }
 
-Handle<Object> GetWasmFunctionName(Handle<JSObject> wasm, uint32_t func_index) {
-  Handle<Object> func_names_arr_obj = handle(
-      wasm->GetInternalField(kWasmFunctionNamesArray), wasm->GetIsolate());
-  if (func_names_arr_obj->IsUndefined())
-    return func_names_arr_obj;  // Return undefined.
+MaybeHandle<String> GetWasmFunctionName(Handle<JSObject> wasm,
+                                        uint32_t func_index) {
+  Object* func_names_arr_obj = wasm->GetInternalField(kWasmFunctionNamesArray);
+  if (func_names_arr_obj->IsUndefined()) return Handle<String>::null();
   return GetWasmFunctionNameFromTable(
-      Handle<ByteArray>::cast(func_names_arr_obj), func_index);
+      handle(ByteArray::cast(func_names_arr_obj)), func_index);
 }
 
 }  // namespace wasm

src/wasm/wasm-module.h

@@ -195,7 +195,7 @@ struct WasmModule {
 
   // Get a string stored in the module bytes representing a name.
   WasmName GetNameOrNull(uint32_t offset, uint32_t length) const {
-    if (length == 0) return {NULL, 0};  // no name.
+    if (offset == 0 && length == 0) return {NULL, 0};  // no name.
     CHECK(BoundsCheck(offset, offset + length));
     DCHECK_GE(static_cast<int>(length), 0);
     return {reinterpret_cast<const char*>(module_start + offset),
@@ -325,9 +325,10 @@ int32_t CompileAndRunWasmModule(Isolate* isolate, const byte* module_start,
 int32_t CompileAndRunWasmModule(Isolate* isolate, WasmModule* module);
 
 // Extract a function name from the given wasm object.
-// Returns undefined if the function is unnamed or the function index is
-// invalid.
-Handle<Object> GetWasmFunctionName(Handle<JSObject> wasm, uint32_t func_index);
+// Returns a null handle if the function is unnamed or the name is not a valid
+// UTF-8 string.
+MaybeHandle<String> GetWasmFunctionName(Handle<JSObject> wasm,
+                                        uint32_t func_index);
 
 }  // namespace wasm
 }  // namespace internal

test/cctest/wasm/test-wasm-function-name-table.cc

@@ -30,10 +30,13 @@ void testFunctionNameTable(Vector<Vector<const char>> names) {
 
   WasmModule module;
   std::vector<char> all_names;
+  // No name should have offset 0, because that encodes unnamed functions.
+  // In real wasm binary, offset 0 is impossible anyway.
+  all_names.push_back('\0');
 
   uint32_t func_index = 0;
   for (Vector<const char> name : names) {
-    size_t name_offset = all_names.size();
+    size_t name_offset = name.start() ? all_names.size() : 0;
     all_names.insert(all_names.end(), name.start(),
                      name.start() + name.length());
     // Make every second function name null-terminated.
@@ -53,19 +56,21 @@ void testFunctionNameTable(Vector<Vector<const char>> names) {
 
   func_index = 0;
   for (Vector<const char> name : names) {
-    Handle<Object> string_obj = GetWasmFunctionNameFromTable(
+    MaybeHandle<String> string = GetWasmFunctionNameFromTable(
         Handle<ByteArray>::cast(wasm_function_name_table), func_index);
-    CHECK(!string_obj.is_null());
-    CHECK(string_obj->IsString());
-    Handle<String> string = Handle<String>::cast(string_obj);
-    CHECK(string->IsUtf8EqualTo(name));
+    if (name.start()) {
+      CHECK(string.ToHandleChecked()->IsUtf8EqualTo(name));
+    } else {
+      CHECK(string.is_null());
+    }
     ++func_index;
   }
 }
 
 void testFunctionNameTable(Vector<const char *> names) {
   std::vector<Vector<const char>> names_vec;
-  for (const char *name : names) names_vec.push_back(CStrVector(name));
+  for (const char *name : names)
+    names_vec.push_back(name ? CStrVector(name) : Vector<const char>());
   testFunctionNameTable(Vector<Vector<const char>>(
       names_vec.data(), static_cast<int>(names_vec.size())));
 }
@@ -108,3 +113,8 @@ TEST(UTF8Names) {
   const char *names[] = {"↱fun↰", "↺", "alpha:α beta:β"};
   testFunctionNameTable(ArrayVector(names));
 }
+
+TEST(UnnamedVsEmptyNames) {
+  const char *names[] = {"", nullptr, nullptr, ""};
+  testFunctionNameTable(ArrayVector(names));
+}

test/cctest/wasm/test-wasm-stack.cc

@@ -112,11 +112,11 @@ TEST(CollectDetailedWasmStack_ExplicitThrowFromJs) {
 
   // Line number is 1-based, with 0 == kNoLineNumberInfo.
   ExceptionInfo expected_exceptions[] = {
-      {"a", 3, 8},                                    // -
-      {"js", 4, 2},                                   // -
-      {"<WASM>", static_cast<int>(wasm_index), 2},    // -
-      {"<WASM>", static_cast<int>(wasm_index_2), 1},  // -
-      {"callFn", 1, 24}                               // -
+      {"a", 3, 8},                                            // -
+      {"js", 4, 2},                                           // -
+      {"<WASM UNNAMED>", static_cast<int>(wasm_index), 2},    // -
+      {"<WASM UNNAMED>", static_cast<int>(wasm_index_2), 1},  // -
+      {"callFn", 1, 24}                                       // -
   };
   CheckExceptionInfos(isolate, maybe_exc.ToHandleChecked(),
                       expected_exceptions);
@@ -157,9 +157,9 @@ TEST(CollectDetailedWasmStack_WasmError) {
 
   // Line number is 1-based, with 0 == kNoLineNumberInfo.
   ExceptionInfo expected_exceptions[] = {
-      {"<WASM>", static_cast<int>(wasm_index), 1},    // -
-      {"<WASM>", static_cast<int>(wasm_index_2), 1},  // -
-      {"callFn", 1, 24}                               //-
+      {"<WASM UNNAMED>", static_cast<int>(wasm_index), 1},    // -
+      {"<WASM UNNAMED>", static_cast<int>(wasm_index_2), 1},  // -
+      {"callFn", 1, 24}                                       //-
   };
   CheckExceptionInfos(isolate, maybe_exc.ToHandleChecked(),
                       expected_exceptions);

test/cctest/wasm/test-wasm-trap-position.cc

@@ -89,8 +89,8 @@ TEST(Unreachable) {
   CHECK(returnObjMaybe.is_null());
 
   ExceptionInfo expected_exceptions[] = {
-      {"<WASM>", static_cast<int>(wasm_index), 1},  // --
-      {"callFn", 1, 24}                             // --
+      {"<WASM UNNAMED>", static_cast<int>(wasm_index), 1},  // --
+      {"callFn", 1, 24}                                     // --
   };
   CheckExceptionInfos(isolate, maybe_exc.ToHandleChecked(),
                       expected_exceptions);
@@ -131,9 +131,9 @@ TEST(IllegalLoad) {
 
   // Line number is 1-based, with 0 == kNoLineNumberInfo.
   ExceptionInfo expected_exceptions[] = {
-      {"<WASM>", static_cast<int>(wasm_index), 6},    // --
-      {"<WASM>", static_cast<int>(wasm_index_2), 2},  // --
-      {"callFn", 1, 24}                               // --
+      {"<WASM UNNAMED>", static_cast<int>(wasm_index), 6},    // --
+      {"<WASM UNNAMED>", static_cast<int>(wasm_index_2), 2},  // --
+      {"callFn", 1, 24}                                       // --
   };
   CheckExceptionInfos(isolate, maybe_exc.ToHandleChecked(),
                       expected_exceptions);

test/mjsunit/wasm/function-names.js

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --expose-wasm
+
+load("test/mjsunit/wasm/wasm-constants.js");
+load("test/mjsunit/wasm/wasm-module-builder.js");
+
+var builder = new WasmModuleBuilder();
+
+var last_func_index = builder.addFunction("exec_unreachable", kSig_v_v)
+  .addBody([kExprUnreachable])
+
+var illegal_func_name = [0xff];
+var func_names = [ "☠", illegal_func_name, "some math: (½)² = ¼", "" ];
+var expected_names = ["exec_unreachable", "☠", null,
+                      "some math: (½)² = ¼", "", "main"];
+
+for (var func_name of func_names) {
+  last_func_index = builder.addFunction(func_name, kSig_v_v)
+    .addBody([kExprCallFunction, kArity0, last_func_index]).index;
+}
+
+builder.addFunction("main", kSig_v_v)
+  .addBody([kExprCallFunction, kArity0, last_func_index])
+  .exportFunc();
+
+var module = builder.instantiate();
+
+(function testFunctionNamesAsString() {
+  var names = expected_names.concat(["testFunctionNamesAsString", null]);
+  try {
+    module.exports.main();
+    assertFalse("should throw");
+  } catch (e) {
+    var lines = e.stack.split(/\r?\n/);
+    lines.shift();
+    assertEquals(names.length, lines.length);
+    for (var i = 0; i < names.length; ++i) {
+      var line = lines[i].trim();
+      if (names[i] === null) continue;
+      var printed_name = names[i] === undefined ? "<WASM UNNAMED>" : names[i]
+      var expected_start = "at " + printed_name + " (";
+      assertTrue(line.startsWith(expected_start),
+          "should start with '" + expected_start + "': '" + line + "'");
+    }
+  }
+})();
+
+// For the remaining tests, collect the Callsite objects instead of just a
+// string:
+Error.prepareStackTrace = function(error, frames) {
+  return frames;
+};
+
+
+(function testFunctionNamesAsCallSites() {
+  var names = expected_names.concat(["testFunctionNamesAsCallSites", null]);
+  try {
+    module.exports.main();
+    assertFalse("should throw");
+  } catch (e) {
+    assertEquals(names.length, e.stack.length);
+    for (var i = 0; i < names.length; ++i) {
+      assertEquals(names[i], e.stack[i].getFunctionName());
+    }
+  }
+})();

test/mjsunit/wasm/stack.js

@@ -28,7 +28,7 @@ function verifyStack(frames, expected) {
     var toString;
     if (exp[0]) {
       var funName = exp[1] == "?" ? "" : exp[1];
-      toString = funName + " (<WASM>:" + exp[2] + ":" + exp[3] + ")";
+      toString = funName + " (<WASM>[" + exp[2] + "]+" + exp[3] + ")";
     } else {
       toString = exp[4] + ":" + exp[2] + ":";
     }
@@ -72,7 +72,7 @@ var module = builder.instantiate({func: STACK});
   var expected_string = "Error\n" +
     // The line numbers below will change as this test gains / loses lines..
     "    at STACK (stack.js:42:11)\n" +           // --
-    "    at main (<WASM>:0:1)\n" +                // --
+    "    at main (<WASM>[0]+1)\n" +               // --
     "    at testSimpleStack (stack.js:79:18)\n" + // --
     "    at stack.js:81:3";                       // --