Subzero, Wasm: Dynamically reallocate read buffer. Runtime improvements.

Subzero, Wasm: Dynamically reallocate read buffer. Runtime improvements.

This change fills in several more runtime functions needed by several
benchmarks, as well as changing the buffer handling in the WASM
decoder. Now the decoder will resize the buffer as needed to
accomodate large .wasm modules.

Tracing can now be enabled on runtime functions to aid with
debugging. Additionally, runtime failures such as bounds check
failures or invalid indirect function calls tell what kind of failure
occured.

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

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

[Eric Holk, 2016-04-26 20:57:03]

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

[Jim Stichnoth, 2016-04-26 22:36:43]

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cpp
File runtime/wasm-runtime.cpp (right):

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:21: // TODO (eholk): change to cstdint
You can probably just remove this TODO...  It made more sense when there was
only one C-style .h file.

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:89: return Union.X;
This kind of type punning is undefined behavior - you're not allowed to write
one field of the union and then read it from a different field (common as this
is in practice).

You could probably use Utils::bitCopy() instead.

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:91: } // end of anonymous namespace;
no semicolon

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:320: int WasmPath = WASM_DEREF(int, VarArgs);
These WASM_DEREFs should be in terms of <x> * sizeof(int)

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:339: int WasmPath = WASM_DEREF(int, VarArgs);
Above, you have "WASM_DEREF(int, VarArgs + 0 * sizeof(int))".

If this is the only WASM_DEREF in the function, I think it's OK to omit the "0 *
", but be consistent.

But note that using "0 * " even for single-WASM_DEREF functions, makes it more
future-proof.

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:392: void *Ptr = WASM_REF(void, WASM_DEREF(int, Iov + I
* 8));
Can this be done without the magic 4/8 constants?

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:408: int Fd = WASM_DEREF(int, VarArgs);
VarArgs + <n> * sizeof(int)

Given all this repetition, what do you think of using a 3-arg macro instead of
most of these WASM_DEREF uses, and that macro does the <n>*sizeof(int)
adjustment?

https://codereview.chromium.org/1918213003/diff/20001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1918213003/diff/20001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:59: return std::regex_match(Filename,
std::regex(".*\\.wasm"));
Instead of removing BuildDefs::llvmIrAsInput(), can you replace it with
BuildDefs::wasm() ?

The point here is to avoid pulling in the regex library code if not needed.

https://codereview.chromium.org/1918213003/diff/20001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1918213003/diff/20001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1359:
Ctx->getConstantExternSym(Ctx->getGlobalString("__sz_bounds_fail")),
Add this string to IceTargetLowering.def (but use the "__Sz_" prefix), and use
makeHelperCall() to create the call instruction.

Also, I think these helpers should be documented in runtime/szrt.c, for
completeness.

[John, 2016-04-27 02:28:20]

lgtm

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

https://codereview.chromium.org/1918213003/diff/40001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:16: #include <cmath>
extreme nit ** 10: space between c++ and c headers?

https://codereview.chromium.org/1918213003/diff/40001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:19: #include <iostream>
move this to the "c++ header section?"

