[wasm] Make wasm info available on the stack trace

[wasm] Make wasm info available on the stack trace

This changes different locations to extract the reference to the wasm
object and the function index from the stack trace, and make it
available through all the APIs which process stack traces.
The javascript CallSite object now has the new methods isWasm(),
getWasmObject() and getWasmFunctionIndex(); the byte offset is
available via getPosition().

Function names of wasm frames should be fully functional with this
commit, position information works reliably for calls, but not for
traps like unreachable or out-of-bounds accesses.

R=titzer@chromium.org, yangguo@chromium.org

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

[titzer, 2016-04-22 12:16:26]

https://codereview.chromium.org/1909353002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1909353002/diff/80001/src/api.cc#newcode2571
src/api.cc:2571: #define STACKFRAME_PROPERTY_HELPER(obj, propertyName,
MAKE_SCOPE) \
As tempting as this is, let's not macro-ify this code. The macros aren't
hygenic, and we'd be doing it only in these few places. So we hold our noses and
do what the surrounding code does.

https://codereview.chromium.org/1909353002/diff/80001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1909353002/diff/80001/src/frames.h#newcode644
src/frames.h:644: Handle<Object> receiver() const { return receiver_; }
AFAICT FrameSummary instances are already immutable, so the const modifier is
redundant (since all FrameSummary instances and references are implicitly const,
too).

https://codereview.chromium.org/1909353002/diff/80001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/80001/src/messages.cc#newcode204
src/messages.cc:204: if (!IsJavaScript()) return
isolate_->factory()->null_value();
We probably want to use the WASM function name here.

https://codereview.chromium.org/1909353002/diff/80001/src/messages.cc#newcode336
src/messages.cc:336: return IsWasm() || receiver_->IsJSGlobalProxy() ||
receiver_->IsNull() ||
I think isTopLevel for WASM should be false.

[Clemens Hammacher, 2016-04-26 13:58:54]

Description was changed from

==========
[wasm] Make wasm info available on the stack trace

This changes different locations to extract the reference to the wasm
object and the function index from the stack trace, and make it
available through all the APIs which process stack traces.
The javascript CallSite object now has the new methods isWasm(),
getWasmObject() and getWasmFunctionIndex(); the byte offset is
available via getPosition().

Function names of wasm frames should be fully functional with this
commit, position information works reliably for calls, but not for
traps like unreachable or out-of-bounds accesses.

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

to

==========
[wasm] Make wasm info available on the stack trace

This changes different locations to extract the reference to the wasm
object and the function index from the stack trace, and make it
available through all the APIs which process stack traces.
The javascript CallSite object now has the new methods isWasm(),
getWasmObject() and getWasmFunctionIndex(); the byte offset is
available via getPosition().

Function names of wasm frames should be fully functional with this
commit, position information works reliably for calls, but not for
traps like unreachable or out-of-bounds accesses.

R=titzer@chromium.org, yangguo@chromium.org
==========

[Clemens Hammacher, 2016-04-26 13:58:54]

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

[Clemens Hammacher, 2016-04-26 14:00:10]

+yangguo for src/js/messages.js

https://codereview.chromium.org/1909353002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1909353002/diff/80001/src/api.cc#newcode2571
src/api.cc:2571: #define STACKFRAME_PROPERTY_HELPER(obj, propertyName,
MAKE_SCOPE) \
On 2016/04/22 at 12:16:25, titzer wrote:
> As tempting as this is, let's not macro-ify this code. The macros aren't
hygenic, and we'd be doing it only in these few places. So we hold our noses and
do what the surrounding code does.

Hm, OK.

https://codereview.chromium.org/1909353002/diff/80001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/1909353002/diff/80001/src/frames.h#newcode644
src/frames.h:644: Handle<Object> receiver() const { return receiver_; }
On 2016/04/22 at 12:16:25, titzer wrote:
> AFAICT FrameSummary instances are already immutable, so the const modifier is
redundant (since all FrameSummary instances and references are implicitly const,
too).

Removed the const.

https://codereview.chromium.org/1909353002/diff/80001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/80001/src/messages.cc#newcode204
src/messages.cc:204: if (!IsJavaScript()) return
isolate_->factory()->null_value();
On 2016/04/22 at 12:16:25, titzer wrote:
> We probably want to use the WASM function name here.

