[wasm] Enable wasm frame inspection for debugging

[wasm] Enable wasm frame inspection for debugging

This changes many interfaces to accept StandardFrames instead of
JavaScriptFrames, and use the StackTraceFrameIterator instead of the
JavaScriptFrameIterator.
Also, the detailed frame information array now contains the script in
addition to the function, as wasm frames are not associated to any
javascript function.

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

[Yang, 2016-06-16 14:10:42]

I would be fine with changing JavaScriptFrameIterator to
StackTraceFrameIterator, but I'm very unhappy about adding is_wasm checks all
over the place. Imo the fact that a frame is wasm should be hidden. The debugger
almost never has to know about that fact. It often just needs to know the
JSFunction associated to a frame, the receiver (which I assume would be
undefined), the context, expressions on the stack, parameters on the stack, the
code object, the code offset in that code object, and a way to compute source
position from code object and code offset.

For bytecode, full code and optimized code this is mostly hidden in the
FrameSummary and virtual methods of the frame. Why can't we have that for wasm
frames? If I understood correctly you do have a JSFunction wrapper for every
piece of wasm code.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames.cc
File src/debug/debug-frames.cc (right):

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:55: return
javascript_frame()->ComputeParametersCount();
Can we make ComputeParameterCount a virtual method on StandardFrame that returns
0 for wasm frames?

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:60: DCHECK(wasm_frame()->wasm_obj()->IsJSObject());
I wish we don't have to check against is_wasm everywhere. Can't we simply have
GetFunction return the correct function wrapper for the wasm code, from which we
can then get its SharedFunctionInfo and script?

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:81: return
handle(javascript_frame()->GetParameter(index), isolate_);
Same here. GetParameter could be a virtual method on StandardFrame.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:122: : javascript_frame()->IsConstructor();
Same here.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:128: return handle(javascript_frame()->context(),
isolate_);
And here.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames.h
File src/debug/debug-frames.h (right):

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.h:18: FrameInspector(StandardFrame* frame, int
inlined_jsframe_index,
Does it make sense to allow the FrameInspector to take a wasm frame?

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:37: return
FrameSummary::GetFirst(JavaScriptFrame::cast(frame)).code_offset();
Is it not feasible to extend FrameSummary for wasm frames?

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1638: int code_offset = GetCodeOffset(frame);
At this point the frame is already known to be a javascript frame, right?

https://codereview.chromium.org/2069823003/diff/20001/src/debug/mirrors.js
File src/debug/mirrors.js (right):

https://codereview.chromium.org/2069823003/diff/20001/src/debug/mirrors.js#ne...
src/debug/mirrors.js:1690: FrameDetails.prototype.script = function() {
Can we get this from the FunctionMirror?

[Clemens Hammacher, 2016-06-16 17:02:31]

Thanks for the detailed review.

Looking at the code, I thought that the FrameInspector would be the instance
arbitrating between the different frame types and providing a unified interface.
Seems like I was wrong.
I now pulled some accessors (script, context, GetParameter,
ComputeParametersCount, IsConstructor) up to the StandardFrame. This removes
most of the is_wasm checks from the FrameInspector. It also allows to remove the
wasm-debug include from debug-frames.cc and isolate.cc.

I think it's a much clearer separation now.

Regarding the JSFunction wrappers: We discarded the idea of materializing the
JSFunction wrapper for wasm functions on demand, since we really don't need it
for anything other than referencing the script (via SharedFunctionInfo, which we
also don't need). Instead, we just create the Scripts on demand, and (now) allow
to access them from the frame.

I also replied to all the individual comments below.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames.cc
File src/debug/debug-frames.cc (right):

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:55: return
javascript_frame()->ComputeParametersCount();
On 2016/06/16 14:10:41, Yang wrote:
> Can we make ComputeParameterCount a virtual method on StandardFrame that
returns
> 0 for wasm frames?

Done.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:60: DCHECK(wasm_frame()->wasm_obj()->IsJSObject());
On 2016/06/16 14:10:41, Yang wrote:
> I wish we don't have to check against is_wasm everywhere. Can't we simply have
> GetFunction return the correct function wrapper for the wasm code, from which
we
> can then get its SharedFunctionInfo and script?

We don't have a JSFunction for wasm frames, that's why we need to get the script
otherwise. But I pulled the script accessor up to the standard frame.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:81: return
handle(javascript_frame()->GetParameter(index), isolate_);
On 2016/06/16 14:10:41, Yang wrote:
> Same here. GetParameter could be a virtual method on StandardFrame.

Done.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:122: : javascript_frame()->IsConstructor();
On 2016/06/16 14:10:41, Yang wrote:
> Same here.

Done.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.cc:128: return handle(javascript_frame()->context(),
isolate_);
On 2016/06/16 14:10:41, Yang wrote:
> And here.

Done.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames.h
File src/debug/debug-frames.h (right):

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug-frames....
src/debug/debug-frames.h:18: FrameInspector(StandardFrame* frame, int
inlined_jsframe_index,
On 2016/06/16 14:10:41, Yang wrote:
> Does it make sense to allow the FrameInspector to take a wasm frame?

It needs to. It's the main tool to access the frame information.
But after the latest changes, there is much less wasm-specific code in it now.

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:37: return
FrameSummary::GetFirst(JavaScriptFrame::cast(frame)).code_offset();
On 2016/06/16 14:10:41, Yang wrote:
> Is it not feasible to extend FrameSummary for wasm frames?

FrameSummary is very JS-specific, and it's only purpose is to encapsulate the
information for a frame with inlined functions.
It seemed inappropriate to try to put wasm information in there (I also
discussed this with Ben several weeks ago).

https://codereview.chromium.org/2069823003/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1638: int code_offset = GetCodeOffset(frame);
On 2016/06/16 14:10:41, Yang wrote:
> At this point the frame is already known to be a javascript frame, right?

Right, but calling the function we already have seemed clearer to me than to
duplicate code here.
I could extract a GetJavaScriptCodeOffset function, and use it here and from
GetCodeOffset. Would you prefer that?

https://codereview.chromium.org/2069823003/diff/20001/src/debug/mirrors.js
File src/debug/mirrors.js (right):

https://codereview.chromium.org/2069823003/diff/20001/src/debug/mirrors.js#ne...
src/debug/mirrors.js:1690: FrameDetails.prototype.script = function() {
On 2016/06/16 14:10:41, Yang wrote:
> Can we get this from the FunctionMirror?

Nope, the FunctionMirror is an UnresolvedFunctionMirror for wasm frames.

[ahaas, 2016-06-17 07:25:44]

wasm looks good

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

https://codereview.chromium.org/2069823003/diff/40001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:148: int func_bytes_len = offset_arr->get_int(2 *
func_index + 1);
I think it could be a good idea to add a wrapper function around accesses to
offset_arr to hide 2 * i + 1.

https://codereview.chromium.org/2069823003/diff/40001/test/mjsunit/wasm/frame...
File test/mjsunit/wasm/frame-inspection.js (right):

https://codereview.chromium.org/2069823003/diff/40001/test/mjsunit/wasm/frame...
test/mjsunit/wasm/frame-inspection.js:35: if (wasm_script === null) wasm_script
= script;
what's the purpose of this if?

[Clemens Hammacher, 2016-06-17 14:01:46]

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

https://codereview.chromium.org/2069823003/diff/40001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:148: int func_bytes_len = offset_arr->get_int(2 *
func_index + 1);
On 2016/06/17 07:25:44, ahaas wrote:
> I think it could be a good idea to add a wrapper function around accesses to
> offset_arr to hide 2 * i + 1.

Done. This now calls GetFunctionOffsetAndLength.

https://codereview.chromium.org/2069823003/diff/40001/test/mjsunit/wasm/frame...
File test/mjsunit/wasm/frame-inspection.js (right):

https://codereview.chromium.org/2069823003/diff/40001/test/mjsunit/wasm/frame...
test/mjsunit/wasm/frame-inspection.js:35: if (wasm_script === null) wasm_script
= script;
On 2016/06/17 07:25:44, ahaas wrote:
> what's the purpose of this if?

You are right, it's pointless :)
I removed it.

[clemensh, 2016-06-20 13:02:58]

@Yang: PTAL

[Yang, 2016-06-20 13:43:40]

I'm a bit hesitant with this CL. This changes JavaScriptFrameIterator to
StackTraceFrameIterator all over the place, without actually introducing support
in most places. Setting break points does not work for wasm frames, yet we have
now use StandardFrame as argument for BreakLocation::FromFrame and other places.
Retrieving information for wasm makes no sense at all, but we now use
StackTraceFrameIterator here as well.

Without test cases I suspect that now more things would break with wasm than
before, where we would simply skip wasm frames.

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

https://codereview.chromium.org/2069823003/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:812: // TODO(clemensh): set breakpoint for wasm.
Can we return false here? This seems untested and it doesn't seem it would work
below.

https://codereview.chromium.org/2069823003/diff/60001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2069823003/diff/60001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:834: it.MaterializeScopeDetails());
Can this even work for wasm frames?

[Yang, 2016-06-20 13:50:44]

On 2016/06/20 13:43:40, Yang wrote:
> I'm a bit hesitant with this CL. This changes JavaScriptFrameIterator to
> StackTraceFrameIterator all over the place, without actually introducing
support
> in most places. Setting break points does not work for wasm frames, yet we
have
> now use StandardFrame as argument for BreakLocation::FromFrame and other
places.
> Retrieving information for wasm makes no sense at all, but we now use
> StackTraceFrameIterator here as well.
> 
> Without test cases I suspect that now more things would break with wasm than
> before, where we would simply skip wasm frames.
> 
> https://codereview.chromium.org/2069823003/diff/60001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2069823003/diff/60001/src/debug/debug.cc#newc...
> src/debug/debug.cc:812: // TODO(clemensh): set breakpoint for wasm.
> Can we return false here? This seems untested and it doesn't seem it would
work
> below.
> 
>
https://codereview.chromium.org/2069823003/diff/60001/src/runtime/runtime-deb...
> File src/runtime/runtime-debug.cc (right):
> 
>
https://codereview.chromium.org/2069823003/diff/60001/src/runtime/runtime-deb...
> src/runtime/runtime-debug.cc:834: it.MaterializeScopeDetails());
> Can this even work for wasm frames?

I meant to say "retrieving scope information". From what I can tell we don't
have any wasm checks before trying to materialize scopes.

[Yang, 2016-06-21 20:09:13]

On 2016/06/20 13:50:44, Yang wrote:
> On 2016/06/20 13:43:40, Yang wrote:
> > I'm a bit hesitant with this CL. This changes JavaScriptFrameIterator to
> > StackTraceFrameIterator all over the place, without actually introducing
> support
> > in most places. Setting break points does not work for wasm frames, yet we
> have
> > now use StandardFrame as argument for BreakLocation::FromFrame and other
> places.
> > Retrieving information for wasm makes no sense at all, but we now use
> > StackTraceFrameIterator here as well.
> > 
> > Without test cases I suspect that now more things would break with wasm than
> > before, where we would simply skip wasm frames.
> > 
> > https://codereview.chromium.org/2069823003/diff/60001/src/debug/debug.cc
> > File src/debug/debug.cc (right):
> > 
> >
>
https://codereview.chromium.org/2069823003/diff/60001/src/debug/debug.cc#newc...
> > src/debug/debug.cc:812: // TODO(clemensh): set breakpoint for wasm.
> > Can we return false here? This seems untested and it doesn't seem it would
> work
> > below.
> > 
> >
>
https://codereview.chromium.org/2069823003/diff/60001/src/runtime/runtime-deb...
> > File src/runtime/runtime-debug.cc (right):
> > 
> >
>
https://codereview.chromium.org/2069823003/diff/60001/src/runtime/runtime-deb...
> > src/runtime/runtime-debug.cc:834: it.MaterializeScopeDetails());
> > Can this even work for wasm frames?
> 
> I meant to say "retrieving scope information". From what I can tell we don't
> have any wasm checks before trying to materialize scopes.

Can we limit the scope of this CL to what the CL title suggests? This is
supposed to add introspection to wasm frames, and only provides test for that.
It should not affect break locations, scopes, etc, which is untested.

[Clemens Hammacher, 2016-06-23 13:26:10]

I removed some more explicit wasm checks, so the changes in debug-scopes and
other unrelated files are minimal now.

Please write back if you find any opportunities that I oversaw.

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

https://codereview.chromium.org/2069823003/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:812: // TODO(clemensh): set breakpoint for wasm.
On 2016/06/20 13:43:39, Yang wrote:
> Can we return false here? This seems untested and it doesn't seem it would
work
> below.

Done.

https://codereview.chromium.org/2069823003/diff/60001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2069823003/diff/60001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:834: it.MaterializeScopeDetails());
On 2016/06/20 13:43:39, Yang wrote:
> Can this even work for wasm frames? 

The ScopeIterator does some checks internally to sort out optimized frames and
other things. For wasm, it find that frame_inspector->GetContext() is undefined,
so it returns an empty iterator.

[Yang, 2016-06-24 05:56:37]

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

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:286: StandardFrame* frame) {
Let's keep using JavaScriptFrame as argument type.

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:287: int code_offset = GetCodeOffset(frame);
GetCodeOffset becomes unnecessary.

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1623: bool Debug::IsBreakAtReturn(StandardFrame* frame) {
See comment in runtime-debug.cc, let's keep JavaScriptFrame.

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1639: int code_offset = GetCodeOffset(frame);
Can we pass js_frame to GetCodeOffset and not change its signature?

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:2333: has_frames ? it.frame()->id() : StackFrame::NO_ID;
We currently cannot break in wasm code at all. Let's keep the
JavaScriptFrameIterator.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:546: Handle<FixedArray> locals;
Can we re-organize this file like this?

if (it.is_wasm()) {
  [all wasm-relevant stuff goes here]

} else {
  JavaScriptFrame* js_frame = ...
  [everything else goes here]
}

I would also put the check for IsBreakAtReturn into the else-block and avoid
changing its argument type to StandardFrame.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:826: StandardFrame* frame = frame_it.frame();
We need to return an empty array if the frame is not javascript.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:834: it.MaterializeScopeDetails());
I don't think we can correctly materialize any scope details for wasm frames
here.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1585: Handle<String> sourceText = info.line_end >
source->length()
can we do a script->is_wasm check instead?

[Clemens Hammacher, 2016-06-24 09:23:32]

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

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:286: StandardFrame* frame) {
On 2016/06/24 05:56:37, Yang wrote:
> Let's keep using JavaScriptFrame as argument type.

Done.

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:287: int code_offset = GetCodeOffset(frame);
On 2016/06/24 05:56:37, Yang wrote:
> GetCodeOffset becomes unnecessary.

Done.

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1623: bool Debug::IsBreakAtReturn(StandardFrame* frame) {
On 2016/06/24 05:56:37, Yang wrote:
> See comment in runtime-debug.cc, let's keep JavaScriptFrame.

Done.

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1639: int code_offset = GetCodeOffset(frame);
On 2016/06/24 05:56:37, Yang wrote:
> Can we pass js_frame to GetCodeOffset and not change its signature?

Done, and removed the GetCodeOffset function; it's inlined again, as before.

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:2333: has_frames ? it.frame()->id() : StackFrame::NO_ID;
On 2016/06/24 05:56:37, Yang wrote:
> We currently cannot break in wasm code at all. Let's keep the
> JavaScriptFrameIterator.

The break_id stored here is used in Runtime_GetFrameCount to advance the
iterator up to this point. If we use a JavaScriptFrameIterator here and a
StackTraceFrameIterator there, we might advance the latter too far, as it
ignores native frames. If the top frame is native, it never sees its id and runs
till the end.
I am not sure whether these small semantic differences between the different
frame iterators are necessary, but by using the same iterator, we can easily
ensure that it will match.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:546: Handle<FixedArray> locals;
On 2016/06/24 05:56:37, Yang wrote:
> Can we re-organize this file like this?
> 
> if (it.is_wasm()) {
>   [all wasm-relevant stuff goes here]
> 
> } else {
>   JavaScriptFrame* js_frame = ...
>   [everything else goes here]
> }
> 
> I would also put the check for IsBreakAtReturn into the else-block and avoid
> changing its argument type to StandardFrame.

Done.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:826: StandardFrame* frame = frame_it.frame();
On 2016/06/24 05:56:37, Yang wrote:
> We need to return an empty array if the frame is not javascript.

This happens implicitely, as the ScopeIterator will be empty for wasm frames.
You really prefer an explicit check?

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:834: it.MaterializeScopeDetails());
On 2016/06/24 05:56:37, Yang wrote:
> I don't think we can correctly materialize any scope details for wasm frames
> here.

See above. This will generate an empty array.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:1585: Handle<String> sourceText = info.line_end >
source->length()
On 2016/06/24 05:56:37, Yang wrote:
> can we do a script->is_wasm check instead?

Done.

[Yang, 2016-06-24 10:02:57]

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

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:2333: has_frames ? it.frame()->id() : StackFrame::NO_ID;
On 2016/06/24 09:23:31, Clemens Hammacher wrote:
> On 2016/06/24 05:56:37, Yang wrote:
> > We currently cannot break in wasm code at all. Let's keep the
> > JavaScriptFrameIterator.
> 
> The break_id stored here is used in Runtime_GetFrameCount to advance the
> iterator up to this point. If we use a JavaScriptFrameIterator here and a
> StackTraceFrameIterator there, we might advance the latter too far, as it
> ignores native frames. If the top frame is native, it never sees its id and
runs
> till the end.
> I am not sure whether these small semantic differences between the different
> frame iterators are necessary, but by using the same iterator, we can easily
> ensure that it will match.

Not sure I understand your explanation...

Since we cannot break in wasm frames, JavaScriptFrameIterator and
StackTraceFrameIterator should find the same frame here. The frame id is not
dependent on which iterator you use.

Runtime_GetFrameCount can only be called with a debug scope in place, so inside
a break, so the top-most frame has to be a Javascript frame anyways. The
StackTraceFrameIterator in Runtime_GetFrameCount should still find the same
frame by id, since StackTraceFrameIterator iterates over a superset of frames.

By using JavaScriptFrameIterator however, we make sure that we are indeed not
leaking wasm frames into the debugger wrt breaking, where we require the use of
a debug scope, and not accidentally violate this invariant. Wasm frames would
simply be ignored for breaking.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:826: StandardFrame* frame = frame_it.frame();
On 2016/06/24 09:23:31, Clemens Hammacher wrote:
> On 2016/06/24 05:56:37, Yang wrote:
> > We need to return an empty array if the frame is not javascript.
> 
> This happens implicitely, as the ScopeIterator will be empty for wasm frames.
> You really prefer an explicit check?

It will be empty because of the check in ScopeIterator's constructor:

!frame_inspector->GetFunction()->IsJSFunction()

Correct? That seems very brittle, relying on the fact that we don't have a
JSFunction. I'd be happier if the constructor had an explicit check against wasm
frames.

[Clemens Hammacher, 2016-06-24 10:45:42]

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

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:2333: has_frames ? it.frame()->id() : StackFrame::NO_ID;
On 2016/06/24 10:02:57, Yang wrote:
> On 2016/06/24 09:23:31, Clemens Hammacher wrote:
> > On 2016/06/24 05:56:37, Yang wrote:
> > > We currently cannot break in wasm code at all. Let's keep the
> > > JavaScriptFrameIterator.
> > 
> > The break_id stored here is used in Runtime_GetFrameCount to advance the
> > iterator up to this point. If we use a JavaScriptFrameIterator here and a
> > StackTraceFrameIterator there, we might advance the latter too far, as it
> > ignores native frames. If the top frame is native, it never sees its id and
> runs
> > till the end.
> > I am not sure whether these small semantic differences between the different
> > frame iterators are necessary, but by using the same iterator, we can easily
> > ensure that it will match.
> 
> Not sure I understand your explanation...
> 
> Since we cannot break in wasm frames, JavaScriptFrameIterator and
> StackTraceFrameIterator should find the same frame here. The frame id is not
> dependent on which iterator you use.
> 
> Runtime_GetFrameCount can only be called with a debug scope in place, so
inside
> a break, so the top-most frame has to be a Javascript frame anyways. The
> StackTraceFrameIterator in Runtime_GetFrameCount should still find the same
> frame by id, since StackTraceFrameIterator iterates over a superset of frames.
> 
> By using JavaScriptFrameIterator however, we make sure that we are indeed not
> leaking wasm frames into the debugger wrt breaking, where we require the use
of
> a debug scope, and not accidentally violate this invariant. Wasm frames would
> simply be ignored for breaking.

Unfortunately, the frame id is dependent on the iterator.
I specifically looked at one of the tests which fail if I change the iterator.
In this test, the top frame is a JavaScript frame with type TYPE_NATIVE. The
JavaScriptFrameIterator returns this frame, so its id is stored as
break_frame_id.
The StackTraceFrameIterator does not include this frame, since it only contains
(quote) "proper" JavaScript frames and wasm frames. So when we later use this
iterator to find the break_frame_id, it won't find it and run the iterator till
the end. Runtime_GetFrameCount thus returns 0.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:826: StandardFrame* frame = frame_it.frame();
On 2016/06/24 10:02:57, Yang wrote:
> On 2016/06/24 09:23:31, Clemens Hammacher wrote:
> > On 2016/06/24 05:56:37, Yang wrote:
> > > We need to return an empty array if the frame is not javascript.
> > 
> > This happens implicitely, as the ScopeIterator will be empty for wasm
frames.
> > You really prefer an explicit check?
> 
> It will be empty because of the check in ScopeIterator's constructor:
> 
> !frame_inspector->GetFunction()->IsJSFunction()
> 
> Correct? That seems very brittle, relying on the fact that we don't have a
> JSFunction. I'd be happier if the constructor had an explicit check against
wasm
> frames.

The check is in debug-scopes.cc:25:
if (!frame_inspector->GetContext()->IsContext()) { [...] return; }

Wasm frames don't have a context, and will never (in the foreseeable future ;) )
have one.

[Yang, 2016-06-24 10:52:25]

lgtm with comments.

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

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:2333: has_frames ? it.frame()->id() : StackFrame::NO_ID;
On 2016/06/24 10:45:42, Clemens Hammacher wrote:
> On 2016/06/24 10:02:57, Yang wrote:
> > On 2016/06/24 09:23:31, Clemens Hammacher wrote:
> > > On 2016/06/24 05:56:37, Yang wrote:
> > > > We currently cannot break in wasm code at all. Let's keep the
> > > > JavaScriptFrameIterator.
> > > 
> > > The break_id stored here is used in Runtime_GetFrameCount to advance the
> > > iterator up to this point. If we use a JavaScriptFrameIterator here and a
> > > StackTraceFrameIterator there, we might advance the latter too far, as it
> > > ignores native frames. If the top frame is native, it never sees its id
and
> > runs
> > > till the end.
> > > I am not sure whether these small semantic differences between the
different
> > > frame iterators are necessary, but by using the same iterator, we can
easily
> > > ensure that it will match.
> > 
> > Not sure I understand your explanation...
> > 
> > Since we cannot break in wasm frames, JavaScriptFrameIterator and
> > StackTraceFrameIterator should find the same frame here. The frame id is not
> > dependent on which iterator you use.
> > 
> > Runtime_GetFrameCount can only be called with a debug scope in place, so
> inside
> > a break, so the top-most frame has to be a Javascript frame anyways. The
> > StackTraceFrameIterator in Runtime_GetFrameCount should still find the same
> > frame by id, since StackTraceFrameIterator iterates over a superset of
frames.
> > 
> > By using JavaScriptFrameIterator however, we make sure that we are indeed
not
> > leaking wasm frames into the debugger wrt breaking, where we require the use
> of
> > a debug scope, and not accidentally violate this invariant. Wasm frames
would
> > simply be ignored for breaking.
> 
> Unfortunately, the frame id is dependent on the iterator.
> I specifically looked at one of the tests which fail if I change the iterator.
> In this test, the top frame is a JavaScript frame with type TYPE_NATIVE. The
> JavaScriptFrameIterator returns this frame, so its id is stored as
> break_frame_id.
> The StackTraceFrameIterator does not include this frame, since it only
contains
> (quote) "proper" JavaScript frames and wasm frames. So when we later use this
> iterator to find the break_frame_id, it won't find it and run the iterator
till
> the end. Runtime_GetFrameCount thus returns 0.

I see. Can we at least add an assertion that the frame we find here is not a
wasm frame?

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:826: StandardFrame* frame = frame_it.frame();
On 2016/06/24 10:45:42, Clemens Hammacher wrote:
> On 2016/06/24 10:02:57, Yang wrote:
> > On 2016/06/24 09:23:31, Clemens Hammacher wrote:
> > > On 2016/06/24 05:56:37, Yang wrote:
> > > > We need to return an empty array if the frame is not javascript.
> > > 
> > > This happens implicitely, as the ScopeIterator will be empty for wasm
> frames.
> > > You really prefer an explicit check?
> > 
> > It will be empty because of the check in ScopeIterator's constructor:
> > 
> > !frame_inspector->GetFunction()->IsJSFunction()
> > 
> > Correct? That seems very brittle, relying on the fact that we don't have a
> > JSFunction. I'd be happier if the constructor had an explicit check against
> wasm
> > frames.
> 
> The check is in debug-scopes.cc:25:
> if (!frame_inspector->GetContext()->IsContext()) { [...] return; }
> 
> Wasm frames don't have a context, and will never (in the foreseeable future ;)
)
> have one.

