[wasm] Split off debug info from wasm object

[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

[Clemens Hammacher, 2016-06-10 09:50:40]

Description was changed from

==========
[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later, a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

to

==========
[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

[Clemens Hammacher, 2016-06-10 09:52:33]

Will rebase later. All tests pass locally.

[titzer, 2016-06-10 10:59:26]

https://codereview.chromium.org/2050953003/diff/1/src/wasm/wasm-debug.h
File src/wasm/wasm-debug.h (right):

https://codereview.chromium.org/2050953003/diff/1/src/wasm/wasm-debug.h#newco...
src/wasm/wasm-debug.h:15: class WasmDebugInfo : public JSObject {
Why not a fixed array?

[clemensh, 2016-06-10 11:06:02]

https://codereview.chromium.org/2050953003/diff/1/src/wasm/wasm-debug.h
File src/wasm/wasm-debug.h (right):

https://codereview.chromium.org/2050953003/diff/1/src/wasm/wasm-debug.h#newco...
src/wasm/wasm-debug.h:15: class WasmDebugInfo : public JSObject {
On 2016/06/10 10:59:26, titzer wrote:
> Why not a fixed array?

Where is the benefit? Is it more lightweight?
I thought an empty JSObject with just internal fields would encapsulate things
better. It's harder for external users to mess up the internal information.

[titzer, 2016-06-10 11:07:53]

On 2016/06/10 11:06:02, clemensh wrote:
> https://codereview.chromium.org/2050953003/diff/1/src/wasm/wasm-debug.h
> File src/wasm/wasm-debug.h (right):
> 
>
https://codereview.chromium.org/2050953003/diff/1/src/wasm/wasm-debug.h#newco...
> src/wasm/wasm-debug.h:15: class WasmDebugInfo : public JSObject {
> On 2016/06/10 10:59:26, titzer wrote:
> > Why not a fixed array?
> 
> Where is the benefit? Is it more lightweight?
> I thought an empty JSObject with just internal fields would encapsulate things
> better. It's harder for external users to mess up the internal information.

JSObjects inherit the whole JS junk, whereas a FixedArray is a V8 internal thing
that should never escape to JavaScript.

[clemensh, 2016-06-15 17:16:54]

Description was changed from

==========
[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

to

==========
[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

[clemensh, 2016-06-15 17:16:54]

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

[Clemens Hammacher, 2016-06-15 17:18:16]

>
https://codereview.chromium.org/2050953003/diff/1/src/wasm/wasm-debug.h#newco...
> > src/wasm/wasm-debug.h:15: class WasmDebugInfo : public JSObject {
> > On 2016/06/10 10:59:26, titzer wrote:
> > > Why not a fixed array?
> > 
> > Where is the benefit? Is it more lightweight?
> > I thought an empty JSObject with just internal fields would encapsulate
things
> > better. It's harder for external users to mess up the internal information.
> 
> JSObjects inherit the whole JS junk, whereas a FixedArray is a V8 internal
thing
> that should never escape to JavaScript.

Ah, thanks for the explanation. Done!

[ahaas, 2016-06-16 12:55:09]

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc
File src/wasm/wasm-debug.cc (right):

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:50: DCHECK_EQ(arr->length(), idx * sizeof(int));
ByteArray uses kIntSize internally instead of sizeof(int), where kIntSize is
defined as sizeof(int), I think you should also use kIntSize.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:60: int funcs_in_arr = arr->length() / sizeof(int) / 2;
same here.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:61: if (func_index < 0 || func_index >= funcs_in_arr)
return {0, 0};
Are func_index < 0 and func_index >= funcs_in_array valid inputs? If not,
wouldn't it be better to replace this {if} by a DCHECK? Anyways, this check
should be the first line of the function.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:70: Vector<const uint8_t> GetFunctionBytes(WasmDebugInfo
*debug_info,
Why don't you make this a private function of WasmDebugInfo?

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:97: Handle<Object> hash_obj =
factory->NewNumberFromInt(hash, TENURED);
Is it guaranteed that {hash} is a SMI? If not, then NewNumberFromInt will
allocate a HeapNumber and therefore crash because of the DisallowHeapAllocation
scope. I think it would be nicer anyways to use handle(Smi::FromInt(...))
instead of NewNumberFromInt because of the DisallowHeapAllocation scope.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:116: return static_cast<WasmDebugInfo *>(object);
For consistency, could you use a reinterpret_cast here, as all object types in
objects-inl.h do it?

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:123: bool WasmDebugInfo::SetBreakPoint(int byte_offset) {
return false; }
Add a to-be-implemented here?

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:163: bool ok = PrintAst(
Is PrintAst supposed to be used like this?

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:213: // TODO(clemensh): Remove function name array,
extract names from module bytes.
Will you have time to do this? In not, I think you should replace it with
TODO(wasm).

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:710: DCHECK_EQ(instance.module->module_end -
instance.module->module_start,
I don't think that this DCHECK is necessary. I would trust the compiler to do
normal arithmetic correctly.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:864: wasm::GetDebugInfo(*instance.js_object);
In GetDebugInfo you access the internal field kWasmDebugInfo before you
initialize it. Could you please add a comment which says why this is okay, if it
is okay?

[clemensh, 2016-06-16 15:59:50]

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc
File src/wasm/wasm-debug.cc (right):

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:50: DCHECK_EQ(arr->length(), idx * sizeof(int));
On 2016/06/16 12:55:09, ahaas wrote:
> ByteArray uses kIntSize internally instead of sizeof(int), where kIntSize is
> defined as sizeof(int), I think you should also use kIntSize.

Thanks for the hint. I also found kMaxInt in globals.h, and use that instead of
std::numeric_limit<int>::max().

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:60: int funcs_in_arr = arr->length() / sizeof(int) / 2;
On 2016/06/16 12:55:09, ahaas wrote:
> same here.

Done.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:61: if (func_index < 0 || func_index >= funcs_in_arr)
return {0, 0};
On 2016/06/16 12:55:09, ahaas wrote:
> Are func_index < 0 and func_index >= funcs_in_array valid inputs? If not,
> wouldn't it be better to replace this {if} by a DCHECK? Anyways, this check
> should be the first line of the function.

Yeah, that's always the question. func_index somehow comes from "external", but
yes, it should be valid.
Changed it to DCHECK, but can't move it further up because we first have to
compute the number of functions.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:70: Vector<const uint8_t> GetFunctionBytes(WasmDebugInfo
*debug_info,
On 2016/06/16 12:55:09, ahaas wrote:
> Why don't you make this a private function of WasmDebugInfo?

Because I would have to add it to the header, and I would like to avoid exposing
such internals in the header, if possible.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:97: Handle<Object> hash_obj =
factory->NewNumberFromInt(hash, TENURED);
On 2016/06/16 12:55:09, ahaas wrote:
> Is it guaranteed that {hash} is a SMI? If not, then NewNumberFromInt will
> allocate a HeapNumber and therefore crash because of the
DisallowHeapAllocation
> scope. I think it would be nicer anyways to use handle(Smi::FromInt(...))
> instead of NewNumberFromInt because of the DisallowHeapAllocation scope.

If a Smi has less than 32 bit, then it could not fit. The heap allocation is out
of the scope of DisallowHeapAllocation though, so it should not be a problem.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:116: return static_cast<WasmDebugInfo *>(object);
On 2016/06/16 12:55:09, ahaas wrote:
> For consistency, could you use a reinterpret_cast here, as all object types in
> objects-inl.h do it?

Done.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:123: bool WasmDebugInfo::SetBreakPoint(int byte_offset) {
return false; }
On 2016/06/16 12:55:09, ahaas wrote:
> Add a to-be-implemented here?

Done.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:163: bool ok = PrintAst(
On 2016/06/16 12:55:09, ahaas wrote:
> Is PrintAst supposed to be used like this?

What do you mean by "like this"?
It is the function to compute a text representation of wasm code, and it
computes the offset table. Here, we only need the latter, so I pass a null
stream for the text representation.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:213: // TODO(clemensh): Remove function name array,
extract names from module bytes.
On 2016/06/16 12:55:09, ahaas wrote:
> Will you have time to do this? In not, I think you should replace it with
> TODO(wasm).

If needed, I will do this at the very end for all the remaining TODOs.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:710: DCHECK_EQ(instance.module->module_end -
instance.module->module_start,
On 2016/06/16 12:55:09, ahaas wrote:
> I don't think that this DCHECK is necessary. I would trust the compiler to do
> normal arithmetic correctly.

This is to check for int-overflows. I refactored this to make it more readable.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:864: wasm::GetDebugInfo(*instance.js_object);
On 2016/06/16 12:55:09, ahaas wrote:
> In GetDebugInfo you access the internal field kWasmDebugInfo before you
> initialize it. Could you please add a comment which says why this is okay, if
it
> is okay?

Internal fields are auto-initialized to undefined.
But I just realized that the early instantiation of the debug info is not
required any more, so I just removed this code completely :)

[ahaas, 2016-06-16 19:42:19]

lgtm

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc
File src/wasm/wasm-debug.cc (right):

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:70: Vector<const uint8_t> GetFunctionBytes(WasmDebugInfo
*debug_info,
On 2016/06/16 at 15:59:50, clemensh wrote:
> On 2016/06/16 12:55:09, ahaas wrote:
> > Why don't you make this a private function of WasmDebugInfo?
> 
> Because I would have to add it to the header, and I would like to avoid
exposing such internals in the header, if possible.

Isn't that what private methods are for?

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:97: Handle<Object> hash_obj =
factory->NewNumberFromInt(hash, TENURED);
On 2016/06/16 at 15:59:50, clemensh wrote:
> On 2016/06/16 12:55:09, ahaas wrote:
> > Is it guaranteed that {hash} is a SMI? If not, then NewNumberFromInt will
> > allocate a HeapNumber and therefore crash because of the
DisallowHeapAllocation
> > scope. I think it would be nicer anyways to use handle(Smi::FromInt(...))
> > instead of NewNumberFromInt because of the DisallowHeapAllocation scope.
> 
> If a Smi has less than 32 bit, then it could not fit. The heap allocation is
out of the scope of DisallowHeapAllocation though, so it should not be a
problem.

True, I missed the ending scope.

https://codereview.chromium.org/2050953003/diff/80001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:163: bool ok = PrintAst(
On 2016/06/16 at 15:59:50, clemensh wrote:
> On 2016/06/16 12:55:09, ahaas wrote:
> > Is PrintAst supposed to be used like this?
> 
> What do you mean by "like this"?
> It is the function to compute a text representation of wasm code, and it
computes the offset table. Here, we only need the latter, so I pass a null
stream for the text representation.

I was just surprised that a function called PrintAst provides computations.

[Clemens Hammacher, 2016-06-17 08:07:30]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2016-06-17 08:07:37]

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

[commit-bot: I haz the power, 2016-06-17 08:33:42]

Description was changed from

==========
[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

to

==========
[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

[commit-bot: I haz the power, 2016-06-17 08:33:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-17 08:35:19]

Description was changed from

==========
[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

to

==========
[wasm] Split off debug info from wasm object

All debugging-related information is now stored inside a dedicated
object, which is only allocated if debugging support is needed.
This is also where later a reference to the interpreter will be stored
for executing to-be-debugged functions and providing stack inspection.

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

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

[commit-bot: I haz the power, 2016-06-17 08:35:20]

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