https://codereview.chromium.org/1918213003/diff/40001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1918213003/diff/40001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:59: return std::regex_match(Filename,
std::regex(".*\\.wasm"));
why did this change?

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

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:780: const auto *Info =
Ctx->getIntrinsicsInfo().find(FnName, BadInstrinsic);
rant: BadIntrinsic is by-ref... I was about to complain about the lack of
constexpr in the BadIntrinsic delcaration (nothing you can/should do)

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:782: assert(Info);
so, I wonder, in a noassert build, an invalid intrinsic will cause sz to
segfault. is this what we want?

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1368: // TODO (eholk): Move this node to the end of the
CFG, or even better,
I like the idea of having one abort per Function better -- it is easier to
debug. Keep that in mind.

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1428: while (true) {
incredibly ridiculous nit: I prefer for(;;) in lieu of while (true);

but seriously, you don't need to change this. :)

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

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.h#ne...
src/WasmTranslator.h:73: std::vector<uint8_t> Buffer;
cool!

[Eric Holk, 2016-04-27 21:45:21]

I tried to address most people's feedback, although some of the changes and
comments might have gotten lost.

The biggest change is that I replaced most of the uses of WASM_REF and
WASM_DEREF with new C++ wrapper classes that make working with Wasm pointers
more like working with regular pointers.

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cpp
File runtime/wasm-runtime.cpp (right):

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:21: // TODO (eholk): change to cstdint
On 2016/04/26 22:36:43, stichnot wrote:
> You can probably just remove this TODO...  It made more sense when there was
> only one C-style .h file.

Done.

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:89: return Union.X;
On 2016/04/26 22:36:43, stichnot wrote:
> This kind of type punning is undefined behavior - you're not allowed to write
> one field of the union and then read it from a different field (common as this
> is in practice).
> 
> You could probably use Utils::bitCopy() instead.

The functions that call this are meant to provide software emulation of long
double support and are normally implemented in compiler-rt. My understanding is
that these functions are pretty much only needed for musl to implement printf on
long doubles, but are rarely used in practice.

In the interest of not having to maintain a floating point emulation library,
I've decided to replace all these functions with UNIMPLEMENTED. They are needed
in order to link, but I've never seen one called in practice.

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:91: } // end of anonymous namespace;
On 2016/04/26 22:36:43, stichnot wrote:
> no semicolon

Done.

https://codereview.chromium.org/1918213003/diff/20001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:408: int Fd = WASM_DEREF(int, VarArgs);
On 2016/04/26 22:36:43, stichnot wrote:
> VarArgs + <n> * sizeof(int)
> 
> Given all this repetition, what do you think of using a 3-arg macro instead of
> most of these WASM_DEREF uses, and that macro does the <n>*sizeof(int)
> adjustment?

Actually, I think this is better done using C++. What about something like
`WasmHeap.asArray<int>[n]`?

https://codereview.chromium.org/1918213003/diff/20001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1918213003/diff/20001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:59: return std::regex_match(Filename,
std::regex(".*\\.wasm"));
On 2016/04/26 22:36:43, stichnot wrote:
> Instead of removing BuildDefs::llvmIrAsInput(), can you replace it with
> BuildDefs::wasm() ?
> 
> The point here is to avoid pulling in the regex library code if not needed.

Done.

https://codereview.chromium.org/1918213003/diff/20001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1918213003/diff/20001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1359:
Ctx->getConstantExternSym(Ctx->getGlobalString("__sz_bounds_fail")),
On 2016/04/26 22:36:43, stichnot wrote:
> Add this string to IceTargetLowering.def (but use the "__Sz_" prefix), and use
> makeHelperCall() to create the call instruction.
> 
> Also, I think these helpers should be documented in runtime/szrt.c, for
> completeness.

I tried this and it ends up being a little bit bigger of a change than I
expected. First, makeHelperCall is not accessible here because the Wasm
translator runs before the TargetLowering object is created (and in a different
thread, currently).

More importantly though, right now wasm-runtime.cpp is not included in the
normal Subzero runtime, but if we add these functions to IceTargetLowering.def,
we'll also have to include them in szrt.c. My main concern about this is that
szrt looks intentionally very minimal, but these error functions write to
stderr. How will that play out with browser integration, etc?

I feel like it's better to leave szrt.c alone unless we have reason to think
we'll also need these functions in the normal Subzero path.

https://codereview.chromium.org/1918213003/diff/40001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1918213003/diff/40001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:59: return std::regex_match(Filename,
std::regex(".*\\.wasm"));
On 2016/04/27 02:28:20, John wrote:
> why did this change?

I found out wasm support was always being disabled in minimal builds because
llvmIrAsInput was returning false. The thing we really want here (which I've
added) is a test to see if wasm was enabled.

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

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:780: const auto *Info =
Ctx->getIntrinsicsInfo().find(FnName, BadInstrinsic);
On 2016/04/27 02:28:20, John wrote:
> rant: BadIntrinsic is by-ref... I was about to complain about the lack of
> constexpr in the BadIntrinsic delcaration (nothing you can/should do)