I see. Can we at least add an assertion in ScopeIterator to guard that we don't
run into a wasm frame?

[Clemens Hammacher, 2016-06-24 15:11:58]

@ahaas: I need a proper LGTM, no "wasm looks good" ;)
And can you then check the commit button for me please? Thanks!

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

https://codereview.chromium.org/2069823003/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:2333: has_frames ? it.frame()->id() : StackFrame::NO_ID;
On 2016/06/24 10:52:25, Yang wrote:
> On 2016/06/24 10:45:42, Clemens Hammacher wrote:
> > On 2016/06/24 10:02:57, Yang wrote:
> > > On 2016/06/24 09:23:31, Clemens Hammacher wrote:
> > > > On 2016/06/24 05:56:37, Yang wrote:
> > > > > We currently cannot break in wasm code at all. Let's keep the
> > > > > JavaScriptFrameIterator.
> > > > 
> > > > The break_id stored here is used in Runtime_GetFrameCount to advance the
> > > > iterator up to this point. If we use a JavaScriptFrameIterator here and
a
> > > > StackTraceFrameIterator there, we might advance the latter too far, as
it
> > > > ignores native frames. If the top frame is native, it never sees its id
> and
> > > runs
> > > > till the end.
> > > > I am not sure whether these small semantic differences between the
> different
> > > > frame iterators are necessary, but by using the same iterator, we can
> easily
> > > > ensure that it will match.
> > > 
> > > Not sure I understand your explanation...
> > > 
> > > Since we cannot break in wasm frames, JavaScriptFrameIterator and
> > > StackTraceFrameIterator should find the same frame here. The frame id is
not
> > > dependent on which iterator you use.
> > > 
> > > Runtime_GetFrameCount can only be called with a debug scope in place, so
> > inside
> > > a break, so the top-most frame has to be a Javascript frame anyways. The
> > > StackTraceFrameIterator in Runtime_GetFrameCount should still find the
same
> > > frame by id, since StackTraceFrameIterator iterates over a superset of
> frames.
> > > 
> > > By using JavaScriptFrameIterator however, we make sure that we are indeed
> not
> > > leaking wasm frames into the debugger wrt breaking, where we require the
use
> > of
> > > a debug scope, and not accidentally violate this invariant. Wasm frames
> would
> > > simply be ignored for breaking.
> > 
> > Unfortunately, the frame id is dependent on the iterator.
> > I specifically looked at one of the tests which fail if I change the
iterator.
> > In this test, the top frame is a JavaScript frame with type TYPE_NATIVE. The
> > JavaScriptFrameIterator returns this frame, so its id is stored as
> > break_frame_id.
> > The StackTraceFrameIterator does not include this frame, since it only
> contains
> > (quote) "proper" JavaScript frames and wasm frames. So when we later use
this
> > iterator to find the break_frame_id, it won't find it and run the iterator
> till
> > the end. Runtime_GetFrameCount thus returns 0.
> 
> I see. Can we at least add an assertion that the frame we find here is not a
> wasm frame?

