Fix SourcePositionInfo for wasm

In wasm we often don't have a SharedFunctionInfo associated with a
compilation job, so we can't get a Script. Just print "unknown" in
these cases (instead of crashing).

R=titzer@chromium.org
CC=​herhut@chromium.org

Bug: chromium:840757, v8:7738
Change-Id: I850c6adfd9e07c9a0f6dd018f1a9314feb89d887
Reviewed-on: https://chromium-review.googlesource.com/1049632
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53080}

src/perf-jit.cc

@@ -272,21 +272,22 @@ void PerfJitLogger::WriteJitCodeLoadEntry(const uint8_t* code_pointer,
 
 namespace {
 
-std::unique_ptr<char[]> GetScriptName(Handle<Script> script) {
-  Object* name_or_url = script->GetNameOrSourceURL();
-  int name_length = 0;
-  std::unique_ptr<char[]> name_string;
-  if (name_or_url->IsString()) {
-    return String::cast(name_or_url)
-        ->ToCString(DISALLOW_NULLS, FAST_STRING_TRAVERSAL, &name_length);
-  } else {
-    const char unknown[] = "<unknown>";
-    name_length = static_cast<int>(strlen(unknown));
-    char* buffer = NewArray<char>(name_length);
-    base::OS::StrNCpy(buffer, name_length + 1, unknown,
-                      static_cast<size_t>(name_length));
-    return std::unique_ptr<char[]>(buffer);
+// TODO(clemensh): The heap-allocated string returned from here is just used to
+// invoke {strlen} on it or to output it directly. Refactor this to make it
+// cheaper.
+std::unique_ptr<char[]> GetScriptName(const SourcePositionInfo& info) {
+  if (!info.script.is_null()) {
+    Object* name_or_url = info.script->GetNameOrSourceURL();
+    if (name_or_url->IsString()) {
+      return String::cast(name_or_url)
+          ->ToCString(DISALLOW_NULLS, FAST_STRING_TRAVERSAL);
+    }
   }
+  constexpr char kUnknownString[] = "<unknown>";
+  constexpr size_t kUnknownStringLen = arraysize(kUnknownString) - 1;
+  std::unique_ptr<char[]> name(NewArray<char>(kUnknownStringLen + 1));
+  memcpy(name.get(), kUnknownString, kUnknownStringLen + 1);
+  return name;
 }
 
 SourcePositionInfo GetSourcePositionInfo(Handle<Code> code,
@@ -332,8 +333,7 @@ void PerfJitLogger::LogWriteDebugInfo(Code* code, SharedFunctionInfo* shared) {
        !iterator.done(); iterator.Advance()) {
     SourcePositionInfo info(GetSourcePositionInfo(code_handle, function_handle,
                                                   iterator.source_position()));
-    Handle<Script> script(Script::cast(info.function->script()));
-    std::unique_ptr<char[]> name_string = GetScriptName(script);
+    std::unique_ptr<char[]> name_string = GetScriptName(info);
     size += (static_cast<uint32_t>(strlen(name_string.get())) + 1);
   }
 
@@ -355,8 +355,7 @@ void PerfJitLogger::LogWriteDebugInfo(Code* code, SharedFunctionInfo* shared) {
     entry.line_number_ = info.line + 1;
     entry.column_ = info.column + 1;
     LogWriteBytes(reinterpret_cast<const char*>(&entry), sizeof(entry));
-    Handle<Script> script(Script::cast(info.function->script()));
-    std::unique_ptr<char[]> name_string = GetScriptName(script);
+    std::unique_ptr<char[]> name_string = GetScriptName(info);
     LogWriteBytes(name_string.get(),
                   static_cast<uint32_t>(strlen(name_string.get())) + 1);
   }

src/profiler/profiler-listener.cc

@@ -278,8 +278,8 @@ void ProfilerListener::RecordDeoptInlinedFrames(CodeEntry* entry,
       std::vector<CpuProfileDeoptFrame> inlined_frames;
       for (SourcePositionInfo& pos_info : last_position.InliningStack(code)) {
         if (pos_info.position.ScriptOffset() == kNoSourcePosition) continue;
-        if (!pos_info.function->script()->IsScript()) continue;
-        int script_id = Script::cast(pos_info.function->script())->id();
+        if (pos_info.script.is_null()) continue;
+        int script_id = pos_info.script->id();
         size_t offset = static_cast<size_t>(pos_info.position.ScriptOffset());
         inlined_frames.push_back(CpuProfileDeoptFrame({script_id, offset}));
       }

src/source-position.cc

@@ -10,17 +10,9 @@ namespace v8 {
 namespace internal {
 
 std::ostream& operator<<(std::ostream& out, const SourcePositionInfo& pos) {
-  Handle<SharedFunctionInfo> function(pos.function);
-  String* name = nullptr;
-  if (function->script()->IsScript()) {
-    Script* script = Script::cast(function->script());
-    if (script->name()->IsString()) {
-      name = String::cast(script->name());
-    }
-  }
   out << "<";
-  if (name != nullptr) {
-    out << name->ToCString(DISALLOW_NULLS).get();
+  if (!pos.script.is_null() && pos.script->name()->IsString()) {
+    out << String::cast(pos.script->name())->ToCString(DISALLOW_NULLS).get();
   } else {
     out << "unknown";
   }
@@ -125,9 +117,11 @@ void SourcePosition::Print(std::ostream& out, Code* code) const {
 
 SourcePositionInfo::SourcePositionInfo(SourcePosition pos,
                                        Handle<SharedFunctionInfo> f)
-    : position(pos), function(f) {
-  if (function->script()->IsScript()) {
-    Handle<Script> script(Script::cast(function->script()));
+    : position(pos),
+      script(f.is_null() || !f->script()->IsScript()
+                 ? Handle<Script>::null()
+                 : handle(Script::cast(f->script()))) {
+  if (!script.is_null()) {
     Script::PositionInfo info;
     if (Script::GetPositionInfo(script, pos.ScriptOffset(), &info,
                                 Script::WITH_OFFSET)) {

src/source-position.h

@@ -105,7 +105,7 @@ struct SourcePositionInfo {
   SourcePositionInfo(SourcePosition pos, Handle<SharedFunctionInfo> f);
 
   SourcePosition position;
-  Handle<SharedFunctionInfo> function;
+  Handle<Script> script;
   int line = -1;
   int column = -1;
 };

test/mjsunit/mjsunit.status

@@ -418,6 +418,10 @@
   # https://bugs.chromium.org/p/v8/issues/detail?id=7102
   # Flaky due to huge string allocation.
   'regress/regress-748069': [SKIP],
+
+  # https://crbug.com/v8/7738
+  # Code comments currently leak memory.
+  'regress/wasm/regress-840757': [SKIP],
 }],  # 'asan == True'
 
 ##############################################################################

test/mjsunit/regress/wasm/regress-840757.js

+// Copyright 2018 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.
+
+load('test/mjsunit/wasm/wasm-constants.js');
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+// Also enable predictable mode. Otherwise, concurrent recompilation will be
+// enabled, and the code generator will not try to print the InliningStack
+// (see CodeGenerator::AssembleSourcePosition).
+// These preconditions make this test quite fragile, but it's the only way
+// currently to reproduce this.
+// Flags: --code-comments --predictable --print-wasm-code
+
+const builder = new WasmModuleBuilder();
+// Add a call instruction, because the segfault happens when processing source
+// positions.
+builder.addFunction('foo', kSig_v_v).addBody([]);
+builder.addFunction('test', kSig_v_v).addBody([kExprCallFunction, 0]);
+
+builder.instantiate();