Acknowledged.

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:782: assert(Info);
On 2016/04/27 02:28:20, John wrote:
> so, I wonder, in a noassert build, an invalid intrinsic will cause sz to
> segfault. is this what we want?

In these cases getInstrinsicsInfo should always succeed because we are calling
hard-coded, known intrinsics. In cases where we are getting intrinsics from the
outside world, hopefully we are actually testing these values rather than just
asserting.

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1368: // TODO (eholk): Move this node to the end of the
CFG, or even better,
On 2016/04/27 02:28:20, John wrote:
> I like the idea of having one abort per Function better -- it is easier to
> debug. Keep that in mind.

Good point. That's actually why I split the generic abort node into one for
bounds checks and one for indirect calls.

https://codereview.chromium.org/1918213003/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1428: while (true) {
On 2016/04/27 02:28:20, John wrote:
> incredibly ridiculous nit: I prefer for(;;) in lieu of while (true);
> 
> but seriously, you don't need to change this. :)

Acknowledged :)

[Jim Stichnoth, 2016-04-29 20:57:38]

lgtm

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

https://codereview.chromium.org/1918213003/diff/80001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:212: // The empty comment is to keep clang-fmt from
breaking this pre-C++11 code.
lol

https://codereview.chromium.org/1918213003/diff/80001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:219: // fprintf(stderr, "main(%d)\n", argc);
remove?

https://codereview.chromium.org/1918213003/diff/80001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:230: WasmPtr<WasmPtr<char> /**/> WasmArgV = HeapBreak;
You could also consider making WasmPtr<char> into a typedef.

https://codereview.chromium.org/1918213003/diff/80001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:281: (void)_;
Can you just omit the argument name in the declaration above, and then remove
this void cast?

[Eric Holk, 2016-04-29 21:42:21]

Description was changed from

==========
Subzero, Wasm: Dynamically reallocate read buffer. Runtime improvements.

This change fills in several more runtime functions needed by several
benchmarks, as well as changing the buffer handling in the WASM
decoder. Now the decoder will resize the buffer as needed to
accomodate large .wasm modules.

Tracing can now be enabled on runtime functions to aid with
debugging. Additionally, runtime failures such as bounds check
failures or invalid indirect function calls tell what kind of failure
occured.

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

to

==========
Subzero, Wasm: Dynamically reallocate read buffer. Runtime improvements.

This change fills in several more runtime functions needed by several
benchmarks, as well as changing the buffer handling in the WASM
decoder. Now the decoder will resize the buffer as needed to
accomodate large .wasm modules.

Tracing can now be enabled on runtime functions to aid with
debugging. Additionally, runtime failures such as bounds check
failures or invalid indirect function calls tell what kind of failure
occured.

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

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

[Eric Holk, 2016-04-29 21:42:22]

Committed patchset #6 (id:100001) manually as
87def2c8fa994f2daf87a090763a472335622697 (presubmit successful).

[Eric Holk, 2016-04-29 21:42:55]

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

https://codereview.chromium.org/1918213003/diff/80001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:219: // fprintf(stderr, "main(%d)\n", argc);
On 2016/04/29 20:57:38, stichnot wrote:
> remove?

Done.

https://codereview.chromium.org/1918213003/diff/80001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:230: WasmPtr<WasmPtr<char> /**/> WasmArgV = HeapBreak;
On 2016/04/29 20:57:38, stichnot wrote:
> You could also consider making WasmPtr<char> into a typedef.

Good idea. Done.

https://codereview.chromium.org/1918213003/diff/80001/runtime/wasm-runtime.cp...
runtime/wasm-runtime.cpp:281: (void)_;
On 2016/04/29 20:57:38, stichnot wrote:
> Can you just omit the argument name in the declaration above, and then remove
> this void cast?

Ah, that seems to work. I seem to remember that not working the last time I
tried, so I'm not sure what happened.

Done.