[wasm] Add an export table.

[wasm] Add an export table.

R=binji@chromium.org,jfb@chromium.org
BUG=

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

[JF, 2016-02-27 00:59:48]

Minor comments, the OOM thing predates this so it's not worth fixing in this CL.

lgtm otherwise.

https://codereview.chromium.org/1744713003/diff/1/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/1744713003/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:251: module->export_table->push_back({0, 0});
What happens if SafeReserve clamped the reserved size, and then push_back fails?

https://codereview.chromium.org/1744713003/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:257: error(sigpos, "invalid function index");
error(sigpos, "invalid function index %i, max is %i", exp->func_index,
module->functions->size());

https://codereview.chromium.org/1744713003/diff/1/test/mjsunit/wasm/export-ta...
File test/mjsunit/wasm/export-table.js (right):

https://codereview.chromium.org/1744713003/diff/1/test/mjsunit/wasm/export-ta...
test/mjsunit/wasm/export-table.js:1: // Copyright 2015 the V8 project authors.
All rights reserved.
2016

[binji, 2016-02-27 01:12:15]

lgtm w/ some nits

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
File test/unittests/wasm/module-decoder-unittest.cc (right):

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:16: #define
EMPTY_FUNCTION(sig_index) 0, static_cast<byte>(sig_index), 0, 0, 0
might was well use SIG_INDEX here too, no?

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:165: NAME_OFFSET(0),  // name
offset
this one should be invalid

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:315: kDeclFunctionLocals |
kDeclFunctionExport | kDeclFunctionName, 0,
SIG_INDEX(0) here?

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:369: U16(0),
SIG_INDEX

[titzer, 2016-02-27 01:21:58]

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
File test/unittests/wasm/module-decoder-unittest.cc (right):

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:165: NAME_OFFSET(0),  // name
offset
On 2016/02/27 01:12:15, binji wrote:
> this one should be invalid

Ah, yeah, the offset should be 3 * 256 or something huge.

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:315: kDeclFunctionLocals |
kDeclFunctionExport | kDeclFunctionName, 0,
On 2016/02/27 01:12:15, binji wrote:
> SIG_INDEX(0) here?

Done.

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:369: U16(0),
On 2016/02/27 01:12:15, binji wrote:
> SIG_INDEX

Done.

[titzer, 2016-02-27 01:22:24]

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
File test/unittests/wasm/module-decoder-unittest.cc (right):

https://codereview.chromium.org/1744713003/diff/40001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:16: #define
EMPTY_FUNCTION(sig_index) 0, static_cast<byte>(sig_index), 0, 0, 0
On 2016/02/27 01:12:15, binji wrote:
> might was well use SIG_INDEX here too, no?

Done.

[titzer, 2016-02-27 01:27:07]

On 2016/02/27 00:59:48, JF wrote:
> Minor comments, the OOM thing predates this so it's not worth fixing in this
CL.
> 
> lgtm otherwise.
> 
> https://codereview.chromium.org/1744713003/diff/1/src/wasm/module-decoder.cc
> File src/wasm/module-decoder.cc (right):
> 
>
https://codereview.chromium.org/1744713003/diff/1/src/wasm/module-decoder.cc#...
> src/wasm/module-decoder.cc:251: module->export_table->push_back({0, 0});
> What happens if SafeReserve clamped the reserved size, and then push_back
fails?
>

IIUC, reserve() just reserves space, as a hint. So further push_back() calls can
still resize.
 
>
https://codereview.chromium.org/1744713003/diff/1/src/wasm/module-decoder.cc#...
> src/wasm/module-decoder.cc:257: error(sigpos, "invalid function index");
> error(sigpos, "invalid function index %i, max is %i", exp->func_index,
> module->functions->size());
> 

Done.

>
https://codereview.chromium.org/1744713003/diff/1/test/mjsunit/wasm/export-ta...
> File test/mjsunit/wasm/export-table.js (right):
> 
>
https://codereview.chromium.org/1744713003/diff/1/test/mjsunit/wasm/export-ta...
> test/mjsunit/wasm/export-table.js:1: // Copyright 2015 the V8 project authors.
> All rights reserved.
> 2016

Done.

[JF, 2016-02-27 01:31:32]

>
https://codereview.chromium.org/1744713003/diff/1/src/wasm/module-decoder.cc#...
> > src/wasm/module-decoder.cc:251: module->export_table->push_back({0, 0});
> > What happens if SafeReserve clamped the reserved size, and then push_back
> fails?
> >
> 
> IIUC, reserve() just reserves space, as a hint. So further push_back() calls
can
> still resize.

Right, it's just a reservation hint, but what happens when push_back fails
allocating? I guess V8's allocator is already hooked to handle this. It just
seems weird to clamp the allocation, but then unconditionally allocating it at
runtime beyond the clamped value, e.g. an attacker giving a huge value will
cause a huge allocation, and because reserve is clamped it'll fail late (in
push_back) instead of early (in reserve).

[titzer, 2016-02-27 01:50:43]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-02-27 01:50:43]

The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org,
jfb@chromium.org
Link to the patchset: https://codereview.chromium.org/1744713003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-02-27 01:50:53]

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

[commit-bot: I haz the power, 2016-02-27 01:52:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-27 01:54:37]

Description was changed from

==========
[wasm] Add an export table.

R=binji@chromium.org,jfb@chromium.org
BUG=
==========

to

==========
[wasm] Add an export table.

R=binji@chromium.org,jfb@chromium.org
BUG=

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

[commit-bot: I haz the power, 2016-02-27 01:54:38]

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