[wasm] Implement an interpreter for WASM.

[wasm] Implement an interpreter for WASM.

This interpreter directly decodes and executes WASM binary code for
the purpose of supporting low-level debugging. It is not currently
integrated into the main WASM implementation.

R=ahaas@chromium.org,clemensh@chromium.org,rossberg@chromium.org,binji@chromium.org
BUG=

Committed: https://crrev.com/e4bb7ff96c463f34dc3a8aa9143d3aed57ddaa88
Cr-Commit-Position: refs/heads/master@{#36497}

[Clemens Hammacher, 2016-05-12 15:20:55]

That's an impressive piece of software! A pleasure to review :)

A few comments below.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.cc
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:84: V(I32DivS, int32_t, _)       \
The third argument is never actually used, you could drop it.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:316: V(I32Clz, uint32_t, _)            \
Same here: third argument can be dropped. Also, you could move this definition
up to the others.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:845: for (size_t i = 0; i <
interpreter_code_.size(); i++) {
In which situations is this different from GetCode(function->func_index)? In
testing only?
Anyway you could check interpreter_code_[function->func_index] first, in most
cases this will be a hit.

And you could add DCHECK_EQ(i, function->func_index) above, when filling the
interpreter_code_ vector.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:873: base::AccountingAllocator allocator;
This does not seem to have any effect.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1656: code->start =
reinterpret_cast<byte*>(malloc(size));
You should allocate this in the Zone. It would also be a leak otherwise.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1659: bool prev = code->start[pc] ==
kInternalBreakpoint;
I think the if should close here. Otherwise this function has no effect on all
but the first call per function.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1665: return prev;
So the function returns whether a breakpoint was set before? That should be
documented in the header file.

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/wasm-run-u...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/wasm-run-u...
test/cctest/wasm/wasm-run-utils.h:499: // TODO(titzer): memory leak!!!!   
delete[] start;
SetFunctionCodeForTesting could allocate memory for a copy of the code in its
zone.

https://codereview.chromium.org/1972153002/diff/1/test/unittests/wasm/control...
File test/unittests/wasm/control-transfer-unittest.cc (right):

https://codereview.chromium.org/1972153002/diff/1/test/unittests/wasm/control...
test/unittests/wasm/control-transfer-unittest.cc:391: kExprEnd       // @19
Nit: 20 ;)

[ahaas, 2016-05-13 12:18:56]

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.cc
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:250: uint32_t ia = bit_cast<uint32_t>(a) &
0x7fffffff;
There exists a {copysign} function in math.h

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:369: int32_t count = 0;
you could use bits::CountTrailingZeroes32 (bits.h) here. Same for the bit
instructions below.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:626: static const int kRunSteps = 1000;
This declaration does not belong here. Is there a reason why you picked "1000"?

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:633: ControlTransferMap map;
Can you call it map_ so that it's easier to distinguish from the local variables
which are also called {map}?

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:640: struct CRef {
The code would be easier to read if these structs were outside the constructor.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:700: size_t value_depth = 0;
Please add a comment which describes how the following algorithm works.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:982: pc_t call_pc;
Instead of storing the call_pc you could just store the result of 
{top->code->start[top->call_pc] == kExprCallIndirect || (top->code->orig_start
&& top->code->orig_start[top->call_pc] == kExprCallIndirect)}
As far as I see that's the only use of call_pc.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1092: int DoGoto(InterpreterCode* code, pc_t pc) {
Could you mention in the function name that you do not only do the goto but also
adjust the stack?

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1123: return CommitPc(pc);
It's not necessary to have the {return} and the {CommitPc} in the same line
here.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1146: return CommitPc(pc);
Same here.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1231: return CommitPc(pc);
Same here.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1295: stack_[stack_.size() - operand.arity -
1].to<uint32_t>();
Can it be that {(stack_.size() - operand.arity - 1) < 0}?

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1419: LOAD_CASE(I32LoadMem, int32_t, int32_t);
I think the I32 cases should all be together.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1423: 
is there a reason why you left out the {#undef LOAD_CASE} here? Or what is the
reason why elsewhere in V8 where they you this kind of macro pattern they always
use the #undef?

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1521: volatile ctype rval = Pop().to<ctype>();     
   \
not rval, just val.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1528: FOREACH_SIMPLE_BINOP(EXECUTE_SIMPLE_BINOP)
move the FOREACH... to the corresponding #define.

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
File test/cctest/wasm/test-run-wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-interpreter.cc:23: #define BUILD(r, ...)         
            \
why don't you use the BUILD from wasm-run-utils.h?

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-interpreter.cc:33: TEST(Run_WasmInt8Const_i) {
Except for the last test, couldn't all these tests go to test-run-wasm.cc?

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/wasm-run-u...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/wasm-run-u...
test/cctest/wasm/wasm-run-utils.h:718: } else {
if the program is more than 1000 (wasm-interpreter.cc::kRunSteps) instructions
long, then the program reaches the PAUSED state.

https://codereview.chromium.org/1972153002/diff/1/test/unittests/wasm/control...
File test/unittests/wasm/control-transfer-unittest.cc (right):

https://codereview.chromium.org/1972153002/diff/1/test/unittests/wasm/control...
test/unittests/wasm/control-transfer-unittest.cc:1: // Copyright 2015 the V8
project authors. All rights reserved.
2016

[dougc, 2016-05-13 13:23:27]

dtc-v8@scieneer.com changed reviewers:
+ dtc-v8@scieneer.com

[dougc, 2016-05-13 13:23:28]

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.cc
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:794: break;
Might the value_depth need to be checked against the depth on entry to the block
to ensure it does not underflow?

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1548: return val;
These appear to be debug checks, but does it need runtime checks to ensure that
the code is valid and does not attempt to pop past a block value stack boundary?

[titzer, 2016-05-23 11:41:39]

PTAL.

I've added dual-mode execution for all WASM_EXEC_TESTS now.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.cc
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:84: V(I32DivS, int32_t, _)       \
On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> The third argument is never actually used, you could drop it.

Good catch. Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:250: uint32_t ia = bit_cast<uint32_t>(a) &
0x7fffffff;
On 2016/05/13 12:18:55, ahaas wrote:
> There exists a {copysign} function in math.h

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:316: V(I32Clz, uint32_t, _)            \
On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> Same here: third argument can be dropped. Also, you could move this definition
> up to the others.

The distinction between UNOP and BINOP operators is how much is popped off the
stack and passed to the evaluator function.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:369: int32_t count = 0;
On 2016/05/13 12:18:55, ahaas wrote:
> you could use bits::CountTrailingZeroes32 (bits.h) here. Same for the bit
> instructions below.

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:633: ControlTransferMap map;
On 2016/05/13 12:18:55, ahaas wrote:
> Can you call it map_ so that it's easier to distinguish from the local
variables
> which are also called {map}?

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:640: struct CRef {
On 2016/05/13 12:18:55, ahaas wrote:
> The code would be easier to read if these structs were outside the
constructor.

I can see that, but this way the are not visible outside of it.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:700: size_t value_depth = 0;
On 2016/05/13 12:18:54, ahaas wrote:
> Please add a comment which describes how the following algorithm works. 

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:845: for (size_t i = 0; i <
interpreter_code_.size(); i++) {
On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> In which situations is this different from GetCode(function->func_index)? In
> testing only?
> Anyway you could check interpreter_code_[function->func_index] first, in most
> cases this will be a hit.
> 
> And you could add DCHECK_EQ(i, function->func_index) above, when filling the
> interpreter_code_ vector.

Good catch. I think this was written before function->func_index existed. I put
in the fast path check but will leave the search in for a backup now.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:873: base::AccountingAllocator allocator;
On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> This does not seem to have any effect.

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:982: pc_t call_pc;
On 2016/05/13 12:18:55, ahaas wrote:
> Instead of storing the call_pc you could just store the result of 
> {top->code->start[top->call_pc] == kExprCallIndirect || (top->code->orig_start
> && top->code->orig_start[top->call_pc] == kExprCallIndirect)}
> As far as I see that's the only use of call_pc.

Ah. I was going to expose the "caller PC" to the debugger so that it know the
exact call bytecode to display. The ret_pc is basically for the interpreter to
know where to jump upon a return.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1092: int DoGoto(InterpreterCode* code, pc_t pc) {
On 2016/05/13 12:18:54, ahaas wrote:
> Could you mention in the function name that you do not only do the goto but
also
> adjust the stack?

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1123: return CommitPc(pc);
On 2016/05/13 12:18:55, ahaas wrote:
> It's not necessary to have the {return} and the {CommitPc} in the same line
> here.

Yeah, but it saves a line :-)

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1146: return CommitPc(pc);
On 2016/05/13 12:18:55, ahaas wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1231: return CommitPc(pc);
On 2016/05/13 12:18:55, ahaas wrote:
> Same here.

Acknowledged.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1295: stack_[stack_.size() - operand.arity -
1].to<uint32_t>();
On 2016/05/13 12:18:55, ahaas wrote:
> Can it be that {(stack_.size() - operand.arity - 1) < 0}?

That could result from invalid bytecode or an interpreter bug. Added a DCHECK.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1419: LOAD_CASE(I32LoadMem, int32_t, int32_t);
On 2016/05/13 12:18:55, ahaas wrote:
> I think the I32 cases should all be together.

They are in the same order as the opcode declarations.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1423: 
On 2016/05/13 12:18:55, ahaas wrote:
> is there a reason why you left out the {#undef LOAD_CASE} here? Or what is the
> reason why elsewhere in V8 where they you this kind of macro pattern they
always
> use the #undef?

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1521: volatile ctype rval = Pop().to<ctype>();     
   \
On 2016/05/13 12:18:55, ahaas wrote:
> not rval, just val.

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1528: FOREACH_SIMPLE_BINOP(EXECUTE_SIMPLE_BINOP)
On 2016/05/13 12:18:55, ahaas wrote:
> move the FOREACH... to the corresponding #define.

Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1548: return val;
On 2016/05/13 13:23:27, dougc wrote:
> These appear to be debug checks, but does it need runtime checks to ensure
that
> the code is valid and does not attempt to pop past a block value stack
boundary?

The interpreter is not designed to catch validation errors. It assumes the code
is verified beforehand.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1656: code->start =
reinterpret_cast<byte*>(malloc(size));
On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> You should allocate this in the Zone. It would also be a leak otherwise.

Good catch. Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1659: bool prev = code->start[pc] ==
kInternalBreakpoint;
On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> I think the if should close here. Otherwise this function has no effect on all
> but the first call per function.

Good catch. Done.

https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1665: return prev;
On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> So the function returns whether a breakpoint was set before? That should be
> documented in the header file.

Done.

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
File test/cctest/wasm/test-run-wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-interpreter.cc:23: #define BUILD(r, ...)         
            \
On 2016/05/13 12:18:55, ahaas wrote:
> why don't you use the BUILD from wasm-run-utils.h?

Done.

https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
test/cctest/wasm/test-run-wasm-interpreter.cc:33: TEST(Run_WasmInt8Const_i) {
On 2016/05/13 12:18:55, ahaas wrote:
> Except for the last test, couldn't all these tests go to test-run-wasm.cc?

Actually, I am still not sure what to do here, since currently the tests
statically switch to the interpreter or the compiler with an #ifdef. It's likely
that the tests need significant refactoring to make them work in either
scenario. Until then I think I will add a few more tests here and make
test-run-wasm.cc continue to exercise the compiler.

[titzer, 2016-05-24 09:23:10]

On 2016/05/23 11:41:39, titzer wrote:
> PTAL.
> 
> I've added dual-mode execution for all WASM_EXEC_TESTS now.
> 
> https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.cc
> File src/wasm/wasm-interpreter.cc (right):
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:84: V(I32DivS, int32_t, _)       \
> On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> > The third argument is never actually used, you could drop it.
> 
> Good catch. Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:250: uint32_t ia = bit_cast<uint32_t>(a) &
> 0x7fffffff;
> On 2016/05/13 12:18:55, ahaas wrote:
> > There exists a {copysign} function in math.h
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:316: V(I32Clz, uint32_t, _)            \
> On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> > Same here: third argument can be dropped. Also, you could move this
definition
> > up to the others.
> 
> The distinction between UNOP and BINOP operators is how much is popped off the
> stack and passed to the evaluator function.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:369: int32_t count = 0;
> On 2016/05/13 12:18:55, ahaas wrote:
> > you could use bits::CountTrailingZeroes32 (bits.h) here. Same for the bit
> > instructions below.
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:633: ControlTransferMap map;
> On 2016/05/13 12:18:55, ahaas wrote:
> > Can you call it map_ so that it's easier to distinguish from the local
> variables
> > which are also called {map}?
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:640: struct CRef {
> On 2016/05/13 12:18:55, ahaas wrote:
> > The code would be easier to read if these structs were outside the
> constructor.
> 
> I can see that, but this way the are not visible outside of it.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:700: size_t value_depth = 0;
> On 2016/05/13 12:18:54, ahaas wrote:
> > Please add a comment which describes how the following algorithm works. 
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:845: for (size_t i = 0; i <
> interpreter_code_.size(); i++) {
> On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> > In which situations is this different from GetCode(function->func_index)? In
> > testing only?
> > Anyway you could check interpreter_code_[function->func_index] first, in
most
> > cases this will be a hit.
> > 
> > And you could add DCHECK_EQ(i, function->func_index) above, when filling the
> > interpreter_code_ vector.
> 
> Good catch. I think this was written before function->func_index existed. I
put
> in the fast path check but will leave the search in for a backup now.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:873: base::AccountingAllocator allocator;
> On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> > This does not seem to have any effect.
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:982: pc_t call_pc;
> On 2016/05/13 12:18:55, ahaas wrote:
> > Instead of storing the call_pc you could just store the result of 
> > {top->code->start[top->call_pc] == kExprCallIndirect ||
(top->code->orig_start
> > && top->code->orig_start[top->call_pc] == kExprCallIndirect)}
> > As far as I see that's the only use of call_pc.
> 
> Ah. I was going to expose the "caller PC" to the debugger so that it know the
> exact call bytecode to display. The ret_pc is basically for the interpreter to
> know where to jump upon a return.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1092: int DoGoto(InterpreterCode* code, pc_t pc)
{
> On 2016/05/13 12:18:54, ahaas wrote:
> > Could you mention in the function name that you do not only do the goto but
> also
> > adjust the stack?
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1123: return CommitPc(pc);
> On 2016/05/13 12:18:55, ahaas wrote:
> > It's not necessary to have the {return} and the {CommitPc} in the same line
> > here.
> 
> Yeah, but it saves a line :-)
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1146: return CommitPc(pc);
> On 2016/05/13 12:18:55, ahaas wrote:
> > Same here.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1231: return CommitPc(pc);
> On 2016/05/13 12:18:55, ahaas wrote:
> > Same here.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1295: stack_[stack_.size() - operand.arity -
> 1].to<uint32_t>();
> On 2016/05/13 12:18:55, ahaas wrote:
> > Can it be that {(stack_.size() - operand.arity - 1) < 0}?
> 
> That could result from invalid bytecode or an interpreter bug. Added a DCHECK.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1419: LOAD_CASE(I32LoadMem, int32_t, int32_t);
> On 2016/05/13 12:18:55, ahaas wrote:
> > I think the I32 cases should all be together.
> 
> They are in the same order as the opcode declarations.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1423: 
> On 2016/05/13 12:18:55, ahaas wrote:
> > is there a reason why you left out the {#undef LOAD_CASE} here? Or what is
the
> > reason why elsewhere in V8 where they you this kind of macro pattern they
> always
> > use the #undef?
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1521: volatile ctype rval = Pop().to<ctype>();   
 
>    \
> On 2016/05/13 12:18:55, ahaas wrote:
> > not rval, just val.
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1528: FOREACH_SIMPLE_BINOP(EXECUTE_SIMPLE_BINOP)
> On 2016/05/13 12:18:55, ahaas wrote:
> > move the FOREACH... to the corresponding #define.
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1548: return val;
> On 2016/05/13 13:23:27, dougc wrote:
> > These appear to be debug checks, but does it need runtime checks to ensure
> that
> > the code is valid and does not attempt to pop past a block value stack
> boundary?
> 
> The interpreter is not designed to catch validation errors. It assumes the
code
> is verified beforehand.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1656: code->start =
> reinterpret_cast<byte*>(malloc(size));
> On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> > You should allocate this in the Zone. It would also be a leak otherwise.
> 
> Good catch. Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1659: bool prev = code->start[pc] ==
> kInternalBreakpoint;
> On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> > I think the if should close here. Otherwise this function has no effect on
all
> > but the first call per function.
> 
> Good catch. Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> src/wasm/wasm-interpreter.cc:1665: return prev;
> On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> > So the function returns whether a breakpoint was set before? That should be
> > documented in the header file.
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
> File test/cctest/wasm/test-run-wasm-interpreter.cc (right):
> 
>
https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
> test/cctest/wasm/test-run-wasm-interpreter.cc:23: #define BUILD(r, ...)       
 
>             \
> On 2016/05/13 12:18:55, ahaas wrote:
> > why don't you use the BUILD from wasm-run-utils.h?
> 
> Done.
> 
>
https://codereview.chromium.org/1972153002/diff/1/test/cctest/wasm/test-run-w...
> test/cctest/wasm/test-run-wasm-interpreter.cc:33: TEST(Run_WasmInt8Const_i) {
> On 2016/05/13 12:18:55, ahaas wrote:
> > Except for the last test, couldn't all these tests go to test-run-wasm.cc?
> 
> Actually, I am still not sure what to do here, since currently the tests
> statically switch to the interpreter or the compiler with an #ifdef. It's
likely
> that the tests need significant refactoring to make them work in either
> scenario. Until then I think I will add a few more tests here and make
> test-run-wasm.cc continue to exercise the compiler.

Above comment has been addressed with the dual-mode execution macros.

[Clemens Hammacher, 2016-05-24 12:47:03]

>
https://codereview.chromium.org/1972153002/diff/1/src/wasm/wasm-interpreter.c...
> > src/wasm/wasm-interpreter.cc:316: V(I32Clz, uint32_t, _)            \
> > On 2016/05/12 15:20:55, Clemens Hammacher wrote:
> > > Same here: third argument can be dropped. Also, you could move this
> definition
> > > up to the others.
> > 
> > The distinction between UNOP and BINOP operators is how much is popped off
the
> > stack and passed to the evaluator function.

I just meant moving the #define up to the other #defines, not merging them.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:862: if (function->func_index >
interpreter_code_.size()) {
You wanted to check for "<".

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:864: if (code && code->function == function) return
code;
Why the null-check here, and not below?
Also, you probably want to Preprocess(code).

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:44: #define WASM_INTERPRETER 1
This can be removed now.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:185: WasmFunction* function =
&module->functions.back();
This can be moved inside the if.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:510: int function_index() { return
static_cast<int>(function_->func_index); }
You could use this function in all the code below.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:547: // the
Huh, what happened here? The "the" should still fit in the first line.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:714: ReturnType CallInterpreter(WasmVal* args,
int count) {
Looks like a candidate for Vector<WasmVal*>.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:768: void RunWasm_##name(WasmExecutionMode
execution_mode)
Passing this execution_mode explicitely adds a ton of code, and can easily be
forgotton. How about setting some static field somewhere, maybe scoped, and then
using this in WasmRunner and TestingModule when no execution mode is explicitely
given?

[ahaas, 2016-05-24 14:48:27]

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:118: if (b == -1 && a == 0x80000000) {
Use std::numeric_limits<int32_t>::min() here.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:170: if (b == -1 && a == 0x8000000000000000ULL) {
Use std::numeric_limits<int64_t>::min() here.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:296: if (b == -1 && a == 0x80000000) return
static_cast<int32_t>(0x80000000);
Use std::numeric_limits<int32_t>::min() here.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:862: if (function->func_index >
interpreter_code_.size()) {
Do you mean "<"?

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:864: if (code && code->function == function) return
code;
What's the reason why you don't return Preprocess(code)?

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:884: return Preprocess(&interpreter_code_[index]);
I think "return GetCode(index);" would be nicer here.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:903: 
Is it true that you assume that function->func_index == interpreter_code_.size()
here? If yes, you could add a DCHECK here. If no, then I don't understand your
optimization in {FindCode}.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1044: frames_.back().ret_pc = InitLocals(code);
Could you add a comment that you set {ret_pc} in a separate line because
InitLocals changes the stack?

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1106: // UGLY: An indirect call has the additional
function index on the
Why can't you pop the function index before the call?

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1255: decoder.Reset(code->start, code->end);
Could {decoder.Reset} be called in {DoReturn}?

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1315: code = target;
You could make a DoCall function for these lines.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:547: // the
Fix the line break.

https://codereview.chromium.org/1972153002/diff/240001/test/unittests/wasm/co...
File test/unittests/wasm/control-transfer-unittest.cc (right):

https://codereview.chromium.org/1972153002/diff/240001/test/unittests/wasm/co...
test/unittests/wasm/control-transfer-unittest.cc:1: // Copyright 2015 the V8
project authors. All rights reserved.
2016

https://codereview.chromium.org/1972153002/diff/240001/test/unittests/wasm/co...
test/unittests/wasm/control-transfer-unittest.cc:148: kExprIf,        // @1
should this be @2?

[titzer, 2016-05-24 14:50:48]

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:862: if (function->func_index >
interpreter_code_.size()) {
On 2016/05/24 12:47:03, Clemens Hammacher wrote:
> You wanted to check for "<".

Done.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:864: if (code && code->function == function) return
code;
On 2016/05/24 12:47:03, Clemens Hammacher wrote:
> Why the null-check here, and not below?
> Also, you probably want to Preprocess(code).

I've changed this function so that it no longer searches. Preprocess(code) is
done by the callers that need it (e.g. SetBreakpoint and GetBreakpoint) don't
need it.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:44: #define WASM_INTERPRETER 1
On 2016/05/24 12:47:03, Clemens Hammacher wrote:
> This can be removed now.

Done.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:185: WasmFunction* function =
&module->functions.back();
On 2016/05/24 12:47:03, Clemens Hammacher wrote:
> This can be moved inside the if.

Done.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:510: int function_index() { return
static_cast<int>(function_->func_index); }
On 2016/05/24 12:47:03, Clemens Hammacher wrote:
> You could use this function in all the code below.

Done.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:547: // the
On 2016/05/24 12:47:03, Clemens Hammacher wrote:
> Huh, what happened here? The "the" should still fit in the first line.

Done.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:714: ReturnType CallInterpreter(WasmVal* args,
int count) {
On 2016/05/24 12:47:03, Clemens Hammacher wrote:
> Looks like a candidate for Vector<WasmVal*>.

Done.

https://codereview.chromium.org/1972153002/diff/240001/test/cctest/wasm/wasm-...
test/cctest/wasm/wasm-run-utils.h:768: void RunWasm_##name(WasmExecutionMode
execution_mode)
On 2016/05/24 12:47:03, Clemens Hammacher wrote:
> Passing this execution_mode explicitely adds a ton of code, and can easily be
> forgotton. How about setting some static field somewhere, maybe scoped, and
then
> using this in WasmRunner and TestingModule when no execution mode is
explicitely
> given?

Explicit is better than relying on global state, even if that state is scoped.

[titzer, 2016-05-24 15:09:52]

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:118: if (b == -1 && a == 0x80000000) {
On 2016/05/24 14:48:26, ahaas wrote:
> Use std::numeric_limits<int32_t>::min() here.

Done.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:170: if (b == -1 && a == 0x8000000000000000ULL) {
On 2016/05/24 14:48:26, ahaas wrote:
> Use std::numeric_limits<int64_t>::min() here.

Done.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:296: if (b == -1 && a == 0x80000000) return
static_cast<int32_t>(0x80000000);
On 2016/05/24 14:48:27, ahaas wrote:
> Use std::numeric_limits<int32_t>::min() here.

Done.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:884: return Preprocess(&interpreter_code_[index]);
On 2016/05/24 14:48:26, ahaas wrote:
> I think "return GetCode(index);" would be nicer here.

Done.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:903: 
On 2016/05/24 14:48:26, ahaas wrote:
> Is it true that you assume that function->func_index ==
interpreter_code_.size()
> here? If yes, you could add a DCHECK here. If no, then I don't understand your
> optimization in {FindCode}.

Done.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1044: frames_.back().ret_pc = InitLocals(code);
On 2016/05/24 14:48:26, ahaas wrote:
> Could you add a comment that you set {ret_pc} in a separate line because
> InitLocals changes the stack?

InitLocals() changes the value stack, but not the frame stack.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1106: // UGLY: An indirect call has the additional
function index on the
On 2016/05/24 14:48:26, ahaas wrote:
> Why can't you pop the function index before the call?

Because the arguments remain on the stack too, so I'd have to pop them all and
them push them all back.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1255: decoder.Reset(code->start, code->end);
On 2016/05/24 14:48:27, ahaas wrote:
> Could {decoder.Reset} be called in {DoReturn}? 

We also do a decoder.Reset() on calls, so I'll keep this symmetric. There might
be a way to clean up the code a bit to make this less manual.

https://codereview.chromium.org/1972153002/diff/240001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1315: code = target;
On 2016/05/24 14:48:26, ahaas wrote:
> You could make a DoCall function for these lines.

Done.

https://codereview.chromium.org/1972153002/diff/240001/test/unittests/wasm/co...
File test/unittests/wasm/control-transfer-unittest.cc (right):

https://codereview.chromium.org/1972153002/diff/240001/test/unittests/wasm/co...
test/unittests/wasm/control-transfer-unittest.cc:1: // Copyright 2015 the V8
project authors. All rights reserved.
On 2016/05/24 14:48:27, ahaas wrote:
> 2016

Done.

https://codereview.chromium.org/1972153002/diff/240001/test/unittests/wasm/co...
test/unittests/wasm/control-transfer-unittest.cc:148: kExprIf,        // @1
On 2016/05/24 14:48:27, ahaas wrote:
> should this be @2?

Done.

[ahaas, 2016-05-24 16:25:15]

lgtm

[titzer, 2016-05-25 08:23:53]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-05-25 08:23:54]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/1972153002/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-25 08:24:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972153002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972153002/300001

[commit-bot: I haz the power, 2016-05-25 08:32:48]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-05-25 08:33:21]

Description was changed from

==========
[wasm] Implement an interpreter for WASM.

This interpreter directly decodes and executes WASM binary code for
the purpose of supporting low-level debugging. It is not currently
integrated into the main WASM implementation.

R=ahaas@chromium.org,clemensh@chromium.org,rossberg@chromium.org,binji@chromi...
BUG=
==========

to

==========
[wasm] Implement an interpreter for WASM.

This interpreter directly decodes and executes WASM binary code for
the purpose of supporting low-level debugging. It is not currently
integrated into the main WASM implementation.

R=ahaas@chromium.org,clemensh@chromium.org,rossberg@chromium.org,binji@chromi...
BUG=

Committed: https://crrev.com/e4bb7ff96c463f34dc3a8aa9143d3aed57ddaa88
Cr-Commit-Position: refs/heads/master@{#36497}
==========

[commit-bot: I haz the power, 2016-05-25 08:33:22]

Patchset 16 (id:??) landed as
https://crrev.com/e4bb7ff96c463f34dc3a8aa9143d3aed57ddaa88
Cr-Commit-Position: refs/heads/master@{#36497}

[Michael Achenbach, 2016-06-06 14:34:02]

FYI: Our cfi bot failed after this CL:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20cfi/builds...

Fix desired, but not super-urgent...

[Michael Achenbach, 2016-06-08 14:34:04]

On 2016/06/06 14:34:02, Michael Achenbach wrote:
> FYI: Our cfi bot failed after this CL:
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20cfi/builds...
> 
> Fix desired, but not super-urgent...

Of course, the bot is now very red and we won't spot any other cfi failures
easily. What would be the ETA of a fix?