[wasm] Add support and unittests for decoding signed LEB128.

[wasm] Add support and unittests for decoding signed LEB128.

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

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

[bradn, 2016-02-29 06:29:59]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2016-02-29 06:30:00]

lgtm

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

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:82: if (!check(base, offset, 1, msg)) {
FWIW, sharing this check ends up being kind of confusing.

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:103: if (*length == 5 && (b & 0xF0) != 0) {
5 -> kMaxDiff ?

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:107: if (b & 0x80) {
might be good to be consistent between b & 0x80 vs (b & 0x80) != 0 -- as in the
condition above.

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/wasm-macro-gen.h
File src/wasm/wasm-macro-gen.h (right):

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/wasm-macro-gen...
src/wasm/wasm-macro-gen.h:280: #define MASK_7 ((1 << 7) - 1)
These are only used in one test, move them there? / inline the values?

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

https://codereview.chromium.org/1746653002/diff/20001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:170: CHECK_UINT32V_INLINE(val & MASK_7,
1, U32V_1(val));
These masks are only used for this test. They might be clearer if inline with
the test?

[ahaas, 2016-02-29 08:51:41]

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

https://codereview.chromium.org/1746653002/diff/20001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:55: CHECK_UINT32V_INLINE(0, 1, 0, 0);
Why don't you use U32V_2 also here?

[titzer, 2016-02-29 16:51:46]

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

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:82: if (!check(base, offset, 1, msg)) {
On 2016/02/29 06:30:00, bradn wrote:
> FWIW, sharing this check ends up being kind of confusing.

Acknowledged.

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:103: if (*length == 5 && (b & 0xF0) != 0) {
On 2016/02/29 06:30:00, bradn wrote:
> 5 -> kMaxDiff ?

Done.

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:107: if (b & 0x80) {
On 2016/02/29 06:30:00, bradn wrote:
> might be good to be consistent between b & 0x80 vs (b & 0x80) != 0 -- as in
the
> condition above.

Done.

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/wasm-macro-gen.h
File src/wasm/wasm-macro-gen.h (right):

https://codereview.chromium.org/1746653002/diff/20001/src/wasm/wasm-macro-gen...
src/wasm/wasm-macro-gen.h:280: #define MASK_7 ((1 << 7) - 1)
On 2016/02/29 06:30:00, bradn wrote:
> These are only used in one test, move them there? / inline the values?

The plan is to use U32V_1, U32V_2, etc in more tests, e.g. when I32Const and
I64Const are using SLEB's.

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

https://codereview.chromium.org/1746653002/diff/20001/test/unittests/wasm/dec...
test/unittests/wasm/decoder-unittest.cc:55: CHECK_UINT32V_INLINE(0, 1, 0, 0);
On 2016/02/29 08:51:41, ahaas wrote:
> Why don't you use U32V_2 also here?

I wanted to have one or two tests that don't go through the macros as well.

[titzer, 2016-02-29 17:05:11]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-02-29 17:05:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@google.com
Link to the patchset: https://codereview.chromium.org/1746653002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-02-29 17:05:24]

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

[commit-bot: I haz the power, 2016-02-29 17:09:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-29 17:09:33]

Try jobs failed on following builders:
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/2093)

[titzer, 2016-02-29 18:15:48]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-29 18:15:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1746653002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1746653002/80001

[commit-bot: I haz the power, 2016-02-29 18:36:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-29 18:36:41]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-02-29 18:39:03]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-02-29 18:39:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@google.com
Link to the patchset: https://codereview.chromium.org/1746653002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-02-29 18:39:10]

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

[commit-bot: I haz the power, 2016-02-29 18:41:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-29 18:42:33]

Description was changed from

==========
[wasm] Add support and unittests for decoding signed LEB128.

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

to

==========
[wasm] Add support and unittests for decoding signed LEB128.

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

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

[commit-bot: I haz the power, 2016-02-29 18:42:34]

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