[stack-trace] Change column number for wasm frames to module offset

The CL https://crrev.com/c/1646846 changed column numbers for Wasm
frames in Error.stack traces. Instead of using the offset relative to
the beginning of the function, the absolute offset inside the module
is displayed as hex.

This CL propagates that change to the StackTrace C++ API, so
StackFrame::GetColumn() also returns the absolute offset. Note that the
StackFrame API historically uses "0" to signal "no information", so the
line and column numbers for Wasm frames are also adjusted to 1-based,
even though they signify function index and absolute offset
into the module.

This CL does not touch Script::PositionInfo.column. That field still
contains the offset relative to the function start.

Bug: v8:8742
Change-Id: If4fd37fa681c7ebd0823ce0d95eccc1335c35272
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1655300
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62171}

src/execution/messages.cc

@@ -770,9 +770,8 @@ void WasmStackFrame::ToString(IncrementalStringBuilder& builder) {
   builder.AppendInt(static_cast<int>(wasm_func_index_));
   builder.AppendCString("]:");
 
-  int function_offset = module_object->GetFunctionOffset(wasm_func_index_);
   char buffer[16];
-  SNPrintF(ArrayVector(buffer), "0x%x", function_offset + GetPosition());
+  SNPrintF(ArrayVector(buffer), "0x%x", GetModuleOffset());
   builder.AppendCString(buffer);
 
   if (has_name) builder.AppendCString(")");
@@ -787,6 +786,14 @@ int WasmStackFrame::GetPosition() const {
                    code_, offset_);
 }
 
+int WasmStackFrame::GetColumnNumber() { return GetModuleOffset(); }
+
+int WasmStackFrame::GetModuleOffset() const {
+  const int function_offset =
+      wasm_instance_->module_object().GetFunctionOffset(wasm_func_index_);
+  return function_offset + GetPosition();
+}
+
 Handle<Object> WasmStackFrame::Null() const {
   return isolate_->factory()->null_value();
 }

src/execution/messages.h

@@ -177,7 +177,7 @@ class WasmStackFrame : public StackFrameBase {
 
   int GetPosition() const override;
   int GetLineNumber() override { return wasm_func_index_; }
-  int GetColumnNumber() override { return kNone; }
+  int GetColumnNumber() override;
 
   int GetPromiseIndex() const override { return kNone; }
 
@@ -203,6 +203,8 @@ class WasmStackFrame : public StackFrameBase {
   int offset_;
 
  private:
+  int GetModuleOffset() const;
+
   WasmStackFrame() = default;
   void FromFrameArray(Isolate* isolate, Handle<FrameArray> array, int frame_ix);
 

src/heap/factory.cc

@@ -3661,10 +3661,9 @@ Handle<StackFrameInfo> Factory::NewStackFrameInfo(
   int line = it.Frame()->GetLineNumber();
   if (is_wasm && line >= 0) line++;
 
-  // Column numbers are 1-based. For Wasm we use the position
-  // as the iterator does not currently provide a column number.
-  const int column =
-      is_wasm ? it.Frame()->GetPosition() + 1 : it.Frame()->GetColumnNumber();
+  // Column numbers are 1-based, for Wasm we need to adjust.
+  int column = it.Frame()->GetColumnNumber();
+  if (is_wasm && column >= 0) column++;
 
   const int script_id = it.Frame()->GetScriptId();
 

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

@@ -92,7 +92,10 @@ void CheckComputeLocation(v8::internal::Isolate* i_isolate, Handle<Object> exc,
   // 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());
+  // TODO(szuend): Remove or re-enable the following check once it is decided
+  //               whether Script::PositionInfo.column should be the offset
+  //               relative to the module or relative to the function.
+  // CHECK_EQ(topLocation.column - 1, message->GetColumnNumber());
 }
 
 #undef CHECK_CSTREQ
@@ -139,11 +142,11 @@ WASM_EXEC_TEST(CollectDetailedWasmStack_ExplicitThrowFromJs) {
 
   // Line and column are 1-based, so add 1 for the expected wasm output.
   ExceptionInfo expected_exceptions[] = {
-      {"a", 3, 8},                                           // -
-      {"js", 4, 2},                                          // -
-      {"main", static_cast<int>(wasm_index_1) + 1, 3},       // -
-      {"call_main", static_cast<int>(wasm_index_2) + 1, 2},  // -
-      {"callFn", 1, 24}                                      // -
+      {"a", 3, 8},                                            // -
+      {"js", 4, 2},                                           // -
+      {"main", static_cast<int>(wasm_index_1) + 1, 8},        // -
+      {"call_main", static_cast<int>(wasm_index_2) + 1, 21},  // -
+      {"callFn", 1, 24}                                       // -
   };
   CheckExceptionInfos(isolate, maybe_exc.ToHandleChecked(),
                       expected_exceptions);
@@ -188,12 +191,20 @@ WASM_EXEC_TEST(CollectDetailedWasmStack_WasmError) {
     Handle<Object> exception = maybe_exc.ToHandleChecked();
 
     static constexpr int kMainLocalsLength = 1;
+    const int main_offset =
+        r.builder().GetFunctionAt(wasm_index_1)->code.offset();
+    const int call_main_offset =
+        r.builder().GetFunctionAt(wasm_index_2)->code.offset();
+
     // Line and column are 1-based, so add 1 for the expected wasm output.
-    const int expected_main_pos = unreachable_pos + kMainLocalsLength + 1;
+    const int expected_main_pos =
+        unreachable_pos + main_offset + kMainLocalsLength + 1;
+    const int expected_call_main_pos = call_main_offset + 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}                                                 //-
+        {"call_main", static_cast<int>(wasm_index_2) + 1,
+         expected_call_main_pos},  // -
+        {"callFn", 1, 24}          //-
     };
     CheckExceptionInfos(isolate, exception, expected_exceptions);
   }

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

@@ -93,7 +93,7 @@ WASM_EXEC_TEST(Unreachable) {
 
   // Line and column are 1-based, so add 1 for the expected wasm output.
   ExceptionInfo expected_exceptions[] = {
-      {"main", static_cast<int>(wasm_index) + 1, 2},  // --
+      {"main", static_cast<int>(wasm_index) + 1, 7},  // --
       {"callFn", 1, 24}                               // --
   };
   CheckExceptionInfos(isolate, maybe_exc.ToHandleChecked(),
@@ -136,9 +136,9 @@ WASM_EXEC_TEST(IllegalLoad) {
 
   // 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, 8},       // --
-      {"call_main", static_cast<int>(wasm_index_2) + 1, 3},  // --
-      {"callFn", 1, 24}                                      // --
+      {"main", static_cast<int>(wasm_index_1) + 1, 13},       // --
+      {"call_main", static_cast<int>(wasm_index_2) + 1, 30},  // --
+      {"callFn", 1, 24}                                       // --
   };
   CheckExceptionInfos(isolate, maybe_exc.ToHandleChecked(),
                       expected_exceptions);