[wasm] Add support for 64-bit LEB encodings.

[wasm] Add support for 64-bit LEB encodings.

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

Committed: https://crrev.com/616f05496e9867cfa934098a76826cfde7feeaa2
Cr-Commit-Position: refs/heads/master@{#34406}

[titzer, 2016-03-01 02:07:08]

PTAL

[ahaas, 2016-03-01 08:22:58]

lgtm

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h
File src/wasm/decoder.h (right):

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:111: return bit_cast<int64_t>(result << shift) >> shift;
Could you add a comment to explain this line?
Something like "// Use arithmetic shift to get sign extension."

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:335: const int kMaxDiff = (sizeof(IntType) * 8 + 6) / 7;
Should this be "kMaxLength"? It is the maximum value of length, is it not?

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:351: const int kExtraBits = (1 + kMaxDiff * 7) -
(sizeof(IntType) * 8);
Could you add a comment here, to hint what this extra bits stuff is about?

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
File test/unittests/wasm/decoder-unittest.cc (right):

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:450: data[j] = (val >> (7 * j)) &
MASK_7;
static_cast?

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:486: TEST_F(DecoderTest,
ReadI64v_TwoByte) {
Should this be ReadI64v_OneByte?

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:569: TEST_F(DecoderTest, ReadI64v_Bits)
{
Would it be possible to write one test function for this test and the test
"ReadU64v_Bits", maybe as a template function with a function pointer parameter?

[titzer, 2016-03-01 18:49:12]

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h
File src/wasm/decoder.h (right):

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:111: return bit_cast<int64_t>(result << shift) >> shift;
On 2016/03/01 08:22:58, ahaas wrote:
> Could you add a comment to explain this line?
> Something like "// Use arithmetic shift to get sign extension."

Done.

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:335: const int kMaxDiff = (sizeof(IntType) * 8 + 6) / 7;
On 2016/03/01 08:22:58, ahaas wrote:
> Should this be "kMaxLength"? It is the maximum value of length, is it not?

Done.

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:351: const int kExtraBits = (1 + kMaxDiff * 7) -
(sizeof(IntType) * 8);
On 2016/03/01 08:22:58, ahaas wrote:
> Could you add a comment here, to hint what this extra bits stuff is about?

Done.

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
File test/unittests/wasm/decoder-unittest.cc (right):

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:450: data[j] = (val >> (7 * j)) &
MASK_7;
On 2016/03/01 08:22:58, ahaas wrote:
> static_cast?

Done.

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:486: TEST_F(DecoderTest,
ReadI64v_TwoByte) {
On 2016/03/01 08:22:58, ahaas wrote:
> Should this be ReadI64v_OneByte?

Done.

[binji, 2016-03-01 18:49:40]

curious about non-minimal leb128, otherwise lgtm

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h
File src/wasm/decoder.h (right):

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:337: const byte* end = ptr + kMaxDiff;
I don't think this is right if we allow non-minimal LEB (which was suggested in
the discussion)

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:351: const int kExtraBits = (1 + kMaxDiff * 7) -
(sizeof(IntType) * 8);
Yeah, it took me walking through it to understand the purpose of the +1, it'd be
nice to have a comment. :)

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
File test/unittests/wasm/decoder-unittest.cc (right):

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:444: // foreach length 1...64
32

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:446: const uint32_t val = kVals[v] &
((1ull << (i - 1)) - 1);
Am I reading this wrong, or does this give the wrong bitmasks?
Looks like it's off by one.

[titzer, 2016-03-01 19:09:20]

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h
File src/wasm/decoder.h (right):

https://codereview.chromium.org/1746063003/diff/80001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:337: const byte* end = ptr + kMaxDiff;
On 2016/03/01 18:49:40, binji wrote:
> I don't think this is right if we allow non-minimal LEB (which was suggested
in
> the discussion)

I don't think we should support arbitrary-length LEB where stuff is expected to
be 32 or 64 bits, so making the cutoff 5 or 10 bytes respectively seems OK.

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
File test/unittests/wasm/decoder-unittest.cc (right):

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:444: // foreach length 1...64
On 2016/03/01 18:49:40, binji wrote:
> 32

Done.

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:446: const uint32_t val = kVals[v] &
((1ull << (i - 1)) - 1);
On 2016/03/01 18:49:40, binji wrote:
> Am I reading this wrong, or does this give the wrong bitmasks?
> Looks like it's off by one.

Good catch. It would strip off one extra high bit.

https://codereview.chromium.org/1746063003/diff/80001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:569: TEST_F(DecoderTest, ReadI64v_Bits)
{
On 2016/03/01 08:22:58, ahaas wrote:
> Would it be possible to write one test function for this test and the test
> "ReadU64v_Bits", maybe as a template function with a function pointer
parameter?

Too much mechanism I think.

[titzer, 2016-03-01 19:34:16]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-03-01 19:34:16]

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

[commit-bot: I haz the power, 2016-03-01 19:34:19]

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

[commit-bot: I haz the power, 2016-03-01 19:36:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-01 19:37:02]

Description was changed from

==========
[wasm] Add support for 64-bit LEB encodings.

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

to

==========
[wasm] Add support for 64-bit LEB encodings.

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

Committed: https://crrev.com/616f05496e9867cfa934098a76826cfde7feeaa2
Cr-Commit-Position: refs/heads/master@{#34406}
==========

[commit-bot: I haz the power, 2016-03-01 19:37:03]

Patchset 6 (id:??) landed as
https://crrev.com/616f05496e9867cfa934098a76826cfde7feeaa2
Cr-Commit-Position: refs/heads/master@{#34406}

[Michael Achenbach, 2016-03-01 19:55:02]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1749343002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Seems to break chromium win compile:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Win/builds/380....