Subzero. WASM. Additional progress.

Subzero. WASM. Additional progress.

This change includes a number of improvements since the last WASM
CL. It compiles against a newer version of V8 that matches the current
WASM binary format. Many more WASM instructions are supported, as well
as global variable initializers.

There is also the beginning of a runtime library that implements some
system calls required by the WASM C library. The c2wasm-exe.sh script
can be used to compile a C program to a .wasm module, which is then
compiled by Subzero into a native executable.

This change includes a new Breakpoint instruction, which inserts an
unconditional breakpoint into the executable. This has been helpful in
debugging code generation for some WASM instructions. The Breakpoint
instruction is only completely implemented on X86.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4369
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=67c7c4161ad5bcf234357348a421e313932a8ee3

[Eric Holk, 2016-04-12 02:12:56]

eholk@chromium.org changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, sehr@chromium.org,
stichnot@chromium.org

[John, 2016-04-12 20:34:28]

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.h#newcode981
src/IceInst.h:981: ///
What's the use case for this? I would think that simply emitting an int3/ud2
directly is enough for your debugging needs...

[Eric Holk, 2016-04-12 21:30:10]

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.h#newcode981
src/IceInst.h:981: ///
On 2016/04/12 20:34:27, John wrote:
> What's the use case for this? I would think that simply emitting an int3/ud2
> directly is enough for your debugging needs...

I used it when I was debugging indirect calls. I had the Wasm translator insert
an int3 right before the indirect call started to make it easier to know which
generated code corresponded to that instruction.

Jim suggested adding some command-line arguments to insert breakpoints, such as
something like --break-before-inst=NNN, to insert a breakpoint before
instruction number NNN.

This high level instruction helps to control where the breakpoint happens
relative to the high level instructions. I think most of this information is
lost by the time we get to the emitter.

[Jim Stichnoth, 2016-04-14 20:03:45]

https://codereview.chromium.org/1876413002/diff/40001/c2wasm-exe.sh
File c2wasm-exe.sh (right):

https://codereview.chromium.org/1876413002/diff/40001/c2wasm-exe.sh#newcode1
c2wasm-exe.sh:1: #!/bin/bash
This is a pretty hacky script, I'd be happier with a TODO comment to retire the
script.

https://codereview.chromium.org/1876413002/diff/40001/c2wasm-exe.sh#newcode3
c2wasm-exe.sh:3: ./wasm-install/bin/emscripten/emcc $1 -s BINARYEN=1 -s
'BINARYEN_METHOD="native-wasm"' -O2 && \
80-col

You may want to put double-quotes around "$1" to be safer.

https://codereview.chromium.org/1876413002/diff/40001/fetch-torture-tests.sh
File fetch-torture-tests.sh (right):

