[wasm] Fix interpreter exception stack height change.

This fixes how the interpreter modifies the operand stack in the case
calls with non-zero parameter or return count throw an exception. The
interpreter raises the exception before arguments are popped and before
results are pushed onto the stack. This makes the control transfer
analysis fit this model. It also makes the tests trigger this aspect.

R=clemensh@chromium.org
TEST=cctest/test-run-wasm-exceptions
BUG=v8:8091

Change-Id: I001fc4bc0030393d3c97be3fa9425bc540575071
Reviewed-on: https://chromium-review.googlesource.com/c/1445972
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59212}

src/wasm/wasm-interpreter.cc

@@ -777,6 +777,7 @@ class SideTable : public ZoneObject {
     for (BytecodeIterator i(code->orig_start, code->orig_end, &code->locals);
          i.has_next(); i.next()) {
       WasmOpcode opcode = i.current();
+      uint32_t exceptional_stack_height = 0;
       if (WasmOpcodes::IsPrefixOpcode(opcode)) opcode = i.prefixed_opcode();
       bool unreachable = control_stack.back().unreachable;
       if (unreachable) {
@@ -791,9 +792,20 @@ class SideTable : public ZoneObject {
         DCHECK_GE(stack_height, stack_effect.first);
         DCHECK_GE(kMaxUInt32, static_cast<uint64_t>(stack_height) -
                                   stack_effect.first + stack_effect.second);
+        exceptional_stack_height = stack_height - stack_effect.first;
         stack_height = stack_height - stack_effect.first + stack_effect.second;
         if (stack_height > max_stack_height_) max_stack_height_ = stack_height;
       }
+      if (!exception_stack.empty() && WasmOpcodes::IsThrowingOpcode(opcode)) {
+        // Record exceptional control flow from potentially throwing opcodes to
+        // the local handler if one is present. The stack height at the throw
+        // point is assumed to have popped all operands and not pushed any yet.
+        DCHECK_GE(control_stack.size() - 1, exception_stack.back());
+        const Control* c = &control_stack[exception_stack.back()];
+        if (!unreachable) c->else_label->Ref(i.pc(), exceptional_stack_height);
+        TRACE("handler @%u: %s -> try @%u\n", i.pc_offset(), OpcodeName(opcode),
+              static_cast<uint32_t>(c->pc - code->start));
+      }
       switch (opcode) {
         case kExprBlock:
         case kExprLoop: {
@@ -881,21 +893,6 @@ class SideTable : public ZoneObject {
           stack_height = c->end_label->target_stack_height + kCatchInArity;
           break;
         }
-        case kExprThrow:
-        case kExprRethrow:
-        case kExprCallFunction: {
-          if (exception_stack.empty()) break;  // Nothing to do here.
-          // TODO(mstarzinger): The same needs to be done for calls, not only
-          // for "throw" and "rethrow". Factor this logic out accordingly.
-          // TODO(mstarzinger): For calls the stack height here is off when the
-          // callee either consumes or produces stack values. Test and fix!
-          DCHECK_GE(control_stack.size() - 1, exception_stack.back());
-          Control* c = &control_stack[exception_stack.back()];
-          if (!unreachable) c->else_label->Ref(i.pc(), stack_height);
-          TRACE("handler @%u: %s -> try @%u\n", i.pc_offset(),
-                OpcodeName(opcode), static_cast<uint32_t>(c->pc - code->start));
-          break;
-        }
         case kExprEnd: {
           Control* c = &control_stack.back();
           TRACE("control @%u: End\n", i.pc_offset());
@@ -2350,12 +2347,13 @@ class ThreadImpl {
 
 #ifdef DEBUG
       // Compute the stack effect of this opcode, and verify later that the
-      // stack was modified accordingly.
+      // stack was modified accordingly (unless an exception was thrown).
       std::pair<uint32_t, uint32_t> stack_effect =
           StackEffect(codemap_->module(), frames_.back().code->function->sig,
                       code->orig_start + pc, code->orig_end);
       sp_t expected_new_stack_height =
           StackHeight() - stack_effect.first + stack_effect.second;
+      bool exception_was_thrown = false;
 #endif
 
       switch (orig) {
@@ -2536,6 +2534,7 @@ class ThreadImpl {
                 return;
               case ExternalCallResult::EXTERNAL_CAUGHT:
                 len = JumpToHandlerDelta(code, pc);
+                DCHECK(exception_was_thrown = true);
                 break;
             }
             if (result.type != ExternalCallResult::INTERNAL) break;
@@ -2575,6 +2574,7 @@ class ThreadImpl {
               return;
             case ExternalCallResult::EXTERNAL_CAUGHT:
               len = JumpToHandlerDelta(code, pc);
+              DCHECK(exception_was_thrown = true);
               break;
           }
         } break;
@@ -2823,7 +2823,7 @@ class ThreadImpl {
       }
 
 #ifdef DEBUG
-      if (!WasmOpcodes::IsControlOpcode(opcode)) {
+      if (!WasmOpcodes::IsControlOpcode(opcode) && !exception_was_thrown) {
         DCHECK_EQ(expected_new_stack_height, StackHeight());
       }
 #endif
@@ -3030,6 +3030,9 @@ class ThreadImpl {
     TRACE("  => External wasm function returned%s\n",
           maybe_retval.is_null() ? " with exception" : "");
 
+    // Pop arguments off the stack.
+    sp_ -= num_args;
+
     if (maybe_retval.is_null()) {
       // JSEntry may throw a stack overflow before we actually get to wasm code
       // or back to the interpreter, meaning the thread-in-wasm flag won't be
@@ -3042,8 +3045,6 @@ class ThreadImpl {
 
     trap_handler::ClearThreadInWasm();
 
-    // Pop arguments off the stack.
-    sp_ -= num_args;
     // Push return values.
     if (sig->return_count() > 0) {
       // TODO(wasm): Handle multiple returns.

src/wasm/wasm-opcodes.cc

@@ -368,6 +368,19 @@ bool WasmOpcodes::IsAnyRefOpcode(WasmOpcode opcode) {
   }
 }
 
+bool WasmOpcodes::IsThrowingOpcode(WasmOpcode opcode) {
+  // TODO(8729): Trapping opcodes are not yet considered to be throwing.
+  switch (opcode) {
+    case kExprThrow:
+    case kExprRethrow:
+    case kExprCallFunction:
+    case kExprCallIndirect:
+      return true;
+    default:
+      return false;
+  }
+}
+
 std::ostream& operator<<(std::ostream& os, const FunctionSig& sig) {
   if (sig.return_count() == 0) os << "v";
   for (auto ret : sig.returns()) {

src/wasm/wasm-opcodes.h

@@ -586,6 +586,7 @@ class V8_EXPORT_PRIVATE WasmOpcodes {
   static bool IsControlOpcode(WasmOpcode opcode);
   static bool IsSignExtensionOpcode(WasmOpcode opcode);
   static bool IsAnyRefOpcode(WasmOpcode opcode);
+  static bool IsThrowingOpcode(WasmOpcode opcode);
   // Check whether the given opcode always jumps, i.e. all instructions after
   // this one in the current block are dead. Returns false for |end|.
   static bool IsUnconditionalJump(WasmOpcode opcode);

test/cctest/wasm/test-run-wasm-exceptions.cc

@@ -41,16 +41,18 @@ WASM_EXEC_TEST(TryCatchCallDirect) {
   constexpr uint32_t kResult1 = 42;
 
   // Build a throwing helper function.
-  WasmFunctionCompiler& throw_func = r.NewFunction<void>();
+  WasmFunctionCompiler& throw_func = r.NewFunction(sigs.i_ii());
   BUILD(throw_func, WASM_THROW(except));
 
   // Build the main test function.
   BUILD(r, WASM_TRY_CATCH_T(
                kWasmI32,
-               WASM_STMTS(
-                   WASM_I32V(kResult1),
-                   WASM_IF(WASM_I32_EQZ(WASM_GET_LOCAL(0)),
-                           WASM_CALL_FUNCTION0(throw_func.function_index()))),
+               WASM_STMTS(WASM_I32V(kResult1),
+                          WASM_IF(WASM_I32_EQZ(WASM_GET_LOCAL(0)),
+                                  WASM_STMTS(WASM_CALL_FUNCTION(
+                                                 throw_func.function_index(),
+                                                 WASM_I32V(7), WASM_I32V(9)),
+                                             WASM_DROP))),
                WASM_STMTS(WASM_DROP, WASM_I32V(kResult0))));
 
   // Need to call through JS to allow for creation of stack traces.
@@ -68,9 +70,9 @@ WASM_EXEC_TEST(TryCatchCallIndirect) {
   constexpr uint32_t kResult1 = 42;
 
   // Build a throwing helper function.
-  WasmFunctionCompiler& throw_func = r.NewFunction(sigs.v_v());
+  WasmFunctionCompiler& throw_func = r.NewFunction(sigs.i_ii());
   BUILD(throw_func, WASM_THROW(except));
-  r.builder().AddSignature(sigs.v_v());
+  r.builder().AddSignature(sigs.i_ii());
   throw_func.SetSigIndex(0);
 
   // Add an indirect function table.
@@ -85,7 +87,10 @@ WASM_EXEC_TEST(TryCatchCallIndirect) {
                kWasmI32,
                WASM_STMTS(WASM_I32V(kResult1),
                           WASM_IF(WASM_I32_EQZ(WASM_GET_LOCAL(0)),
-                                  WASM_CALL_INDIRECT0(0, WASM_GET_LOCAL(0)))),
+                                  WASM_STMTS(WASM_CALL_INDIRECT2(
+                                                 0, WASM_GET_LOCAL(0),
+                                                 WASM_I32V(7), WASM_I32V(9)),
+                                             WASM_DROP))),
                WASM_STMTS(WASM_DROP, WASM_I32V(kResult0))));
 
   // Need to call through JS to allow for creation of stack traces.
@@ -102,18 +107,22 @@ WASM_EXEC_TEST(TryCatchCallExternal) {
   Handle<JSFunction> js_function =
       Handle<JSFunction>::cast(v8::Utils::OpenHandle(
           *v8::Local<v8::Function>::Cast(CompileRun(source))));
-  ManuallyImportedJSFunction import = {sigs.v_v(), js_function};
+  ManuallyImportedJSFunction import = {sigs.i_ii(), js_function};
   WasmRunner<uint32_t, uint32_t> r(execution_tier, &import);
   constexpr uint32_t kResult0 = 23;
   constexpr uint32_t kResult1 = 42;
   constexpr uint32_t kJSFunc = 0;
 
   // Build the main test function.
-  BUILD(r, WASM_TRY_CATCH_T(kWasmI32,
-                            WASM_STMTS(WASM_I32V(kResult1),
-                                       WASM_IF(WASM_I32_EQZ(WASM_GET_LOCAL(0)),
-                                               WASM_CALL_FUNCTION0(kJSFunc))),
-                            WASM_STMTS(WASM_DROP, WASM_I32V(kResult0))));
+  BUILD(r, WASM_TRY_CATCH_T(
+               kWasmI32,
+               WASM_STMTS(
+                   WASM_I32V(kResult1),
+                   WASM_IF(WASM_I32_EQZ(WASM_GET_LOCAL(0)),
+                           WASM_STMTS(WASM_CALL_FUNCTION(kJSFunc, WASM_I32V(7),
+                                                         WASM_I32V(9)),
+                                      WASM_DROP))),
+               WASM_STMTS(WASM_DROP, WASM_I32V(kResult0))));
 
   // Need to call through JS to allow for creation of stack traces.
   r.CheckCallViaJS(kResult0, 0);