Done.

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/2069823003/diff/80001/src/runtime/runtime-deb...
src/runtime/runtime-debug.cc:826: StandardFrame* frame = frame_it.frame();
On 2016/06/24 10:52:25, Yang wrote:
> On 2016/06/24 10:45:42, Clemens Hammacher wrote:
> > On 2016/06/24 10:02:57, Yang wrote:
> > > On 2016/06/24 09:23:31, Clemens Hammacher wrote:
> > > > On 2016/06/24 05:56:37, Yang wrote:
> > > > > We need to return an empty array if the frame is not javascript.
> > > > 
> > > > This happens implicitely, as the ScopeIterator will be empty for wasm
> > frames.
> > > > You really prefer an explicit check?
> > > 
> > > It will be empty because of the check in ScopeIterator's constructor:
> > > 
> > > !frame_inspector->GetFunction()->IsJSFunction()
> > > 
> > > Correct? That seems very brittle, relying on the fact that we don't have a
> > > JSFunction. I'd be happier if the constructor had an explicit check
against
> > wasm
> > > frames.
> > 
> > The check is in debug-scopes.cc:25:
> > if (!frame_inspector->GetContext()->IsContext()) { [...] return; }
> > 
> > Wasm frames don't have a context, and will never (in the foreseeable future
;)
> )
> > have one.
> 
> I see. Can we at least add an assertion in ScopeIterator to guard that we
don't
> run into a wasm frame?

