[wasm] Disassemble wasm code from script

[wasm] Disassemble wasm code from script

This stores the wasm object and the function index in the script, and
adds functions to get the disassembled wasm code as well as the offset
table mapping from byte position to line and column in the disassembly
solely from the script.
This will be used to show "ui source code" in DevTools, and map raw
locations from the stack trace into this code view.

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

[Yang, 2016-06-16 13:49:36]

https://codereview.chromium.org/2063013004/diff/40001/src/debug/debug.js
File src/debug/debug.js (right):

https://codereview.chromium.org/2063013004/diff/40001/src/debug/debug.js#newc...
src/debug/debug.js:861: Debug.scriptById = function(scriptId) {
Please do not put this helper function on the Debug object if you don't intend
this to be exposed as part of the debugger API.

https://codereview.chromium.org/2063013004/diff/40001/src/debug/debug.js#newc...
src/debug/debug.js:876: return script ? %GetWasmFunctionOffsetTable(script) :
UNDEFINED;
Why do we need to expose this table? Can't we encapsulate the code-offset ->
source position mapping in Code::SourcePosition, like we do for ordinary code
objects?

https://codereview.chromium.org/2063013004/diff/40001/src/debug/debug.js#newc...
src/debug/debug.js:881: return script ? %DisassembleWasmFunction(script) :
UNDEFINED;
With this, you would still need to change DevTools to add special handling for
wasm scripts.

Why can't we simply compute this on-demand and present it as script source
string?

https://codereview.chromium.org/2063013004/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2063013004/diff/40001/src/objects-inl.h#newco...
src/objects-inl.h:5580: ACCESSORS_CHECKED(Script, eval_from_shared, Object,
kEvalFromSharedOffset,
I have plans to move the EvalFromShared and EvalFromPosition fields into a
separate FixedArray (because I need to add yet another field for eval scripts).
Could you do the same? We would replace these two fields by a TypeSpecificData
field.

[ahaas, 2016-06-17 06:57:50]

wasm looks good

https://codereview.chromium.org/2063013004/diff/40001/test/mjsunit/wasm/debug...
File test/mjsunit/wasm/debug-disassembly.js (right):

https://codereview.chromium.org/2063013004/diff/40001/test/mjsunit/wasm/debug...
test/mjsunit/wasm/debug-disassembly.js:48: if (name.endsWith("/0")) {
What's the idea behind this condition? Can you add a comment when name ends with
"/0"?

[Clemens Hammacher, 2016-06-17 18:01:55]

https://codereview.chromium.org/2063013004/diff/40001/src/debug/debug.js
File src/debug/debug.js (right):

https://codereview.chromium.org/2063013004/diff/40001/src/debug/debug.js#newc...
src/debug/debug.js:861: Debug.scriptById = function(scriptId) {
On 2016/06/16 13:49:35, Yang wrote:
> Please do not put this helper function on the Debug object if you don't intend
> this to be exposed as part of the debugger API.

Done.

https://codereview.chromium.org/2063013004/diff/40001/src/debug/debug.js#newc...
src/debug/debug.js:876: return script ? %GetWasmFunctionOffsetTable(script) :
UNDEFINED;
On 2016/06/16 13:49:35, Yang wrote:
> Why do we need to expose this table? Can't we encapsulate the code-offset ->
> source position mapping in Code::SourcePosition, like we do for ordinary code
> objects?

As discussed offline: We stick to reporting byte offsets as source position, and
make the mapping to disassembly explicit.

https://codereview.chromium.org/2063013004/diff/40001/src/debug/debug.js#newc...
src/debug/debug.js:881: return script ? %DisassembleWasmFunction(script) :
UNDEFINED;
On 2016/06/16 13:49:35, Yang wrote:
> With this, you would still need to change DevTools to add special handling for
> wasm scripts.
> 
> Why can't we simply compute this on-demand and present it as script source
> string?

Same as above.

https://codereview.chromium.org/2063013004/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2063013004/diff/40001/src/objects-inl.h#newco...
src/objects-inl.h:5580: ACCESSORS_CHECKED(Script, eval_from_shared, Object,
kEvalFromSharedOffset,
On 2016/06/16 13:49:35, Yang wrote:
> I have plans to move the EvalFromShared and EvalFromPosition fields into a
> separate FixedArray (because I need to add yet another field for eval
scripts).
> Could you do the same? We would replace these two fields by a TypeSpecificData
> field.

Sure, that change would be compatible with wasm. All we need is getting the wasm
object and the function index from the script.

https://codereview.chromium.org/2063013004/diff/40001/test/mjsunit/wasm/debug...
File test/mjsunit/wasm/debug-disassembly.js (right):

https://codereview.chromium.org/2063013004/diff/40001/test/mjsunit/wasm/debug...
test/mjsunit/wasm/debug-disassembly.js:48: if (name.endsWith("/0")) {
On 2016/06/17 06:57:50, ahaas wrote:
> What's the idea behind this condition? Can you add a comment when name ends
with
> "/0"?

Done.

[Yang, 2016-06-17 18:16:31]

lgtm

https://codereview.chromium.org/2063013004/diff/60001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2063013004/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:813: ->SetBreakPoint(*source_position);
The reason we pass in a pointer for source_position is because the actual break
location may not be at the exact specified source position, so we reflect the
change through this by-reference argument.

Is it possible to set break points at arbitrary code offsets for wasm?

Also, does this actually already work? I don't see any test case for this.

https://codereview.chromium.org/2063013004/diff/60001/src/debug/debug.js
File src/debug/debug.js (right):

https://codereview.chromium.org/2063013004/diff/60001/src/debug/debug.js#newc...
src/debug/debug.js:874: Debug.getWasmFunctionOffsetTable = function(scriptId) {
Please add comments that these two methods are intended for source maps.

https://codereview.chromium.org/2063013004/diff/60001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2063013004/diff/60001/src/objects-inl.h#newco...
src/objects-inl.h:5583: this->type() != TYPE_WASM)
we should be able to assert here that compilation type is COMPILATION_TYPE_EVAL

[Clemens Hammacher, 2016-06-24 15:12:33]

@ahaas: same here, missing LGTM

https://codereview.chromium.org/2063013004/diff/60001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2063013004/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:813: ->SetBreakPoint(*source_position);
On 2016/06/17 18:16:31, Yang wrote:
> The reason we pass in a pointer for source_position is because the actual
break
> location may not be at the exact specified source position, so we reflect the
> change through this by-reference argument.
> 
> Is it possible to set break points at arbitrary code offsets for wasm?
> 
> Also, does this actually already work? I don't see any test case for this.

No, the called function just returned false.
I thus removed it for now, and just return false here immediately.

https://codereview.chromium.org/2063013004/diff/60001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2063013004/diff/60001/src/objects-inl.h#newco...
src/objects-inl.h:5583: this->type() != TYPE_WASM)
On 2016/06/17 18:16:31, Yang wrote:
> we should be able to assert here that compilation type is
COMPILATION_TYPE_EVAL

Nope, it already fails in mksnapshot. I'll leave it like this for now.

[Clemens Hammacher, 2016-06-24 15:45:44]

https://codereview.chromium.org/2063013004/diff/60001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2063013004/diff/60001/src/objects-inl.h#newco...
src/objects-inl.h:5583: this->type() != TYPE_WASM)
On 2016/06/24 15:12:33, Clemens Hammacher wrote:
> On 2016/06/17 18:16:31, Yang wrote:
> > we should be able to assert here that compilation type is
> COMPILATION_TYPE_EVAL
> 
> Nope, it already fails in mksnapshot. I'll leave it like this for now.

Short follow-up: The check fails since the check is also performed on the
setter, and the setter is called unconditionally from Factory::NewScript.

[titzer, 2016-06-29 09:35:04]

lgtm

[titzer, 2016-06-29 09:35:09]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-06-29 09:35:09]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2063013004/#ps80001
(title: "remove unimplemented wasm SetBreakPoint method")

[commit-bot: I haz the power, 2016-06-29 09:35:24]

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

[commit-bot: I haz the power, 2016-06-29 09:35:25]

This CL has an open dependency (Issue 2069823003 Patch 120001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[titzer, 2016-06-29 12:00:35]

Description was changed from

==========
[wasm] Disassemble wasm code from script

This stores the wasm object and the function index in the script, and
adds functions to get the disassembled wasm code as well as the offset
table mapping from byte position to line and column in the disassembly
solely from the script.
This will be used to show "ui source code" in DevTools, and map raw
locations from the stack trace into this code view.

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

to

==========
[wasm] Disassemble wasm code from script

This stores the wasm object and the function index in the script, and
adds functions to get the disassembled wasm code as well as the offset
table mapping from byte position to line and column in the disassembly
solely from the script.
This will be used to show "ui source code" in DevTools, and map raw
locations from the stack trace into this code view.

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

[titzer, 2016-06-29 12:00:41]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2016-06-29 12:00:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 12:03:09]

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

[commit-bot: I haz the power, 2016-06-29 12:03:10]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/8223)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8203)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/8183)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/18382)

[titzer, 2016-06-29 12:17:11]

Description was changed from

==========
[wasm] Disassemble wasm code from script

This stores the wasm object and the function index in the script, and
adds functions to get the disassembled wasm code as well as the offset
table mapping from byte position to line and column in the disassembly
solely from the script.
This will be used to show "ui source code" in DevTools, and map raw
locations from the stack trace into this code view.

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

to

==========
[wasm] Disassemble wasm code from script

This stores the wasm object and the function index in the script, and
adds functions to get the disassembled wasm code as well as the offset
table mapping from byte position to line and column in the disassembly
solely from the script.
This will be used to show "ui source code" in DevTools, and map raw
locations from the stack trace into this code view.

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

[titzer, 2016-06-29 12:18:23]

On 2016/06/29 12:03:10, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
>   v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
>   v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/8223)
>   v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
>   v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8203)
>   v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
>   v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/8183)
>   v8_presubmit on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/18382)

The patch seems to be failing due to the dependency which was reuploaded and
landed.

Superceded by: https://codereview.chromium.org/2105303002

[titzer, 2016-06-30 14:17:12]

titzer@chromium.org changed reviewers:
- ahaas@chromium.org, titzer@chromium.org, yangguo@chromium.org