Oh, definitely. Forgot to change this. Nice catch!

https://codereview.chromium.org/1909353002/diff/80001/src/messages.cc#newcode336
src/messages.cc:336: return IsWasm() || receiver_->IsJSGlobalProxy() ||
receiver_->IsNull() ||
On 2016/04/22 at 12:16:26, titzer wrote:
> I think isTopLevel for WASM should be false.

Probably, yes. It's a bit unclear from the documentation. Made it false for
WASM.

[Clemens Hammacher, 2016-04-27 13:55:03]

PTAL.

[Yang, 2016-04-28 13:08:43]

Wow. This is a pretty intrusive change to the frame iterator. It seems to me
that there are some changes to blink planned as well, since there are API
changes as well.

Is there any document detailing what the plan is? Why do we need to expose the
wasm object and the bytecode offset through the stack trace? Why can't we extend
the function mirror with a wasm mirror?

https://codereview.chromium.org/1909353002/diff/120001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1909353002/diff/120001/include/v8.h#newcode1654
include/v8.h:1654: /**
Why do we need these three methods? What changes are planned on blink's side? I
don't think this is necessary to make the current stack trace mechanism work, is
it?

https://codereview.chromium.org/1909353002/diff/120001/include/v8.h#newcode1670
include/v8.h:1670: * Message::kNoWasmByteOffsetInfo if the associated function
of the frame is
Can we just crash with a check failure if the function is not wasm? The embedder
should be responsible to check IsWasm() upfront.

https://codereview.chromium.org/1909353002/diff/120001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1909353002/diff/120001/src/api.cc#newcode2667
src/api.cc:2667: return getObjectProperty(this, "wasmObject");
This should also not be called if IsWasm() returns false.

[Clemens Hammacher, 2016-04-29 08:13:50]

On 2016/04/28 at 13:08:43, yangguo wrote:
> Wow. This is a pretty intrusive change to the frame iterator. It seems to me
that there are some changes to blink planned as well, since there are API
changes as well.
> 
> Is there any document detailing what the plan is? Why do we need to expose the
wasm object and the bytecode offset through the stack trace?

No, there is no detailed plan yet. The idea is that the embedder should know the
precise position where the exception / trap happened. For Wasm, this means
<module, function, byte offset>. That's why these three methods were introduced.
We might have to add more information to the wasm object later though.

> Why can't we extend the function mirror with a wasm mirror?

I don't get what you mean by that. Can you elaborate on that?


> https://codereview.chromium.org/1909353002/diff/120001/include/v8.h
> File include/v8.h (right):
> 
>
https://codereview.chromium.org/1909353002/diff/120001/include/v8.h#newcode1654
> include/v8.h:1654: /**
> Why do we need these three methods? What changes are planned on blink's side?
I don't think this is necessary to make the current stack trace mechanism work,
is it?

No, it's not strictly necessary yet. If you wish, I can also postpone the
changes to v8.h and api.cc, and change the test to work on the
v8::internal::Object directly.


>
https://codereview.chromium.org/1909353002/diff/120001/include/v8.h#newcode1670
> include/v8.h:1670: * Message::kNoWasmByteOffsetInfo if the associated function
of the frame is
> Can we just crash with a check failure if the function is not wasm? The
embedder should be responsible to check IsWasm() upfront.

The problem here is that the caller could pass kWasmByteOffset, but not kIsWasm
when capturing the stack trace.
Then we cannot check IsWasm().

> 
> https://codereview.chromium.org/1909353002/diff/120001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/1909353002/diff/120001/src/api.cc#newcode2667
> src/api.cc:2667: return getObjectProperty(this, "wasmObject");
> This should also not be called if IsWasm() returns false.

Same here.

[Clemens Hammacher, 2016-05-02 15:01:48]

I removed all changes to public APIs, and make the information available now via
the existing getLineNumber and getPosition methods on CallSite.

PTAL

[Yang, 2016-05-03 18:59:08]

https://codereview.chromium.org/1909353002/diff/180001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:3058: maybe_name =
isolate_->factory()->NewStringFromUtf8(
Can we have brackets around if-body here and below?

https://codereview.chromium.org/1909353002/diff/180001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/frames.cc#newcode1338
src/frames.cc:1338: DCHECK_IMPLIES(!code.is_null(), *code == LookupCode());
I assume this is a performance optimization. Is this really necessary at this
point? LookupCode does have a cache built in.

https://codereview.chromium.org/1909353002/diff/180001/src/frames.cc#newcode1360
src/frames.cc:1360: Object* WasmFrame::function_name(Handle<Code> code) {
Isn't this always going to return a String?

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode544
src/isolate.cc:544: Handle<JSObject> NewStackFrameObject(FrameSummary& summ) {
This seems to have just one caller. Can we inline this at the call site?

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode570
src/isolate.cc:570: // For the case where the code is on the same line as the
script
Can this comment fit on a single line?

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode623
src/isolate.cc:623: auto get_code = [&code, frame, this] {
Let's not use a lambda here. In fact, I consider this a premature optimization.
Let's simply always look up the code. I don't think not including the function,
line or column is a use case we have to optimize for.

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode750
src/isolate.cc:750: } else if (frame->is_wasm()) {
Can the frame be not wasm at this point? How about just having an else here and
add a DCHECK that the frame is wasm?

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode...
src/isolate.cc:1379: // TODO(clemensh): include wasm frames here
add a DCHECK that this frame is wasm.

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode...
src/isolate.cc:1409: if (!script->IsScript() ||
(Script::cast(script)->source()->IsUndefined()))
add brackets around if-body on line break.

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js#new...
src/js/messages.js:573: SET_PRIVATE(this, callSiteWasmFunctionIndexSymbol,
TO_UINT32(fun));
I don't think we need to do the TO_UINT32 conversion. We are guaranteed to get a
Smi.

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js#new...
src/js/messages.js:635: IS_STRING(wasm) ? wasm : %WasmGetFunctionName(wasm,
func_index);
This is the same weirdness where we store a string in place of the wasm object.
Why do we do that?

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:178: } else if (!(maybe_wasm_func_index =
JSObject::GetDataProperty(
Please don't do assignments inside an if-expression. I'd rather have it more
verbose.

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:198: if (!IsJavaScript() || !(script =
fun_->shared()->script())->IsScript())
same here. No assignments inside if-expression. And use brackets around the
if-body in case of line break.

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:207: // wasm_obj_ can be a String if we generate WASM code
directly in a test
That... is really weird and sounds like a hack. Why do we put a string there in
the first place and not undefined?

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:227: if (!IsJavaScript() || !(script_obj =
fun_->shared()->script())->IsScript())
same as above.

https://codereview.chromium.org/1909353002/diff/180001/test/mjsunit/wasm/stac...
File test/mjsunit/wasm/stack.js (right):

https://codereview.chromium.org/1909353002/diff/180001/test/mjsunit/wasm/stac...
test/mjsunit/wasm/stack.js:30: : exp[4]+":"+exp[2]+":";
This is so hard to read... could we avoid nesting tertiary operators and maybe
use string templates?

[Clemens Hammacher, 2016-05-04 09:06:20]

Thanks for the detailed comments. I fixed most of them, and commented on the
rest.

PTAL

https://codereview.chromium.org/1909353002/diff/180001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:3058: maybe_name =
isolate_->factory()->NewStringFromUtf8(
On 2016/05/03 at 18:59:07, Yang wrote:
> Can we have brackets around if-body here and below?

It turned out the first if is even redundant, so I removed it. But I added
brackets around the one below.

https://codereview.chromium.org/1909353002/diff/180001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/frames.cc#newcode1338
src/frames.cc:1338: DCHECK_IMPLIES(!code.is_null(), *code == LookupCode());
On 2016/05/03 at 18:59:07, Yang wrote:
> I assume this is a performance optimization. Is this really necessary at this
point? LookupCode does have a cache built in.

Yes it's a performance optimization, and no, it's not strictly necessary. I
added it since for example function_name() (below) calls both wasm_obj() and
function_index(), and I wanted to avoid having to look up the code several
times.

I removed it, since it's probably not performance critical.

https://codereview.chromium.org/1909353002/diff/180001/src/frames.cc#newcode1360
src/frames.cc:1360: Object* WasmFrame::function_name(Handle<Code> code) {
On 2016/05/03 at 18:59:07, Yang wrote:
> Isn't this always going to return a String?

No, it returns undefined if the the wasm module does not have names attached, or
if the name is no valid UTF8 string.
Would it be nicer to return a MaybeHandle<String> here, and make the caller
convert this to undefined if needed?

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode544
src/isolate.cc:544: Handle<JSObject> NewStackFrameObject(FrameSummary& summ) {
On 2016/05/03 at 18:59:07, Yang wrote:
> This seems to have just one caller. Can we inline this at the call site?

We could, but I am not sure if it would make the code more readable. This is in
an internal helper class anyway, which creates a stack frame object from either
a FrameSummary or a WasmFrame, so these two functions seem to be natural here.
The third one with <fun,position,is_constructor> is the outlier, but it's needed
when constructing a detailed from a simple stack trace.

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode570
src/isolate.cc:570: // For the case where the code is on the same line as the
script
On 2016/05/03 at 18:59:07, Yang wrote:
> Can this comment fit on a single line?

Done.

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode623
src/isolate.cc:623: auto get_code = [&code, frame, this] {
On 2016/05/03 at 18:59:07, Yang wrote:
> Let's not use a lambda here. In fact, I consider this a premature
optimization. Let's simply always look up the code. I don't think not including
the function, line or column is a use case we have to optimize for.

With the change from above, this is now obsolete anyway. The code is just looked
up whenever needed in the individual functions.

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode750
src/isolate.cc:750: } else if (frame->is_wasm()) {
On 2016/05/03 18:59:07, Yang wrote:
> Can the frame be not wasm at this point? How about just having an else here
and
> add a DCHECK that the frame is wasm?

You are right, it will always be wasm. I changed it to just else, and skipped
the DCHECK since this is done in WasmFrame::cast anyway (and for readability
it's not helpful here either).

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode...
src/isolate.cc:1379: // TODO(clemensh): include wasm frames here
On 2016/05/03 18:59:07, Yang wrote:
> add a DCHECK that this frame is wasm.

Done.

https://codereview.chromium.org/1909353002/diff/180001/src/isolate.cc#newcode...
src/isolate.cc:1409: if (!script->IsScript() ||
(Script::cast(script)->source()->IsUndefined()))
On 2016/05/03 18:59:07, Yang wrote:
> add brackets around if-body on line break.

Done.

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js#new...
src/js/messages.js:573: SET_PRIVATE(this, callSiteWasmFunctionIndexSymbol,
TO_UINT32(fun));
On 2016/05/03 18:59:07, Yang wrote:
> I don't think we need to do the TO_UINT32 conversion. We are guaranteed to get
a
> Smi.

I though this is more for expressing the intent. It's also done below, where pos
is guaranteed to be Smi and strict_mode is boolean already.

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js#new...
src/js/messages.js:635: IS_STRING(wasm) ? wasm : %WasmGetFunctionName(wasm,
func_index);
On 2016/05/03 18:59:07, Yang wrote:
> This is the same weirdness where we store a string in place of the wasm
object.
> Why do we do that?

This is done only for testing, where we don't always instantiate a wasm object.
Storing a string instead is explicitely approved by Ben.

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:178: } else if (!(maybe_wasm_func_index =
JSObject::GetDataProperty(
On 2016/05/03 18:59:08, Yang wrote:
> Please don't do assignments inside an if-expression. I'd rather have it more
> verbose.

Done.

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:198: if (!IsJavaScript() || !(script =
fun_->shared()->script())->IsScript())
On 2016/05/03 18:59:08, Yang wrote:
> same here. No assignments inside if-expression. And use brackets around the
> if-body in case of line break.

Done.

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:207: // wasm_obj_ can be a String if we generate WASM code
directly in a test
On 2016/05/03 18:59:08, Yang wrote:
> That... is really weird and sounds like a hack. Why do we put a string there
in
> the first place and not undefined?

See above.

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:227: if (!IsJavaScript() || !(script_obj =
fun_->shared()->script())->IsScript())
On 2016/05/03 18:59:08, Yang wrote:
> same as above.

Done.

https://codereview.chromium.org/1909353002/diff/180001/test/mjsunit/wasm/stac...
File test/mjsunit/wasm/stack.js (right):

https://codereview.chromium.org/1909353002/diff/180001/test/mjsunit/wasm/stac...
test/mjsunit/wasm/stack.js:30: : exp[4]+":"+exp[2]+":";
On 2016/05/03 18:59:08, Yang wrote:
> This is so hard to read... could we avoid nesting tertiary operators and maybe
> use string templates?

Done.

[Yang, 2016-05-04 12:04:27]

some more comments.

https://codereview.chromium.org/1909353002/diff/180001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/frames.cc#newcode1360
src/frames.cc:1360: Object* WasmFrame::function_name(Handle<Code> code) {
On 2016/05/04 09:06:20, Clemens Hammacher wrote:
> On 2016/05/03 at 18:59:07, Yang wrote:
> > Isn't this always going to return a String?
> 
> No, it returns undefined if the the wasm module does not have names attached,
or
> if the name is no valid UTF8 string.
> Would it be nicer to return a MaybeHandle<String> here, and make the caller
> convert this to undefined if needed?

No I think this is fine. MaybeHandle would only work if you were to return a
Handle<String>

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js#new...
src/js/messages.js:573: SET_PRIVATE(this, callSiteWasmFunctionIndexSymbol,
TO_UINT32(fun));
On 2016/05/04 09:06:20, Clemens Hammacher wrote:
> On 2016/05/03 18:59:07, Yang wrote:
> > I don't think we need to do the TO_UINT32 conversion. We are guaranteed to
get
> a
> > Smi.
> 
> I though this is more for expressing the intent. It's also done below, where
pos
> is guaranteed to be Smi and strict_mode is boolean already.

I see. Alright. Can you add a comment so that people unfamiliar with this don't
get surprised by 'fun' being an index instead of an actual function?

https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js#new...
src/js/messages.js:635: IS_STRING(wasm) ? wasm : %WasmGetFunctionName(wasm,
func_index);
On 2016/05/04 09:06:20, Clemens Hammacher wrote:
> On 2016/05/03 18:59:07, Yang wrote:
> > This is the same weirdness where we store a string in place of the wasm
> object.
> > Why do we do that?
> 
> This is done only for testing, where we don't always instantiate a wasm
object.
> Storing a string instead is explicitely approved by Ben.

I'm just not really happy with the fact that we are leaking this detail
introduced only for testing into other parts of V8. Can't we create a dummy wasm
object that has that string as name for testing? Or at least contain the logic
inside %WasmGetFunctionName to deal with the wasm object being a string (with
comments to explain).

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/180001/src/messages.cc#newcod...
src/messages.cc:207: // wasm_obj_ can be a String if we generate WASM code
directly in a test
On 2016/05/04 09:06:20, Clemens Hammacher wrote:
> On 2016/05/03 18:59:08, Yang wrote:
> > That... is really weird and sounds like a hack. Why do we put a string there
> in
> > the first place and not undefined?
> 
> See above.

So, like above, can we hide this detail (introduced only for testing) inside
wasm::GetWasmFunctionName?

https://codereview.chromium.org/1909353002/diff/200001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/200001/src/messages.cc#newcod...
src/messages.cc:187: // invalid: neither javascript nor wasm
Can/should this actually happen? Rather than failing silently, we should have an
assertion failure. Let's DCHECK that at this point maybe_wasm_func_index is not
just a maybe, but definitely a Smi.

https://codereview.chromium.org/1909353002/diff/200001/src/messages.cc#newcod...
src/messages.cc:229: if (!IsJavaScript() || !(script_obj =
fun_->shared()->script())->IsScript())
We still got an assignment inside an if-expression here. And also brackets.

[Clemens Hammacher, 2016-05-04 12:44:51]

All done.

On 2016/05/04 12:04:27, Yang wrote:
>
https://codereview.chromium.org/1909353002/diff/180001/src/js/messages.js#new...
> src/js/messages.js:635: IS_STRING(wasm) ? wasm : %WasmGetFunctionName(wasm,
> func_index);
> On 2016/05/04 09:06:20, Clemens Hammacher wrote:
> > On 2016/05/03 18:59:07, Yang wrote:
> > > This is the same weirdness where we store a string in place of the wasm
> > object.
> > > Why do we do that?
> > 
> > This is done only for testing, where we don't always instantiate a wasm
> object.
> > Storing a string instead is explicitely approved by Ben.
> 
> I'm just not really happy with the fact that we are leaking this detail
> introduced only for testing into other parts of V8. Can't we create a dummy
wasm
> object that has that string as name for testing? Or at least contain the logic
> inside %WasmGetFunctionName to deal with the wasm object being a string (with
> comments to explain).

I now removed storing the string there completely, and changed the cctests
accordingly. This saves a few special cases here and there.
We have enough mjsunit tests which check for the right function names.

https://codereview.chromium.org/1909353002/diff/200001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/200001/src/messages.cc#newcod...
src/messages.cc:187: // invalid: neither javascript nor wasm
On 2016/05/04 12:04:27, Yang wrote:
> Can/should this actually happen? Rather than failing silently, we should have
an
> assertion failure. Let's DCHECK that at this point maybe_wasm_func_index is
not
> just a maybe, but definitely a Smi.

I am not sure what the intent of this was. There is a RUNTIME_ASSERT in
runtime-internal.cc for call_site.IsValid(), which would fail if an error is
detected in this constructor. But since the call_site_obj should be trustworthy
here, I don't see how it could fail.
I changed this to just do the casts here, which DCHECK internally.

https://codereview.chromium.org/1909353002/diff/200001/src/messages.cc#newcod...
src/messages.cc:229: if (!IsJavaScript() || !(script_obj =
fun_->shared()->script())->IsScript())
On 2016/05/04 12:04:27, Yang wrote:
> We still got an assignment inside an if-expression here. And also brackets.

Done.

[Yang, 2016-05-04 19:02:22]

LGTM once comments are addressed.

https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc#newcode1357
src/frames.cc:1357: Object* wasm_obj_or_string = wasm_obj();
is the name of this variable still accurate?

https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc#newcode1358
src/frames.cc:1358: if (wasm_obj_or_string->IsUndefined()) return
wasm_obj_or_string;
I'm curious, in which case would wasm_obj() return undefined?

https://codereview.chromium.org/1909353002/diff/220001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/220001/src/messages.cc#newcod...
src/messages.cc:178: } else {
I guess I was wrong in my previous comments. Now that I remember, I think we
should keep the defensive programming style. In case call_site_obj is not
actually a call site object, we could end up with undefined here. Let's check
that maybe_function is a Smi in the else case. If not, return. That way, we
could still use IsValid to check whether call_site_obj has the expected
properties.

https://codereview.chromium.org/1909353002/diff/220001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (left):

https://codereview.chromium.org/1909353002/diff/220001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:344: RUNTIME_ASSERT(call_site.IsValid());       
            \
Can we at least have a runtime assert that checks that call_site is either
javascript or wasm? This has been put here to check that the CallSite object has
been successfully constructed. I.e. the argument call_site_obj indeed has the
correct properties.

[Clemens Hammacher, 2016-05-05 12:29:16]

https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc#newcode1357
src/frames.cc:1357: Object* wasm_obj_or_string = wasm_obj();
On 2016/05/04 19:02:21, Yang wrote:
> is the name of this variable still accurate?

No, sorry. Done.

https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc#newcode1358
src/frames.cc:1358: if (wasm_obj_or_string->IsUndefined()) return
wasm_obj_or_string;
On 2016/05/04 19:02:22, Yang wrote:
> I'm curious, in which case would wasm_obj() return undefined?

Just in the tests. Where it was string or undefined before, depending on whether
a name was set.

https://codereview.chromium.org/1909353002/diff/220001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1909353002/diff/220001/src/messages.cc#newcod...
src/messages.cc:178: } else {
On 2016/05/04 19:02:22, Yang wrote:
> I guess I was wrong in my previous comments. Now that I remember, I think we
> should keep the defensive programming style. In case call_site_obj is not
> actually a call site object, we could end up with undefined here. Let's check
> that maybe_function is a Smi in the else case. If not, return. That way, we
> could still use IsValid to check whether call_site_obj has the expected
> properties.

OK, I changed it accordingly.
I guess I will still have to understand fully how much we can trust the input
values here.

https://codereview.chromium.org/1909353002/diff/220001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (left):

https://codereview.chromium.org/1909353002/diff/220001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:344: RUNTIME_ASSERT(call_site.IsValid());       
            \
On 2016/05/04 19:02:22, Yang wrote:
> Can we at least have a runtime assert that checks that call_site is either
> javascript or wasm? This has been put here to check that the CallSite object
has
> been successfully constructed. I.e. the argument call_site_obj indeed has the
> correct properties.

Done.

[titzer, 2016-05-06 08:38:28]

On 2016/05/05 12:29:16, Clemens Hammacher wrote:
> https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc
> File src/frames.cc (right):
> 
>
https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc#newcode1357
> src/frames.cc:1357: Object* wasm_obj_or_string = wasm_obj();
> On 2016/05/04 19:02:21, Yang wrote:
> > is the name of this variable still accurate?
> 
> No, sorry. Done.
> 
>
https://codereview.chromium.org/1909353002/diff/220001/src/frames.cc#newcode1358
> src/frames.cc:1358: if (wasm_obj_or_string->IsUndefined()) return
> wasm_obj_or_string;
> On 2016/05/04 19:02:22, Yang wrote:
> > I'm curious, in which case would wasm_obj() return undefined?
> 
> Just in the tests. Where it was string or undefined before, depending on
whether
> a name was set.
> 
> https://codereview.chromium.org/1909353002/diff/220001/src/messages.cc
> File src/messages.cc (right):
> 
>
https://codereview.chromium.org/1909353002/diff/220001/src/messages.cc#newcod...
> src/messages.cc:178: } else {
> On 2016/05/04 19:02:22, Yang wrote:
> > I guess I was wrong in my previous comments. Now that I remember, I think we
> > should keep the defensive programming style. In case call_site_obj is not
> > actually a call site object, we could end up with undefined here. Let's
check
> > that maybe_function is a Smi in the else case. If not, return. That way, we
> > could still use IsValid to check whether call_site_obj has the expected
> > properties.
> 
> OK, I changed it accordingly.
> I guess I will still have to understand fully how much we can trust the input
> values here.
> 
>
https://codereview.chromium.org/1909353002/diff/220001/src/runtime/runtime-in...
> File src/runtime/runtime-internal.cc (left):
> 
>
https://codereview.chromium.org/1909353002/diff/220001/src/runtime/runtime-in...
> src/runtime/runtime-internal.cc:344: RUNTIME_ASSERT(call_site.IsValid());     
 
>             \
> On 2016/05/04 19:02:22, Yang wrote:
> > Can we at least have a runtime assert that checks that call_site is either
> > javascript or wasm? This has been put here to check that the CallSite object
> has
> > been successfully constructed. I.e. the argument call_site_obj indeed has
the
> > correct properties.
> 
> Done.

lgtm

[Clemens Hammacher, 2016-05-06 08:39:12]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-05-06 08:39:12]

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/1909353002/#ps240001
(title: "last changes")

[commit-bot: I haz the power, 2016-05-06 08:39:16]

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

[commit-bot: I haz the power, 2016-05-06 09:07:36]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-06 09:07:55]

Description was changed from

==========
[wasm] Make wasm info available on the stack trace

This changes different locations to extract the reference to the wasm
object and the function index from the stack trace, and make it
available through all the APIs which process stack traces.
The javascript CallSite object now has the new methods isWasm(),
getWasmObject() and getWasmFunctionIndex(); the byte offset is
available via getPosition().

Function names of wasm frames should be fully functional with this
commit, position information works reliably for calls, but not for
traps like unreachable or out-of-bounds accesses.

R=titzer@chromium.org, yangguo@chromium.org
==========

to

==========
[wasm] Make wasm info available on the stack trace

This changes different locations to extract the reference to the wasm
object and the function index from the stack trace, and make it
available through all the APIs which process stack traces.
The javascript CallSite object now has the new methods isWasm(),
getWasmObject() and getWasmFunctionIndex(); the byte offset is
available via getPosition().

Function names of wasm frames should be fully functional with this
commit, position information works reliably for calls, but not for
traps like unreachable or out-of-bounds accesses.

R=titzer@chromium.org, yangguo@chromium.org

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

[commit-bot: I haz the power, 2016-05-06 09:07:56]

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