[wasm] Use right data type for storing error location

Use int instead of byte to store the source position when computing a
location based on the stack trace stored in an error object.
Also add tests, since this code path was not covered before (not even
for small position where it would have succeeded).
Also, add some comments about which positions are 0-based and 1-based.

R=titzer@chromium.org

Change-Id: I313dcd6c47b77093ced9bb687415715d04eafb97
Reviewed-on: https://chromium-review.googlesource.com/645527
Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47739}

src/isolate.cc

@@ -1629,7 +1629,7 @@ bool Isolate::ComputeLocationFromStackTrace(MessageLocation* target,
       bool is_at_number_conversion =
           elements->IsAsmJsWasmFrame(i) &&
           elements->Flags(i)->value() & FrameArray::kAsmJsAtNumberConversion;
-      byte pos = WasmCompiledModule::GetSourcePosition(
+      int pos = WasmCompiledModule::GetSourcePosition(
           compiled_module, func_index, byte_offset, is_at_number_conversion);
       Handle<Script> script(compiled_module->script());
 

src/objects.h

@@ -5315,6 +5315,8 @@ class JSMessageObject: public JSObject {
   inline int end_position() const;
   inline void set_end_position(int value);
 
+  // Returns the line number for the error message (1-based), or
+  // Message::kNoLineNumberInfo if the line cannot be determined.
   int GetLineNumber() const;
 
   // Returns the offset of the given position within the containing line.

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

@@ -47,8 +47,8 @@ void PrintStackTrace(v8::Isolate* isolate, v8::Local<v8::StackTrace> stack) {
 
 struct ExceptionInfo {
   const char* func_name;
-  int line_nr;
-  int column;
+  int line_nr;  // 1-based
+  int column;   // 1-based
 };
 
 template <int N>
@@ -70,9 +70,29 @@ void CheckExceptionInfos(v8::internal::Isolate* i_isolate, Handle<Object> exc,
     v8::Local<v8::StackFrame> frame = stack->GetFrame(frameNr);
     v8::String::Utf8Value funName(v8_isolate, frame->GetFunctionName());
     CHECK_CSTREQ(excInfos[frameNr].func_name, *funName);
+    // Line and column are 1-based in v8::StackFrame, just as in ExceptionInfo.
     CHECK_EQ(excInfos[frameNr].line_nr, frame->GetLineNumber());
     CHECK_EQ(excInfos[frameNr].column, frame->GetColumn());
   }
+
+  CheckComputeLocation(i_isolate, exc, excInfos[0]);
+}
+
+void CheckComputeLocation(v8::internal::Isolate* i_isolate, Handle<Object> exc,
+                          const ExceptionInfo& topLocation) {
+  MessageLocation loc;
+  CHECK(i_isolate->ComputeLocationFromStackTrace(&loc, exc));
+  printf("loc start: %d, end: %d\n", loc.start_pos(), loc.end_pos());
+  Handle<JSMessageObject> message = i_isolate->CreateMessage(exc, nullptr);
+  printf("msg start: %d, end: %d, line: %d, col: %d\n",
+         message->start_position(), message->end_position(),
+         message->GetLineNumber(), message->GetColumnNumber());
+  CHECK_EQ(loc.start_pos(), message->start_position());
+  CHECK_EQ(loc.end_pos(), message->end_position());
+  // In the message, the line is 1-based, but the column is 0-based.
+  CHECK_EQ(topLocation.line_nr, message->GetLineNumber());
+  CHECK_LE(1, topLocation.column);
+  CHECK_EQ(topLocation.column - 1, message->GetColumnNumber());
 }
 
 }  // namespace
@@ -125,40 +145,49 @@ TEST(CollectDetailedWasmStack_ExplicitThrowFromJs) {
 
 // Trigger a trap in wasm, stack should be JS -> wasm -> wasm.
 TEST(CollectDetailedWasmStack_WasmError) {
-  TestSignatures sigs;
-  // Create a WasmRunner with stack checks and traps enabled.
-  WasmRunner<int> r(kExecuteCompiled, "main", true);
+  for (int pos_shift = 0; pos_shift < 3; ++pos_shift) {
+    // Test a position with 1, 2 or 3 bytes needed to represent it.
+    int unreachable_pos = 1 << (8 * pos_shift);
+    TestSignatures sigs;
+    // Create a WasmRunner with stack checks and traps enabled.
+    WasmRunner<int> r(kExecuteCompiled, "main", true);
 
-  BUILD(r, WASM_UNREACHABLE);
-  uint32_t wasm_index_1 = r.function()->func_index;
+    std::vector<byte> code(unreachable_pos + 1, kExprNop);
+    code[unreachable_pos] = kExprUnreachable;
+    r.Build(code.data(), code.data() + code.size());
 
-  WasmFunctionCompiler& f2 = r.NewFunction<int>("call_main");
-  BUILD(f2, WASM_CALL_FUNCTION0(0));
-  uint32_t wasm_index_2 = f2.function_index();
+    uint32_t wasm_index_1 = r.function()->func_index;
 
-  Handle<JSFunction> js_wasm_wrapper = r.builder().WrapCode(wasm_index_2);
+    WasmFunctionCompiler& f2 = r.NewFunction<int>("call_main");
+    BUILD(f2, WASM_CALL_FUNCTION0(0));
+    uint32_t wasm_index_2 = f2.function_index();
 
-  Handle<JSFunction> js_trampoline = Handle<JSFunction>::cast(
-      v8::Utils::OpenHandle(*v8::Local<v8::Function>::Cast(
-          CompileRun("(function callFn(fn) { fn(); })"))));
+    Handle<JSFunction> js_wasm_wrapper = r.builder().WrapCode(wasm_index_2);
 
-  Isolate* isolate = js_wasm_wrapper->GetIsolate();
-  isolate->SetCaptureStackTraceForUncaughtExceptions(true, 10,
-                                                     v8::StackTrace::kOverview);
-  Handle<Object> global(isolate->context()->global_object(), isolate);
-  MaybeHandle<Object> maybe_exc;
-  Handle<Object> args[] = {js_wasm_wrapper};
-  MaybeHandle<Object> maybe_return_obj =
-      Execution::TryCall(isolate, js_trampoline, global, 1, args,
-                         Execution::MessageHandling::kReport, &maybe_exc);
-  CHECK(maybe_return_obj.is_null());
+    Handle<JSFunction> js_trampoline = Handle<JSFunction>::cast(
+        v8::Utils::OpenHandle(*v8::Local<v8::Function>::Cast(
+            CompileRun("(function callFn(fn) { fn(); })"))));
 
-  // Line and column are 1-based, so add 1 for the expected wasm output.
-  ExceptionInfo expected_exceptions[] = {
-      {"main", static_cast<int>(wasm_index_1) + 1, 2},       // -
-      {"call_main", static_cast<int>(wasm_index_2) + 1, 2},  // -
-      {"callFn", 1, 24}                                      //-
-  };
-  CheckExceptionInfos(isolate, maybe_exc.ToHandleChecked(),
-                      expected_exceptions);
+    Isolate* isolate = js_wasm_wrapper->GetIsolate();
+    isolate->SetCaptureStackTraceForUncaughtExceptions(
+        true, 10, v8::StackTrace::kOverview);
+    Handle<Object> global(isolate->context()->global_object(), isolate);
+    MaybeHandle<Object> maybe_exc;
+    Handle<Object> args[] = {js_wasm_wrapper};
+    MaybeHandle<Object> maybe_return_obj =
+        Execution::TryCall(isolate, js_trampoline, global, 1, args,
+                           Execution::MessageHandling::kReport, &maybe_exc);
+    CHECK(maybe_return_obj.is_null());
+    Handle<Object> exception = maybe_exc.ToHandleChecked();
+
+    static constexpr int kMainLocalsLength = 1;
+    // Line and column are 1-based, so add 1 for the expected wasm output.
+    const int expected_main_pos = unreachable_pos + kMainLocalsLength + 1;
+    ExceptionInfo expected_exceptions[] = {
+        {"main", static_cast<int>(wasm_index_1) + 1, expected_main_pos},  // -
+        {"call_main", static_cast<int>(wasm_index_2) + 1, 2},             // -
+        {"callFn", 1, 24}                                                 //-
+    };
+    CheckExceptionInfos(isolate, exception, expected_exceptions);
+  }
 }

test/cctest/wasm/wasm-run-utils.h

@@ -406,6 +406,7 @@ class TestingModuleBuilder {
     Handle<FixedArray> weak_exported = isolate_->factory()->NewFixedArray(0);
     compiled_module->set_weak_exported_functions(weak_exported);
     DCHECK(WasmCompiledModule::IsWasmCompiledModule(*compiled_module));
+    script->set_wasm_compiled_module(*compiled_module);
     return WasmInstanceObject::New(isolate_, compiled_module);
   }
 };

test/mjsunit/wasm/stack.js

@@ -128,7 +128,6 @@ Error.prepareStackTrace = function(error, frames) {
   }
 })();
 
