[wasm] Cleanup wasm interpreter

This is a cleanup in preparation to implement calling imported
functions via the wasm interpreter.
For imported functions, we do not create entries in the
interpreter_code_ vector any more.

I also simplified the interface and removed unused or redundant return
values. More things are now DCHECKed instead of bailing out.

Also, we previously had two PushFrame methods: One is supposed to
initialize the interpreter from external code (i.e. adds the first
frame to the stack), the other one is used to push new frames on the
frame stack for called functions. This CL renames the first to
InitFrame, and makes it use the second one. The other remaining user is
the DoCall method.

R=titzer@chromium.org
BUG=v8:5822

Change-Id: Id09ff1e3256428fbd8c955e4664507a0c3167e53
Reviewed-on: https://chromium-review.googlesource.com/453482
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#43793}

src/wasm/wasm-debug.cc

@@ -109,7 +109,7 @@ class InterpreterHandle {
     DCHECK(thread->state() == WasmInterpreter::STOPPED ||
            thread->state() == WasmInterpreter::FINISHED);
     thread->Reset();
-    thread->PushFrame(&module()->functions[func_index], wasm_args.start());
+    thread->InitFrame(&module()->functions[func_index], wasm_args.start());
     bool finished = false;
     while (!finished) {
       // TODO(clemensh): Add occasional StackChecks.

src/wasm/wasm-interpreter.cc

@@ -847,25 +847,27 @@ class CodeMap {
   CodeMap(const WasmModule* module, const uint8_t* module_start, Zone* zone)
       : zone_(zone), module_(module), interpreter_code_(zone) {
     if (module == nullptr) return;
-    for (size_t i = 0; i < module->functions.size(); ++i) {
-      const WasmFunction* function = &module->functions[i];
-      const byte* code_start = module_start + function->code_start_offset;
-      const byte* code_end = module_start + function->code_end_offset;
-      AddFunction(function, code_start, code_end);
+    interpreter_code_.reserve(module->functions.size());
+    for (const WasmFunction& function : module->functions) {
+      if (function.imported) {
+        DCHECK_EQ(function.code_start_offset, function.code_end_offset);
+        AddFunction(&function, nullptr, nullptr);
+      } else {
+        const byte* code_start = module_start + function.code_start_offset;
+        const byte* code_end = module_start + function.code_end_offset;
+        AddFunction(&function, code_start, code_end);
+      }
     }
   }
 
-  InterpreterCode* FindCode(const WasmFunction* function) {
-    if (function->func_index < interpreter_code_.size()) {
-      InterpreterCode* code = &interpreter_code_[function->func_index];
-      DCHECK_EQ(function, code->function);
-      return Preprocess(code);
-    }
-    return nullptr;
+  InterpreterCode* GetCode(const WasmFunction* function) {
+    InterpreterCode* code = GetCode(function->func_index);
+    DCHECK_EQ(function, code->function);
+    return code;
   }
 
   InterpreterCode* GetCode(uint32_t function_index) {
-    CHECK_LT(function_index, interpreter_code_.size());
+    DCHECK_LT(function_index, interpreter_code_.size());
     return Preprocess(&interpreter_code_[function_index]);
   }
 
@@ -880,7 +882,8 @@ class CodeMap {
   }
 
   InterpreterCode* Preprocess(InterpreterCode* code) {
-    if (code->targets == nullptr && code->start) {
+    DCHECK_EQ(code->function->imported, code->start == nullptr);
+    if (code->targets == nullptr && code->start != nullptr) {
       // Compute the control targets map and the local declarations.
       CHECK(DecodeLocalDecls(&code->locals, code->start, code->end));
       code->targets = new (zone_) ControlTransfers(
@@ -889,8 +892,8 @@ class CodeMap {
     return code;
   }
 
-  int AddFunction(const WasmFunction* function, const byte* code_start,
-                  const byte* code_end) {
+  void AddFunction(const WasmFunction* function, const byte* code_start,
+                   const byte* code_end) {
     InterpreterCode code = {
         function, BodyLocalDecls(zone_),         code_start,
         code_end, const_cast<byte*>(code_start), const_cast<byte*>(code_end),
@@ -898,20 +901,19 @@ class CodeMap {
 
     DCHECK_EQ(interpreter_code_.size(), function->func_index);
     interpreter_code_.push_back(code);
-    return static_cast<int>(interpreter_code_.size()) - 1;
   }
 
-  bool SetFunctionCode(const WasmFunction* function, const byte* start,
+  void SetFunctionCode(const WasmFunction* function, const byte* start,
                        const byte* end) {
-    InterpreterCode* code = FindCode(function);
-    if (code == nullptr) return false;
+    DCHECK_LT(function->func_index, interpreter_code_.size());
+    InterpreterCode* code = &interpreter_code_[function->func_index];
+    DCHECK_EQ(function, code->function);
     code->targets = nullptr;
     code->orig_start = start;
     code->orig_end = end;
     code->start = const_cast<byte*>(start);
     code->end = const_cast<byte*>(end);
     Preprocess(code);
-    return true;
   }
 };
 
@@ -932,41 +934,31 @@ class ThreadImpl {
 
   WasmInterpreter::State state() { return state_; }
 
-  void PushFrame(const WasmFunction* function, WasmVal* args) {
-    InterpreterCode* code = codemap()->FindCode(function);
-    CHECK_NOT_NULL(code);
-    ++num_interpreted_calls_;
-    frames_.push_back({code, 0, stack_.size()});
+  void InitFrame(const WasmFunction* function, WasmVal* args) {
+    InterpreterCode* code = codemap()->GetCode(function);
     for (size_t i = 0; i < function->sig->parameter_count(); ++i) {
       stack_.push_back(args[i]);
     }
-    frames_.back().pc = InitLocals(code);
-    blocks_.push_back(
-        {0, stack_.size(), frames_.size(),
-         static_cast<uint32_t>(code->function->sig->return_count())});
-    TRACE("  => PushFrame(#%u @%zu)\n", code->function->func_index,
-          frames_.back().pc);
+    PushFrame(code);
   }
 
   WasmInterpreter::State Run() {
+    DCHECK(state_ == WasmInterpreter::STOPPED ||
+           state_ == WasmInterpreter::PAUSED);
     do {
       TRACE("  => Run()\n");
-      if (state_ == WasmInterpreter::STOPPED ||
-          state_ == WasmInterpreter::PAUSED) {
-        state_ = WasmInterpreter::RUNNING;
-        Execute(frames_.back().code, frames_.back().pc, kRunSteps);
-      }
+      state_ = WasmInterpreter::RUNNING;
+      Execute(frames_.back().code, frames_.back().pc, kRunSteps);
     } while (state_ == WasmInterpreter::STOPPED);
     return state_;
   }
 
   WasmInterpreter::State Step() {
+    DCHECK(state_ == WasmInterpreter::STOPPED ||
+           state_ == WasmInterpreter::PAUSED);
     TRACE("  => Step()\n");
-    if (state_ == WasmInterpreter::STOPPED ||
-        state_ == WasmInterpreter::PAUSED) {
-      state_ = WasmInterpreter::RUNNING;
-      Execute(frames_.back().code, frames_.back().pc, 1);
-    }
+    state_ = WasmInterpreter::RUNNING;
+    Execute(frames_.back().code, frames_.back().pc, 1);
     return state_;
   }
 
@@ -1059,21 +1051,19 @@ class ThreadImpl {
   }
 
   // Push a frame with arguments already on the stack.
-  void PushFrame(InterpreterCode* code, pc_t pc) {
-    CHECK_NOT_NULL(code);
-    DCHECK(!frames_.empty());
+  void PushFrame(InterpreterCode* code) {
+    DCHECK_NOT_NULL(code);
     ++num_interpreted_calls_;
-    frames_.back().pc = pc;
     size_t arity = code->function->sig->parameter_count();
-    DCHECK_GE(stack_.size(), arity);
     // The parameters will overlap the arguments already on the stack.
+    DCHECK_GE(stack_.size(), arity);
     frames_.push_back({code, 0, stack_.size() - arity});
     blocks_.push_back(
         {0, stack_.size(), frames_.size(),
          static_cast<uint32_t>(code->function->sig->return_count())});
     frames_.back().pc = InitLocals(code);
-    TRACE("  => push func#%u @%zu\n", code->function->func_index,
-          frames_.back().pc);
+    TRACE("  => PushFrame #%zu (#%u @%zu)\n", frames_.size() - 1,
+          code->function->func_index, frames_.back().pc);
   }
 
   pc_t InitLocals(InterpreterCode* code) {
@@ -1102,9 +1092,8 @@ class ThreadImpl {
   }
 
   void CommitPc(pc_t pc) {
-    if (!frames_.empty()) {
-      frames_.back().pc = pc;
-    }
+    DCHECK(!frames_.empty());
+    frames_.back().pc = pc;
   }
 
   bool SkipBreakpoint(InterpreterCode* code, pc_t pc) {
@@ -1167,7 +1156,8 @@ class ThreadImpl {
       decoder->Reset((*code)->start, (*code)->end);
       *pc = ReturnPc(decoder, *code, top->pc);
       *limit = top->code->end - top->code->start;
-      TRACE("  => pop func#%u @%zu\n", (*code)->function->func_index, *pc);
+      TRACE("  => Return to #%zu (#%u @%zu)\n", frames_.size() - 1,
+            (*code)->function->func_index, *pc);
       DoStackTransfer(dest, arity);
       return true;
     }
@@ -1175,7 +1165,8 @@ class ThreadImpl {
 
   void DoCall(Decoder* decoder, InterpreterCode* target, pc_t* pc,
               pc_t* limit) {
-    PushFrame(target, *pc);
+    frames_.back().pc = *pc;
+    PushFrame(target);
     *pc = frames_.back().pc;
     *limit = target->end - target->start;
     decoder->Reset(target->start, target->end);
@@ -1415,6 +1406,10 @@ class ThreadImpl {
         case kExprCallFunction: {
           CallFunctionOperand operand(&decoder, code->at(pc));
           code = codemap()->GetCode(operand.index);
+          if (code->function->imported) {
+            // TODO(clemensh): Call imported function.
+            UNREACHABLE();
+          }
           DoCall(&decoder, code, &pc, &limit);
           PAUSE_IF_BREAK_FLAG(AfterCall);
           continue;
@@ -1425,9 +1420,8 @@ class ThreadImpl {
           // Assume only one table for now.
           DCHECK_LE(module()->function_tables.size(), 1u);
           InterpreterCode* target = codemap()->GetIndirectCode(0, entry_index);
-          if (target == nullptr) {
-            return DoTrap(kTrapFuncInvalid, pc);
-          } else if (target->function->sig_index != operand.index) {
+          if (target == nullptr) return DoTrap(kTrapFuncInvalid, pc);
+          if (target->function->sig_index != operand.index) {
             // If not an exact match, we have to do a canonical check.
             // TODO(titzer): make this faster with some kind of caching?
             const WasmIndirectFunctionTable* table =
@@ -1439,7 +1433,10 @@ class ThreadImpl {
               return DoTrap(kTrapFuncSigMismatch, pc);
             }
           }
-
+          if (target->function->imported) {
+            // TODO(clemensh): Call imported function.
+            UNREACHABLE();
+          }
           DoCall(&decoder, target, &pc, &limit);
           code = target;
           PAUSE_IF_BREAK_FLAG(AfterCall);
@@ -1707,7 +1704,8 @@ class ThreadImpl {
 
   void Push(pc_t pc, WasmVal val) {
     // TODO(titzer): store PC as well?
-    if (val.type != kWasmStmt) stack_.push_back(val);
+    DCHECK_NE(kWasmStmt, val.type);
+    stack_.push_back(val);
   }
 
   void TraceStack(const char* phase, pc_t pc) {
@@ -1780,9 +1778,9 @@ static ThreadImpl* ToImpl(WasmInterpreter::Thread* thread) {
 WasmInterpreter::State WasmInterpreter::Thread::state() {
   return ToImpl(this)->state();
 }
-void WasmInterpreter::Thread::PushFrame(const WasmFunction* function,
+void WasmInterpreter::Thread::InitFrame(const WasmFunction* function,
                                         WasmVal* args) {
-  return ToImpl(this)->PushFrame(function, args);
+  ToImpl(this)->InitFrame(function, args);
 }
 WasmInterpreter::State WasmInterpreter::Thread::Run() {
   return ToImpl(this)->Run();
@@ -1866,8 +1864,7 @@ void WasmInterpreter::Pause() { internals_->threads_[0].Pause(); }
 
 bool WasmInterpreter::SetBreakpoint(const WasmFunction* function, pc_t pc,
                                     bool enabled) {
-  InterpreterCode* code = internals_->codemap_.FindCode(function);
-  if (!code) return false;
+  InterpreterCode* code = internals_->codemap_.GetCode(function);
   size_t size = static_cast<size_t>(code->end - code->start);
   // Check bounds for {pc}.
   if (pc < code->locals.encoded_size || pc >= size) return false;
@@ -1887,8 +1884,7 @@ bool WasmInterpreter::SetBreakpoint(const WasmFunction* function, pc_t pc,
 }
 
 bool WasmInterpreter::GetBreakpoint(const WasmFunction* function, pc_t pc) {
-  InterpreterCode* code = internals_->codemap_.FindCode(function);
-  if (!code) return false;
+  InterpreterCode* code = internals_->codemap_.GetCode(function);
   size_t size = static_cast<size_t>(code->end - code->start);
   // Check bounds for {pc}.
   if (pc < code->locals.encoded_size || pc >= size) return false;
@@ -1923,14 +1919,14 @@ void WasmInterpreter::WriteMemory(size_t offset, WasmVal val) {
   UNIMPLEMENTED();
 }
 
-int WasmInterpreter::AddFunctionForTesting(const WasmFunction* function) {
-  return internals_->codemap_.AddFunction(function, nullptr, nullptr);
+void WasmInterpreter::AddFunctionForTesting(const WasmFunction* function) {
+  internals_->codemap_.AddFunction(function, nullptr, nullptr);
 }
 
-bool WasmInterpreter::SetFunctionCodeForTesting(const WasmFunction* function,
+void WasmInterpreter::SetFunctionCodeForTesting(const WasmFunction* function,
                                                 const byte* start,
                                                 const byte* end) {
-  return internals_->codemap_.SetFunctionCode(function, start, end);
+  internals_->codemap_.SetFunctionCode(function, start, end);
 }
 
 ControlTransferMap WasmInterpreter::ComputeControlTransfersForTesting(

src/wasm/wasm-interpreter.h

@@ -136,7 +136,7 @@ class V8_EXPORT_PRIVATE WasmInterpreter {
    public:
     // Execution control.
     State state();
-    void PushFrame(const WasmFunction* function, WasmVal* args);
+    void InitFrame(const WasmFunction* function, WasmVal* args);
     State Run();
     State Step();
     void Pause();
@@ -201,11 +201,11 @@ class V8_EXPORT_PRIVATE WasmInterpreter {
   //==========================================================================
   // Testing functionality.
   //==========================================================================
-  // Manually adds a function to this interpreter, returning the index of the
-  // function.
-  int AddFunctionForTesting(const WasmFunction* function);
+  // Manually adds a function to this interpreter. The func_index of the
+  // function must match the current number of functions.
+  void AddFunctionForTesting(const WasmFunction* function);
   // Manually adds code to the interpreter for the given function.
-  bool SetFunctionCodeForTesting(const WasmFunction* function,
+  void SetFunctionCodeForTesting(const WasmFunction* function,
                                  const byte* start, const byte* end);
 
   // Computes the control transfers for the given bytecode. Used internally in

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

@@ -197,7 +197,7 @@ TEST(Breakpoint_I32Add) {
     for (uint32_t b = 11; b < 3000000000u; b += 1000000000u) {
       thread->Reset();
       WasmVal args[] = {WasmVal(*a), WasmVal(b)};
-      thread->PushFrame(r.function(), args);
+      thread->InitFrame(r.function(), args);
 
       for (int i = 0; i < kNumBreakpoints; i++) {
         thread->Run();  // run to next breakpoint
@@ -232,7 +232,7 @@ TEST(Step_I32Mul) {
     for (uint32_t b = 33; b < 3000000000u; b += 1000000000u) {
       thread->Reset();
       WasmVal args[] = {WasmVal(*a), WasmVal(b)};
-      thread->PushFrame(r.function(), args);
+      thread->InitFrame(r.function(), args);
 
       // Run instructions one by one.
       for (int i = 0; i < kTraceLength - 1; i++) {
@@ -274,7 +274,7 @@ TEST(Breakpoint_I32And_disable) {
                                    do_break);
         thread->Reset();
         WasmVal args[] = {WasmVal(*a), WasmVal(b)};
-        thread->PushFrame(r.function(), args);
+        thread->InitFrame(r.function(), args);
 
         if (do_break) {
           thread->Run();  // run to next breakpoint

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

@@ -204,9 +204,7 @@ class TestingModule : public ModuleEnv {
     }
     instance->function_code.push_back(code);
     if (interpreter_) {
-      const WasmFunction* function = &module->functions.back();
-      int interpreter_index = interpreter_->AddFunctionForTesting(function);
-      CHECK_EQ(index, interpreter_index);
+      interpreter_->AddFunctionForTesting(&module->functions.back());
     }
     DCHECK_LT(index, kMaxFunctions);  // limited for testing.
     return index;
@@ -551,7 +549,7 @@ class WasmFunctionCompiler : private GraphAndBuilders {
 
     if (interpreter_) {
       // Add the code to the interpreter.
-      CHECK(interpreter_->SetFunctionCodeForTesting(function_, start, end));
+      interpreter_->SetFunctionCodeForTesting(function_, start, end);
       return;
     }
 
@@ -799,7 +797,7 @@ class WasmRunner : public WasmRunnerBase {
     WasmInterpreter::Thread* thread = interpreter()->GetThread(0);
     thread->Reset();
     std::array<WasmVal, sizeof...(p)> args{{WasmVal(p)...}};
-    thread->PushFrame(function(), args.data());
+    thread->InitFrame(function(), args.data());
     if (thread->Run() == WasmInterpreter::FINISHED) {
       WasmVal val = thread->GetReturnValue();
       possible_nondeterminism_ |= thread->PossibleNondeterminism();

test/common/wasm/wasm-module-runner.cc

@@ -143,7 +143,7 @@ int32_t InterpretWasmModule(Isolate* isolate, ErrorThrower* thrower,
 
   WasmInterpreter::Thread* thread = interpreter.GetThread(0);
   thread->Reset();
-  thread->PushFrame(&(module->functions[function_index]), args);
+  thread->InitFrame(&(module->functions[function_index]), args);
   WasmInterpreter::State interpreter_result = thread->Run();
   if (instance.mem_start) {
     free(instance.mem_start);