Done.

[ahaas, 2016-06-27 06:56:28]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-06-27 06:56:29]

lgtm

[ahaas, 2016-06-27 06:56:30]

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/2069823003/#ps120001
(title: "Add two DCHECKs")

[commit-bot: I haz the power, 2016-06-27 06:56:38]

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

[ahaas, 2016-06-27 06:58:40]

The CQ bit was unchecked by ahaas@chromium.org

[titzer, 2016-06-29 09:17:14]

On 2016/06/27 06:56:38, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Looks like this patch no longer applies cleanly, so we'll have to rebase it and
land it in another CL.

[titzer, 2016-06-29 09:23:08]

On 2016/06/29 09:17:14, titzer wrote:
> On 2016/06/27 06:56:38, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Looks like this patch no longer applies cleanly, so we'll have to rebase it
and
> land it in another CL.

Rebased CL: https://codereview.chromium.org/2109093003/

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

Description was changed from

==========
[wasm] Enable wasm frame inspection for debugging

This changes many interfaces to accept StandardFrames instead of
JavaScriptFrames, and use the StackTraceFrameIterator instead of the
JavaScriptFrameIterator.
Also, the detailed frame information array now contains the script in
addition to the function, as wasm frames are not associated to any
javascript function.

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

to

==========
[wasm] Enable wasm frame inspection for debugging

This changes many interfaces to accept StandardFrames instead of
JavaScriptFrames, and use the StackTraceFrameIterator instead of the
JavaScriptFrameIterator.
Also, the detailed frame information array now contains the script in
addition to the function, as wasm frames are not associated to any
javascript function.

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

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

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