-
 (function testStackOverflow() {
   print("testStackOverflow");
   var builder = new WasmModuleBuilder();
@@ -139,7 +138,7 @@ Error.prepareStackTrace = function(error, frames) {
       kExprI32Const, 0,
       kExprCallIndirect, sig_index, kTableZero
     ])
-    .exportFunc()
+    .exportFunc();
   builder.appendToTable([0]);
 
   try {
@@ -157,3 +156,29 @@ Error.prepareStackTrace = function(error, frames) {
     ]);
   }
 })();
+
+(function testBigOffset() {
+  print('testBigOffset');
+  var builder = new WasmModuleBuilder();
+
+  let body = [kExprI32Const, 0, kExprI32Add];
+  while (body.length <= 65536) body = body.concat(body);
+  body.unshift(kExprI32Const, 0);
+  body.push(kExprUnreachable);
+  let unreachable_pos = body.length - 1;
+
+  builder.addFunction('main', kSig_v_v).addBody(body).exportFunc();
+
+  try {
+    builder.instantiate().exports.main();
+    fail('expected wasm exception');
+  } catch (e) {
+    assertEquals('unreachable', e.message, 'trap reason');
+    verifyStack(e.stack, [
+      // isWasm, function, line, pos, file
+      [true, 'main', 0, unreachable_pos + 1, null],  // -
+      [false, 'testBigOffset', 173, 0, 'stack.js'],  //-
+      [false, null, 184, 0, 'stack.js']
+    ]);
+  }
+})();