https://codereview.chromium.org/1876413002/diff/40001/fetch-torture-tests.sh#...
fetch-torture-tests.sh:7: rm wasm-torture-s-$BUILDID.tbz2
How about something like this, to avoid temp files:

  wget -O - https://... | tar xj

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c
File runtime/wasm-runtime.c (right):

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:10: // This file implements the system calls required the
the libc that is included
the the ==> in the
?

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:16: #include <stdio.h>
sort?

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:20: extern char WASM_MEMORY;
I wonder if this could be extern char WASM_MEMORY[] ?

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:44: void *wasmPtr(int Index) {
Is Index allowed to be negative?  It seems like not because of the mask.  If it
can't be negative, consider declaring it as unsigned.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:46: const int MASK = 0x3fffff;
If I'm reading this right, there are only 22 bits = 4MB of memory available? 
Can you document what this is for?  because I must be missing something...

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:62: int Buffer = WASM_DEREF(int, VarArgs + 4);
Can you use sizeof() calculations instead of 4/8?

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:68: int env$$__syscall54(int A, int B) {
syscall4 was documented as sys_write.  What is this syscall54?

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:69: // fprintf(stderr, "Unimplemented: sys_ioctl(%d,
%d)\n", a, b);
Remove the commented-out code?

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:75: return -25; /* ENOTTY */
Can you just return -ENOTTY ?

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.cpp#newcode114
src/IceInst.cpp:114: X(Breakpoint, "break");
sort

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.h#newcode71
src/IceInst.h:71: Breakpoint,   // not part of LLVM/PNaCl bitcode
Sort this - between Assign and BundleLock

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:449: case Inst::Breakpoint:
sort

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLowering.h...
src/IceTargetLowering.h:392: virtual void lowerBreakpoint(const InstBreakpoint
*Instr) = 0;
sort

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:290: void lowerBreakpoint(const InstBreakpoint
*Instr) override;
sort

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:304: void lowerBreakpoint(const InstBreakpoint
*Instr) override;
sort

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:265: void lowerBreakpoint(const InstBreakpoint
*Instr) override;
sort

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:919: void _int3() { Context.insert<typename
Traits::Insts::Int3>(); }
sort, I think

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:6104: keepEspLiveAtExit();
I don't think this should be necessary.  Can it be removed?

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:89: llvm_unreachable("unexpected type");
Might as well make this report_fatal_error like above.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:95: } else if (Type == MachineType::Uint8()) {
All these "else if" should just be "if" because of the "return" statements.

Also, I would expect this function to be called quite a lot, so you may want to
try to reorder the tests based on expected frequency.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:132: break;
Can you just return FnName here, and remove the NameFound flag?

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:202: auto *Oper = Op.toOperand();
const auto * ?

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:308: Node Phi(wasm::LocalType, unsigned Count, Node
*Vals, Node Control) {
Can all these unsigneds be uint32_t instead?  That's the style in the rest of
the code base.  (Same for int --> int32_t)

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:875: //
Control()->appendInst(InstBreakpoint::create(Func));
remove?

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1234: std::string FnName = getFunctionName(Module,
Fn.func_index);
use "auto", like above?

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.h
File src/WasmTranslator.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.h#ne...
src/WasmTranslator.h:66: ///   Body - information about the function to
translate
Keep the documentation of Zone?

https://codereview.chromium.org/1876413002/diff/40001/wasm-tests/hello-putchar.c
File wasm-tests/hello-putchar.c (right):

https://codereview.chromium.org/1876413002/diff/40001/wasm-tests/hello-putcha...
wasm-tests/hello-putchar.c:38: // write(2, "---\n", 4);
Do these commented lines need to stay?

[Eric Holk, 2016-04-15 15:24:28]

https://codereview.chromium.org/1876413002/diff/40001/c2wasm-exe.sh
File c2wasm-exe.sh (right):

https://codereview.chromium.org/1876413002/diff/40001/c2wasm-exe.sh#newcode1
c2wasm-exe.sh:1: #!/bin/bash
On 2016/04/14 20:03:43, stichnot wrote:
> This is a pretty hacky script, I'd be happier with a TODO comment to retire
the
> script.

Done.

https://codereview.chromium.org/1876413002/diff/40001/c2wasm-exe.sh#newcode3
c2wasm-exe.sh:3: ./wasm-install/bin/emscripten/emcc $1 -s BINARYEN=1 -s
'BINARYEN_METHOD="native-wasm"' -O2 && \
On 2016/04/14 20:03:43, stichnot wrote:
> 80-col
> 
> You may want to put double-quotes around "$1" to be safer.

Done.

https://codereview.chromium.org/1876413002/diff/40001/fetch-torture-tests.sh
File fetch-torture-tests.sh (right):

https://codereview.chromium.org/1876413002/diff/40001/fetch-torture-tests.sh#...
fetch-torture-tests.sh:7: rm wasm-torture-s-$BUILDID.tbz2
On 2016/04/14 20:03:43, stichnot wrote:
> How about something like this, to avoid temp files:
> 
>   wget -O - https://... | tar xj

Done.

I also tried to make this 80-col, but unfortunately the URL is long that 80
characters.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c
File runtime/wasm-runtime.c (right):

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:10: // This file implements the system calls required the
the libc that is included
On 2016/04/14 20:03:43, stichnot wrote:
> the the ==> in the
> ?

I went with "by the..."

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:16: #include <stdio.h>
On 2016/04/14 20:03:43, stichnot wrote:
> sort?

Done.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:20: extern char WASM_MEMORY;
On 2016/04/14 20:03:43, stichnot wrote:
> I wonder if this could be extern char WASM_MEMORY[] ?

Ah, much better. I tried `char *` initially and that crashed. I mistakenly
assumed `WASM_MEMORY[]` meant the same thing, but it turns out it works.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:44: void *wasmPtr(int Index) {
On 2016/04/14 20:03:43, stichnot wrote:
> Is Index allowed to be negative?  It seems like not because of the mask.  If
it
> can't be negative, consider declaring it as unsigned.

It is, and it's actually quite common for it to be negative.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:46: const int MASK = 0x3fffff;
On 2016/04/14 20:03:43, stichnot wrote:
> If I'm reading this right, there are only 22 bits = 4MB of memory available? 
> Can you document what this is for?  because I must be missing something...

The Wasm file says how many 64KB pages of memory it wants, which is used to
compute this mask. For development purposes, I just hardcoded the value that I'm
seeing in most tests, but the right thing to do here (and what I'm planning to
do soon) is to have the WasmTranslator emit a global variable with the requested
memory size.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:62: int Buffer = WASM_DEREF(int, VarArgs + 4);
On 2016/04/14 20:03:43, stichnot wrote:
> Can you use sizeof() calculations instead of 4/8?

Done.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:68: int env$$__syscall54(int A, int B) {
On 2016/04/14 20:03:44, stichnot wrote:
> syscall4 was documented as sys_write.  What is this syscall54?

sys_ioctl. I added a comment for it.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:69: // fprintf(stderr, "Unimplemented: sys_ioctl(%d,
%d)\n", a, b);
On 2016/04/14 20:03:44, stichnot wrote:
> Remove the commented-out code?

Done.

https://codereview.chromium.org/1876413002/diff/40001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:75: return -25; /* ENOTTY */
On 2016/04/14 20:03:44, stichnot wrote:
> Can you just return -ENOTTY ?

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.cpp#newcode114
src/IceInst.cpp:114: X(Breakpoint, "break");
On 2016/04/14 20:03:44, stichnot wrote:
> sort

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceInst.h#newcode71
src/IceInst.h:71: Breakpoint,   // not part of LLVM/PNaCl bitcode
On 2016/04/14 20:03:44, stichnot wrote:
> Sort this - between Assign and BundleLock

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLowering.cpp
File src/IceTargetLowering.cpp (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLowering.c...
src/IceTargetLowering.cpp:449: case Inst::Breakpoint:
On 2016/04/14 20:03:44, stichnot wrote:
> sort

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLowering.h...
src/IceTargetLowering.h:392: virtual void lowerBreakpoint(const InstBreakpoint
*Instr) = 0;
On 2016/04/14 20:03:44, stichnot wrote:
> sort

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:290: void lowerBreakpoint(const InstBreakpoint
*Instr) override;
On 2016/04/14 20:03:44, stichnot wrote:
> sort

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.h:304: void lowerBreakpoint(const InstBreakpoint
*Instr) override;
On 2016/04/14 20:03:44, stichnot wrote:
> sort

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:265: void lowerBreakpoint(const InstBreakpoint
*Instr) override;
On 2016/04/14 20:03:44, stichnot wrote:
> sort

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:919: void _int3() { Context.insert<typename
Traits::Insts::Int3>(); }
On 2016/04/14 20:03:44, stichnot wrote:
> sort, I think

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:6104: keepEspLiveAtExit();
On 2016/04/14 20:03:44, stichnot wrote:
> I don't think this should be necessary.  Can it be removed?

Done.

I cargo-culted it from lowerUnreachable, so I know of no reason why it needs to
be here.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:89: llvm_unreachable("unexpected type");
On 2016/04/14 20:03:44, stichnot wrote:
> Might as well make this report_fatal_error like above.

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:95: } else if (Type == MachineType::Uint8()) {
On 2016/04/14 20:03:44, stichnot wrote:
> All these "else if" should just be "if" because of the "return" statements.
> 
> Also, I would expect this function to be called quite a lot, so you may want
to
> try to reorder the tests based on expected frequency.

I changed the `else if` to `if`.

I moved the 32-bit integer cases to the top, since I expect they'll be called
most often. For the others, I think it would be best to measure instead of just
guessing the frequencies.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:132: break;
On 2016/04/14 20:03:44, stichnot wrote:
> Can you just return FnName here, and remove the NameFound flag?

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:202: auto *Oper = Op.toOperand();
On 2016/04/14 20:03:44, stichnot wrote:
> const auto * ?

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:308: Node Phi(wasm::LocalType, unsigned Count, Node
*Vals, Node Control) {
On 2016/04/14 20:03:44, stichnot wrote:
> Can all these unsigneds be uint32_t instead?  That's the style in the rest of
> the code base.  (Same for int --> int32_t)

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:875: //
Control()->appendInst(InstBreakpoint::create(Func));
On 2016/04/14 20:03:44, stichnot wrote:
> remove?

Done.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1234: std::string FnName = getFunctionName(Module,
Fn.func_index);
On 2016/04/14 20:03:44, stichnot wrote:
> use "auto", like above?

Done. I went ahead and made it const too.

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.h
File src/WasmTranslator.h (right):

https://codereview.chromium.org/1876413002/diff/40001/src/WasmTranslator.h#ne...
src/WasmTranslator.h:66: ///   Body - information about the function to
translate
On 2016/04/14 20:03:44, stichnot wrote:
> Keep the documentation of Zone?

Oops. It's back now.

https://codereview.chromium.org/1876413002/diff/40001/wasm-tests/hello-putchar.c
File wasm-tests/hello-putchar.c (right):

https://codereview.chromium.org/1876413002/diff/40001/wasm-tests/hello-putcha...
wasm-tests/hello-putchar.c:38: // write(2, "---\n", 4);
On 2016/04/14 20:03:44, stichnot wrote:
> Do these commented lines need to stay?

Nope.

Done.

[Eric Holk, 2016-04-15 15:24:39]

Description was changed from

==========
Subzero. WASM. Additional progress.

This change includes a number of improvements since the last WASM
CL. It compiles against a newer version of V8 that matches the current
WASM binary format. Many more WASM instructions are supported, as well
as global variable initializers.

There is also the beginning of a runtime library that implements some
system calls required by the WASM C library. The c2wasm-exe.sh script
can be used to compile a C program to a .wasm module, which is then
compiled by Subzero into a native executable.

This change includes a new Breakpoint instruction, which inserts an
unconditional breakpoint into the executable. This has been helpful in
debugging code generation for some WASM instructions. The Breakpoint
instruction is only completely implemented on X86.

BUG=
==========

to

==========
Subzero. WASM. Additional progress.

This change includes a number of improvements since the last WASM
CL. It compiles against a newer version of V8 that matches the current
WASM binary format. Many more WASM instructions are supported, as well
as global variable initializers.

There is also the beginning of a runtime library that implements some
system calls required by the WASM C library. The c2wasm-exe.sh script
can be used to compile a C program to a .wasm module, which is then
compiled by Subzero into a native executable.

This change includes a new Breakpoint instruction, which inserts an
unconditional breakpoint into the executable. This has been helpful in
debugging code generation for some WASM instructions. The Breakpoint
instruction is only completely implemented on X86.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4369
==========

[Derek Schuff, 2016-04-15 16:13:22]

dschuff@chromium.org changed reviewers:
+ dschuff@chromium.org

[Derek Schuff, 2016-04-15 16:13:23]

https://codereview.chromium.org/1876413002/diff/80001/c2wasm-exe.sh
File c2wasm-exe.sh (right):

https://codereview.chromium.org/1876413002/diff/80001/c2wasm-exe.sh#newcode6
c2wasm-exe.sh:6: ./wasm-install/bin/emscripten/emcc "$1" -s BINARYEN=1 \
This invocation of emcc will use whatever configuration you happen to have in
your ~/.emscripten file, and may not match what the waterfall uses. To try to
make things more hermetic, the waterfall has 2 config files: emscripten_config
(which uses the fastcomp/asm.js backend, which will use asm2wasm if you use
BINARYEN=1) and emscripten_config_vanilla (which uses the wasm backend
directly). There are 2 different config files because they have different
LLVM_ROOT paths to different builds of LLVM. You can specify the emscripten
config file with the --em-config flag.

[Eric Holk, 2016-04-15 16:16:59]

https://codereview.chromium.org/1876413002/diff/80001/c2wasm-exe.sh
File c2wasm-exe.sh (right):

https://codereview.chromium.org/1876413002/diff/80001/c2wasm-exe.sh#newcode6
c2wasm-exe.sh:6: ./wasm-install/bin/emscripten/emcc "$1" -s BINARYEN=1 \
On 2016/04/15 16:13:23, Derek Schuff wrote:
> This invocation of emcc will use whatever configuration you happen to have in
> your ~/.emscripten file, and may not match what the waterfall uses. To try to
> make things more hermetic, the waterfall has 2 config files: emscripten_config
> (which uses the fastcomp/asm.js backend, which will use asm2wasm if you use
> BINARYEN=1) and emscripten_config_vanilla (which uses the wasm backend
> directly). There are 2 different config files because they have different
> LLVM_ROOT paths to different builds of LLVM. You can specify the emscripten
> config file with the --em-config flag.

Thanks for the suggestion. I'll include the --em-config option in my next CL.

[Jim Stichnoth, 2016-04-15 19:47:26]

lgtm

https://codereview.chromium.org/1876413002/diff/40001/fetch-torture-tests.sh
File fetch-torture-tests.sh (right):

https://codereview.chromium.org/1876413002/diff/40001/fetch-torture-tests.sh#...
fetch-torture-tests.sh:7: rm wasm-torture-s-$BUILDID.tbz2
On 2016/04/15 15:24:26, Eric Holk wrote:
> On 2016/04/14 20:03:43, stichnot wrote:
> > How about something like this, to avoid temp files:
> > 
> >   wget -O - https://... | tar xj
> 
> Done.
> 
> I also tried to make this 80-col, but unfortunately the URL is long that 80
> characters.

Yeah, that's why I didn't comment on 80-col. :)

Now that you mention it, you could achieve 80-col plus other benefits by putting
the common prefix "https://.../git" into a variable.

https://codereview.chromium.org/1876413002/diff/80001/fetch-torture-tests.sh
File fetch-torture-tests.sh (right):

https://codereview.chromium.org/1876413002/diff/80001/fetch-torture-tests.sh#...
fetch-torture-tests.sh:8: wget -O -
https://storage.googleapis.com/wasm-llvm/builds/git/wasm-torture-s2wasm-sexpr...
I would put a space before the trailing backslash, like above, here and below.

https://codereview.chromium.org/1876413002/diff/80001/runtime/wasm-runtime.c
File runtime/wasm-runtime.c (right):

https://codereview.chromium.org/1876413002/diff/80001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:62: int Fd = WASM_DEREF(int, VarArgs);
Totally optional, but my OCD or whatever would have me writing it more like
this:

int xx = WASM_DEREF(int, VarArgs + 0 * sizeof(int));
int yy = WASM_DEREF(int, VarArgs + 1 * sizeof(int));
int zz = WASM_DEREF(int, VarArgs + 2 * sizeof(int));

[Eric Holk, 2016-04-15 20:05:16]

https://codereview.chromium.org/1876413002/diff/40001/fetch-torture-tests.sh
File fetch-torture-tests.sh (right):

https://codereview.chromium.org/1876413002/diff/40001/fetch-torture-tests.sh#...
fetch-torture-tests.sh:7: rm wasm-torture-s-$BUILDID.tbz2
On 2016/04/15 19:47:25, stichnot wrote:
> On 2016/04/15 15:24:26, Eric Holk wrote:
> > On 2016/04/14 20:03:43, stichnot wrote:
> > > How about something like this, to avoid temp files:
> > > 
> > >   wget -O - https://... | tar xj
> > 
> > Done.
> > 
> > I also tried to make this 80-col, but unfortunately the URL is long that 80
> > characters.
> 
> Yeah, that's why I didn't comment on 80-col. :)
> 
> Now that you mention it, you could achieve 80-col plus other benefits by
putting
> the common prefix "https://.../git" into a variable.

I like it. Done.

https://codereview.chromium.org/1876413002/diff/80001/fetch-torture-tests.sh
File fetch-torture-tests.sh (right):

https://codereview.chromium.org/1876413002/diff/80001/fetch-torture-tests.sh#...
fetch-torture-tests.sh:8: wget -O -
https://storage.googleapis.com/wasm-llvm/builds/git/wasm-torture-s2wasm-sexpr...
On 2016/04/15 19:47:25, stichnot wrote:
> I would put a space before the trailing backslash, like above, here and below.

Done.

https://codereview.chromium.org/1876413002/diff/80001/runtime/wasm-runtime.c
File runtime/wasm-runtime.c (right):

https://codereview.chromium.org/1876413002/diff/80001/runtime/wasm-runtime.c#...
runtime/wasm-runtime.c:62: int Fd = WASM_DEREF(int, VarArgs);
On 2016/04/15 19:47:26, stichnot wrote:
> Totally optional, but my OCD or whatever would have me writing it more like
> this:
> 
> int xx = WASM_DEREF(int, VarArgs + 0 * sizeof(int));
> int yy = WASM_DEREF(int, VarArgs + 1 * sizeof(int));
> int zz = WASM_DEREF(int, VarArgs + 2 * sizeof(int));

I agree. This is better. Done.

I think adding a WASM_ARRAY_REF(...) macro or something similar is probably the
right way to do it. I will probably add this as the runtime gets bigger.

[Eric Holk, 2016-04-15 20:05:40]

Description was changed from

==========
Subzero. WASM. Additional progress.

This change includes a number of improvements since the last WASM
CL. It compiles against a newer version of V8 that matches the current
WASM binary format. Many more WASM instructions are supported, as well
as global variable initializers.

There is also the beginning of a runtime library that implements some
system calls required by the WASM C library. The c2wasm-exe.sh script
can be used to compile a C program to a .wasm module, which is then
compiled by Subzero into a native executable.

This change includes a new Breakpoint instruction, which inserts an
unconditional breakpoint into the executable. This has been helpful in
debugging code generation for some WASM instructions. The Breakpoint
instruction is only completely implemented on X86.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4369
==========

to

==========
Subzero. WASM. Additional progress.

This change includes a number of improvements since the last WASM
CL. It compiles against a newer version of V8 that matches the current
WASM binary format. Many more WASM instructions are supported, as well
as global variable initializers.

There is also the beginning of a runtime library that implements some
system calls required by the WASM C library. The c2wasm-exe.sh script
can be used to compile a C program to a .wasm module, which is then
compiled by Subzero into a native executable.

This change includes a new Breakpoint instruction, which inserts an
unconditional breakpoint into the executable. This has been helpful in
debugging code generation for some WASM instructions. The Breakpoint
instruction is only completely implemented on X86.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4369
R=stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[Eric Holk, 2016-04-15 20:05:41]

Committed patchset #6 (id:100001) manually as
67c7c4161ad5bcf234357348a421e313932a8ee3 (presubmit successful).