[wasm] Implement AST printing into an ostream

[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.
The PrintAst function now also optionally provides an offset table
mapping from byte offset to line and column in the generated text.

R=titzer@chromium.org, ahaas@chromium.org
BUG=chromium:613110
Committed: https://crrev.com/12aa132d873da628971f0610a987bcef302d9d9f
Cr-Commit-Position: refs/heads/master@{#37026}

[Clemens Hammacher, 2016-06-09 12:19:08]

Description was changed from

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.

R=titzer@chromium.org
==========

to

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.

R=titzer@chromium.org
BUG=chromium:613110
==========

[titzer, 2016-06-10 10:57:41]

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:151: if (!m || !m->module) return false;
This change means that there will not be an error recorded for when there isn't
a module environment supplied. That will probably break downstream assumptions.

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:216: static FunctionBody ForTesting(const byte* start,
const byte* end) {
Can you please move this out to the top level? (style guide recommends no
functions within structs).

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:232: std::function<void(uint32_t)> instruction_callback =
{});
There isn't a usecase for the instruction_callback function yet. (I assume there
is one in a CL based on this one).

Can we avoid a callback and instead return a map from byte index to line (since
that is what I assume this is being used for)?

[Clemens Hammacher, 2016-06-13 09:40:11]

Description was changed from

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.

R=titzer@chromium.org
BUG=chromium:613110
==========

to

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.
The PrintAst function now also optionally provides an offset table
mapping from byte offset to line and column in the generated text.

R=titzer@chromium.org
BUG=chromium:613110
==========

[Clemens Hammacher, 2016-06-13 09:45:17]

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:151: if (!m || !m->module) return false;
On 2016/06/10 10:57:40, titzer wrote:
> This change means that there will not be an error recorded for when there
isn't
> a module environment supplied. That will probably break downstream
assumptions.

I changed it to have a Complete() and a Validate() function, s.t. Validate()
calls Complete(), and only Validate() will report any errors.

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:216: static FunctionBody ForTesting(const byte* start,
const byte* end) {
On 2016/06/10 10:57:40, titzer wrote:
> Can you please move this out to the top level? (style guide recommends no
> functions within structs).

Done.
Not sure though if style guide really advises against this. It sais for structs:
Methods should not provide behavior but should only be used to set up the data
members, e.g., constructor, destructor, Initialize(), Reset(), Validate().

https://codereview.chromium.org/2050213002/diff/40001/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:232: std::function<void(uint32_t)> instruction_callback =
{});
On 2016/06/10 10:57:40, titzer wrote:
> There isn't a usecase for the instruction_callback function yet. (I assume
there
> is one in a CL based on this one).
> 
> Can we avoid a callback and instead return a map from byte index to line
(since
> that is what I assume this is being used for)?

Done.

[Clemens Hammacher, 2016-06-15 07:49:09]

Description was changed from

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.
The PrintAst function now also optionally provides an offset table
mapping from byte offset to line and column in the generated text.

R=titzer@chromium.org
BUG=chromium:613110
==========

to

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.
The PrintAst function now also optionally provides an offset table
mapping from byte offset to line and column in the generated text.

R=titzer@chromium.org, ahaas@chromium.org
BUG=chromium:613110
==========

[Clemens Hammacher, 2016-06-15 07:49:09]

clemensh@chromium.org changed reviewers:
+ ahaas@chromium.org

[Clemens Hammacher, 2016-06-15 07:49:23]

PTAL

[ahaas, 2016-06-15 14:22:53]

lgtm with nits

https://codereview.chromium.org/2050213002/diff/60001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2050213002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1598: const char* padding = "                           
    ";
The old code allowed 64 whitespaces. (Well observed by yourself ;) )

https://codereview.chromium.org/2050213002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1605: os << "k" << WasmOpcodes::OpcodeName(opcode) <<
",";
Would it not be cleaner to keep the padding and the writing of the opcode
together, and afterwards you deal with the offset_table?

[Clemens Hammacher, 2016-06-15 17:00:55]

https://codereview.chromium.org/2050213002/diff/60001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2050213002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1598: const char* padding = "                           
    ";
On 2016/06/15 14:22:53, ahaas wrote:
> The old code allowed 64 whitespaces. (Well observed by yourself ;) )

Done.

https://codereview.chromium.org/2050213002/diff/60001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1605: os << "k" << WasmOpcodes::OpcodeName(opcode) <<
",";
On 2016/06/15 14:22:53, ahaas wrote:
> Would it not be cleaner to keep the padding and the writing of the opcode
> together, and afterwards you deal with the offset_table?

Done.

[Clemens Hammacher, 2016-06-16 07:20:18]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-06-16 07:20:18]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2050213002/#ps80001
(title: "address ahaas' comments")

[commit-bot: I haz the power, 2016-06-16 07:20:24]

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

[commit-bot: I haz the power, 2016-06-16 07:22:52]

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

[commit-bot: I haz the power, 2016-06-16 07:22:53]

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

[Michael Starzinger, 2016-06-16 07:44:46]

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

[Michael Starzinger, 2016-06-16 07:44:46]

LGTM (rubber-stamp for compiler part).

[Clemens Hammacher, 2016-06-16 07:46:31]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2016-06-16 07:46:36]

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

[commit-bot: I haz the power, 2016-06-16 07:48:39]

Description was changed from

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.
The PrintAst function now also optionally provides an offset table
mapping from byte offset to line and column in the generated text.

R=titzer@chromium.org, ahaas@chromium.org
BUG=chromium:613110
==========

to

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.
The PrintAst function now also optionally provides an offset table
mapping from byte offset to line and column in the generated text.

R=titzer@chromium.org, ahaas@chromium.org
BUG=chromium:613110
==========

[commit-bot: I haz the power, 2016-06-16 07:48:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-16 07:50:02]

Description was changed from

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.
The PrintAst function now also optionally provides an offset table
mapping from byte offset to line and column in the generated text.

R=titzer@chromium.org, ahaas@chromium.org
BUG=chromium:613110
==========

to

==========
[wasm] Implement AST printing into an ostream

This will be used for disassembling individual wasm function for
showing them in devtools.
The PrintAst function now also optionally provides an offset table
mapping from byte offset to line and column in the generated text.

R=titzer@chromium.org, ahaas@chromium.org
BUG=chromium:613110

Committed: https://crrev.com/12aa132d873da628971f0610a987bcef302d9d9f
Cr-Commit-Position: refs/heads/master@{#37026}
==========

[commit-bot: I haz the power, 2016-06-16 07:50:03]

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