[wasm] [interpreter] Avoid double parsing of locals

The local variables were parsed two times, which in fact doubled the
amount of local variables allocated for each called function.
This was costing memory and performance. As the additional local
variables were never used, we did not recognize this before.

Add a test case for locals and stack values of interpreted frames.

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

Change-Id: Ie5cb8d8f5441edee6abb46aa6bebef4a033d582b
Reviewed-on: https://chromium-review.googlesource.com/474749
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44602}

src/wasm/function-body-decoder.cc

@@ -175,6 +175,7 @@ class WasmDecoder : public Decoder {
   static bool DecodeLocals(Decoder* decoder, const FunctionSig* sig,
                            ZoneVector<ValueType>* type_list) {
     DCHECK_NOT_NULL(type_list);
+    DCHECK_EQ(0, type_list->size());
     // Initialize from signature.
     if (sig != nullptr) {
       type_list->assign(sig->parameters().begin(), sig->parameters().end());
@@ -1721,7 +1722,7 @@ class WasmFullDecoder : public WasmDecoder {
         PrintF(", control = ");
         compiler::WasmGraphBuilder::PrintDebugName(env->control);
       }
-      PrintF("}");
+      PrintF("}\n");
     }
 #endif
     ssa_env_ = env;

src/wasm/wasm-interpreter.cc

@@ -982,7 +982,6 @@ class CodeMap {
     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(
           zone_, &code->locals, code->orig_start, code->orig_end);
     }

src/wasm/wasm-interpreter.h

@@ -57,26 +57,38 @@ struct WasmVal {
   FOREACH_UNION_MEMBER(DECLARE_CONSTRUCTOR)
 #undef DECLARE_CONSTRUCTOR
 
+  bool operator==(const WasmVal& other) const {
+    if (type != other.type) return false;
+#define CHECK_VAL_EQ(field, localtype, ctype) \
+  if (type == localtype) {                    \
+    return val.field == other.val.field;      \
+  }
+    FOREACH_UNION_MEMBER(CHECK_VAL_EQ)
+#undef CHECK_VAL_EQ
+    UNREACHABLE();
+    return false;
+  }
+
   template <typename T>
-  inline T to() {
+  inline T to() const {
     UNREACHABLE();
   }
 
   template <typename T>
-  inline T to_unchecked() {
+  inline T to_unchecked() const {
     UNREACHABLE();
   }
 };
 
-#define DECLARE_CAST(field, localtype, ctype) \
-  template <>                                 \
-  inline ctype WasmVal::to_unchecked() {      \
-    return val.field;                         \
-  }                                           \
-  template <>                                 \
-  inline ctype WasmVal::to() {                \
-    CHECK_EQ(localtype, type);                \
-    return val.field;                         \
+#define DECLARE_CAST(field, localtype, ctype)  \
+  template <>                                  \
+  inline ctype WasmVal::to_unchecked() const { \
+    return val.field;                          \
+  }                                            \
+  template <>                                  \
+  inline ctype WasmVal::to() const {           \
+    CHECK_EQ(localtype, type);                 \
+    return val.field;                          \
   }
 FOREACH_UNION_MEMBER(DECLARE_CAST)
 #undef DECLARE_CAST

test/cctest/wasm/test-wasm-breakpoints.cc

@@ -158,6 +158,109 @@ void SetBreakpoint(WasmRunnerBase& runner, int function_index, int byte_offset,
   WasmDebugInfo::SetBreakpoint(debug_info, function_index, set_byte_offset);
 }
 
+// Wrapper with operator<<.
+struct WasmValWrapper {
+  WasmVal val;
+
+  bool operator==(const WasmValWrapper& other) const {
+    return val == other.val;
+  }
+};
+
+// Only needed in debug builds. Avoid unused warning otherwise.
+#ifdef DEBUG
+std::ostream& operator<<(std::ostream& out, const WasmValWrapper& wrapper) {
+  switch (wrapper.val.type) {
+    case kWasmI32:
+      out << "i32: " << wrapper.val.to<int32_t>();
+      break;
+    case kWasmI64:
+      out << "i64: " << wrapper.val.to<int64_t>();
+      break;
+    case kWasmF32:
+      out << "f32: " << wrapper.val.to<float>();
+      break;
+    case kWasmF64:
+      out << "f64: " << wrapper.val.to<double>();
+      break;
+    default:
+      UNIMPLEMENTED();
+  }
+  return out;
+}
+#endif
+
+class CollectValuesBreakHandler : public debug::DebugDelegate {
+ public:
+  struct BreakpointValues {
+    std::vector<WasmVal> locals;
+    std::vector<WasmVal> stack;
+  };
+
+  explicit CollectValuesBreakHandler(
+      Isolate* isolate, std::initializer_list<BreakpointValues> expected_values)
+      : isolate_(isolate), expected_values_(expected_values) {
+    v8::debug::SetDebugDelegate(reinterpret_cast<v8::Isolate*>(isolate_), this);
+  }
+  ~CollectValuesBreakHandler() {
+    v8::debug::SetDebugDelegate(reinterpret_cast<v8::Isolate*>(isolate_),
+                                nullptr);
+  }
+
+ private:
+  Isolate* isolate_;
+  int count_ = 0;
+  std::vector<BreakpointValues> expected_values_;
+
+  void BreakProgramRequested(v8::Local<v8::Context> paused_context,
+                             v8::Local<v8::Object> exec_state,
+                             v8::Local<v8::Value> break_points_hit) override {
+    printf("Break #%d\n", count_);
+    CHECK_GT(expected_values_.size(), count_);
+    auto& expected = expected_values_[count_];
+    ++count_;
+
+    HandleScope handles(isolate_);
+
+    StackTraceFrameIterator frame_it(isolate_);
+    auto summ = FrameSummary::GetTop(frame_it.frame()).AsWasmInterpreted();
+    Handle<WasmInstanceObject> instance = summ.wasm_instance();
+
+    auto frame =
+        instance->debug_info()->GetInterpretedFrame(frame_it.frame()->fp(), 0);
+    CHECK_EQ(expected.locals.size(), frame->GetLocalCount());
+    for (int i = 0; i < frame->GetLocalCount(); ++i) {
+      CHECK_EQ(WasmValWrapper{expected.locals[i]},
+               WasmValWrapper{frame->GetLocalValue(i)});
+    }
+
+    CHECK_EQ(expected.stack.size(), frame->GetStackHeight());
+    for (int i = 0; i < frame->GetStackHeight(); ++i) {
+      CHECK_EQ(WasmValWrapper{expected.stack[i]},
+               WasmValWrapper{frame->GetStackValue(i)});
+    }
+
+    isolate_->debug()->PrepareStep(StepAction::StepIn);
+  }
+};
+
+// Special template to explicitly cast to WasmVal.
+template <typename Arg>
+WasmVal MakeWasmVal(Arg arg) {
+  return WasmVal(arg);
+}
+// Translate long to i64 (ambiguous otherwise).
+template <>
+WasmVal MakeWasmVal(long arg) {  // NOLINT: allow long parameter
+  return WasmVal(static_cast<int64_t>(arg));
+}
+
+template <typename... Args>
+std::vector<WasmVal> wasmVec(Args... args) {
+  std::array<WasmVal, sizeof...(args)> arr{{MakeWasmVal(args)...}};
+  return std::vector<WasmVal>{arr.begin(), arr.end()};
+}
+
 }  // namespace
 
 TEST(WasmCollectPossibleBreakpoints) {
@@ -272,3 +375,48 @@ TEST(WasmStepInAndOut) {
   CHECK(!Execution::Call(isolate, main_fun_wrapper, global, 0, nullptr)
              .is_null());
 }
+
+TEST(WasmGetLocalsAndStack) {
+  WasmRunner<void, int> runner(kExecuteCompiled);
+  runner.AllocateLocal(ValueType::kWord64);
+  runner.AllocateLocal(ValueType::kFloat32);
+  runner.AllocateLocal(ValueType::kFloat64);
+
+  BUILD(runner,
+        // set [1] to 17
+        WASM_SET_LOCAL(1, WASM_I64V_1(17)),
+        // set [2] to <arg0> = 7
+        WASM_SET_LOCAL(2, WASM_F32_SCONVERT_I32(WASM_GET_LOCAL(0))),
+        // set [3] to <arg1>/2 = 8.5
+        WASM_SET_LOCAL(3, WASM_F64_DIV(WASM_F64_SCONVERT_I64(WASM_GET_LOCAL(1)),
+                                       WASM_F64(2))));
+
+  Isolate* isolate = runner.main_isolate();
+  Handle<JSFunction> main_fun_wrapper =
+      runner.module().WrapCode(runner.function_index());
+
+  // Set breakpoint at the first instruction (7 bytes for local decls: num
+  // entries + 3x<count, type>).
+  SetBreakpoint(runner, runner.function_index(), 7, 7);
+
+  CollectValuesBreakHandler break_handler(
+      isolate,
+      {
+          // params + locals          stack
+          {wasmVec(7, 0L, 0.f, 0.), wasmVec()},          // 0: i64.const[17]
+          {wasmVec(7, 0L, 0.f, 0.), wasmVec(17L)},       // 1: set_local[1]
+          {wasmVec(7, 17L, 0.f, 0.), wasmVec()},         // 2: get_local[0]
+          {wasmVec(7, 17L, 0.f, 0.), wasmVec(7)},        // 3: f32.convert_s
+          {wasmVec(7, 17L, 0.f, 0.), wasmVec(7.f)},      // 4: set_local[2]
+          {wasmVec(7, 17L, 7.f, 0.), wasmVec()},         // 5: get_local[1]
+          {wasmVec(7, 17L, 7.f, 0.), wasmVec(17L)},      // 6: f64.convert_s
+          {wasmVec(7, 17L, 7.f, 0.), wasmVec(17.)},      // 7: f64.const[2]
+          {wasmVec(7, 17L, 7.f, 0.), wasmVec(17., 2.)},  // 8: f64.div
+          {wasmVec(7, 17L, 7.f, 0.), wasmVec(8.5)},      // 9: set_local[3]
+          {wasmVec(7, 17L, 7.f, 8.5), wasmVec()},        // 10: end
+      });
+
+  Handle<Object> global(isolate->context()->global_object(), isolate);
+  Handle<Object> args[]{handle(Smi::FromInt(7), isolate)};
+  CHECK(!Execution::Call(isolate, main_fun_wrapper, global, 1, args).is_null());
+}