[wasm] Differentiate unnamed and empty names

[wasm] Differentiate unnamed and empty names

Empty function names are allowed and are output as such, unnamed
functions or functions with no valid UTF-8 name are output as
"<WASM UNNAMED>", while the CallSite object returns null as the
function name.

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

Committed: https://crrev.com/3596cac87ecb107b723aecb9f457c1c9fb5c1df0
Cr-Commit-Position: refs/heads/master@{#36348}

[titzer, 2016-05-12 09:05:51]

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js#newcode682
src/js/messages.js:682: funName = "<WASM UNNAMED>";
Should probably also add the position here.

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js#newcode683
src/js/messages.js:683: return funName + " (<WASM>:" + funcIndex + ":" + pos +
")";
bikeshed: for this line: 
" (<WASM>[" + funcIndex +"]+" + pos + ")"

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
File src/wasm/wasm-function-name-table.cc (right):

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
src/wasm/wasm-function-name-table.cc:19: // integer entry is the negative offset
of the next function name.
Why does it have to be the negative offset of the current function? Could it
just be -1?

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
src/wasm/wasm-function-name-table.cc:60: // name is no valid UTF-8 string;
returns null for unnamed functions (not to be
s/no valid/not a valid/

Why not just return undefined for unnamed functions too?

[Clemens Hammacher, 2016-05-12 09:45:57]

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js#newcode682
src/js/messages.js:682: funName = "<WASM UNNAMED>";
On 2016/05/12 09:05:51, titzer wrote:
> Should probably also add the position here.

It's still added in the line below, there is no return here.

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js#newcode683
src/js/messages.js:683: return funName + " (<WASM>:" + funcIndex + ":" + pos +
")";
On 2016/05/12 09:05:51, titzer wrote:
> bikeshed: for this line: 
> " (<WASM>[" + funcIndex +"]+" + pos + ")"

Would you prefer this format for all wasm frames? For the simple stack traces,
we currently also output "<WASM>:funcIndex:pos".

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
File src/wasm/wasm-function-name-table.cc (right):

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
src/wasm/wasm-function-name-table.cc:19: // integer entry is the negative offset
of the next function name.
On 2016/05/12 09:05:51, titzer wrote:
> Why does it have to be the negative offset of the current function? Could it
> just be -1?

The negative offset of the next function name allows to easily compute the
length of the current function name: abs(next_offset) - offset.

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
src/wasm/wasm-function-name-table.cc:60: // name is no valid UTF-8 string;
returns null for unnamed functions (not to be
On 2016/05/12 09:05:51, titzer wrote:
> s/no valid/not a valid/
> 
> Why not just return undefined for unnamed functions too?

I am not the JavaScript expert, but I think undefined really means "cannot
tell", where null means "no value". I am not sure if anyone will ever rely on
this distinction, but I thought it would not hurt to provide it.

[titzer, 2016-05-12 09:54:36]

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js#newcode682
src/js/messages.js:682: funName = "<WASM UNNAMED>";
On 2016/05/12 09:45:57, Clemens Hammacher wrote:
> On 2016/05/12 09:05:51, titzer wrote:
> > Should probably also add the position here.
> 
> It's still added in the line below, there is no return here.

Acknowledged.

https://codereview.chromium.org/1970503004/diff/1/src/js/messages.js#newcode683
src/js/messages.js:683: return funName + " (<WASM>:" + funcIndex + ":" + pos +
")";
On 2016/05/12 09:45:57, Clemens Hammacher wrote:
> On 2016/05/12 09:05:51, titzer wrote:
> > bikeshed: for this line: 
> > " (<WASM>[" + funcIndex +"]+" + pos + ")"
> 
> Would you prefer this format for all wasm frames? For the simple stack traces,
> we currently also output "<WASM>:funcIndex:pos".

yes

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
File src/wasm/wasm-function-name-table.cc (right):

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
src/wasm/wasm-function-name-table.cc:19: // integer entry is the negative offset
of the next function name.
On 2016/05/12 09:45:57, Clemens Hammacher wrote:
> On 2016/05/12 09:05:51, titzer wrote:
> > Why does it have to be the negative offset of the current function? Could it
> > just be -1?
> 
> The negative offset of the next function name allows to easily compute the
> length of the current function name: abs(next_offset) - offset.

ok

https://codereview.chromium.org/1970503004/diff/1/src/wasm/wasm-function-name...
src/wasm/wasm-function-name-table.cc:60: // name is no valid UTF-8 string;
returns null for unnamed functions (not to be
On 2016/05/12 09:45:57, Clemens Hammacher wrote:
> On 2016/05/12 09:05:51, titzer wrote:
> > s/no valid/not a valid/
> > 
> > Why not just return undefined for unnamed functions too?
> 
> I am not the JavaScript expert, but I think undefined really means "cannot
> tell", where null means "no value". I am not sure if anyone will ever rely on
> this distinction, but I thought it would not hurt to provide it.

I think it's OK to use undefined for both here: the caller just wants the name,
and if the name is invalid, we can treat that case as if the name was simply
omitted.

[Clemens Hammacher, 2016-05-12 13:26:11]

all done.

[titzer, 2016-05-12 13:35:42]

On 2016/05/12 13:26:11, Clemens Hammacher wrote:
> all done.

lgtm

[Yang, 2016-05-13 07:41:10]

https://codereview.chromium.org/1970503004/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1970503004/diff/100001/src/isolate.cc#newcode624
src/isolate.cc:624: fun_name = isolate_->factory()->InternalizeUtf8String(
We should not be using InternalizeUtf8String here. Use NewStringFromStaticChars
instead.

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js#new...
src/js/messages.js:635: var func_index = GET_PRIVATE(this,
callSiteWasmFunctionIndexSymbol);
CallSiteGetFunctionName is accessible from the stack trace API. This here
changes the behavior, and we could be returning undefined for the function name.
Do we really want to expose that?

What would be nicer imo is to hide that, and return <WASM UNNAMED> directly. In
fact, we should do that in GetWasmFunctionName or GetWasmFunctionNameFromTable.

In another CL you commented that
"We cannot completely hide the logic inside %WasmGetFunctionName, since the
string representation differs in some cases from the information returned by
CallSite.getFunctionName()."

I still haven't understood when that is the case, and why we can't simply have
CallSiteGetFunctionName return what %WasmGetFunctionName returns.

I also looked at CallSite::GetFunctionName(). We have a wasm code path there as
well, whcih again calls GetWasmFunctionName. How come we have two different, yet
similar code paths? CallSite::GetFunctionName() converts undefined to null,
which we don't do here. Why the difference?

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js#new...
src/js/messages.js:682: return funName + " (<WASM>[" + funcIndex + "]+" + pos +
")";
So we could get a return value like "<WASM UNNAMED> (<WASM>[0]+44)". Is that
intended?

[Clemens Hammacher, 2016-05-17 17:03:53]

Description was changed from

==========
[wasm] Differentiate unnamed and empty names

Empty function names are allowed and are output as such, unnamed
functions or functions with no valid UTF-8 name are output as
"<WASM UNNAMED>", while the CallSite object return null or undefined
respectively.

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

to

==========
[wasm] Differentiate unnamed and empty names

Empty function names are allowed and are output as such, unnamed
functions or functions with no valid UTF-8 name are output as
"<WASM UNNAMED>", while the CallSite object returns null as the
function name.

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

[Clemens Hammacher, 2016-05-17 17:43:24]

https://codereview.chromium.org/1970503004/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1970503004/diff/100001/src/isolate.cc#newcode624
src/isolate.cc:624: fun_name = isolate_->factory()->InternalizeUtf8String(
On 2016/05/13 07:41:10, Yang wrote:
> We should not be using InternalizeUtf8String here. Use
NewStringFromStaticChars
> instead.

Done.

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js#new...
src/js/messages.js:635: var func_index = GET_PRIVATE(this,
callSiteWasmFunctionIndexSymbol);
On 2016/05/13 07:41:10, Yang wrote:
> CallSiteGetFunctionName is accessible from the stack trace API. This here
> changes the behavior, and we could be returning undefined for the function
name.
> Do we really want to expose that?
> 
> What would be nicer imo is to hide that, and return <WASM UNNAMED> directly.
In
> fact, we should do that in GetWasmFunctionName or
GetWasmFunctionNameFromTable.

I was assuming that it would be nice for users to be able to tell that a
function was unnamed. The string "<WASM UNNAMED>" would only be used for
printing such unnamed functions, e.g. on the stack trace.

As discussed offline, I now changed it to return null on
CallSite::getFunctionName (just as anonymous JavaScript function), and print it
as "<WASM UNNAMED>" on CallSite::toString().


> In another CL you commented that
> "We cannot completely hide the logic inside %WasmGetFunctionName, since the
> string representation differs in some cases from the information returned by
> CallSite.getFunctionName()."
> 
> I still haven't understood when that is the case, and why we can't simply have
> CallSiteGetFunctionName return what %WasmGetFunctionName returns.

OK, this is not about %WasmGetFunctionName vs. CallSite.getFunctionName(), but
about those two vs. CallSite.toString(). There needs to be some (tiny) logic in
messages.js to translate null (previously undefined) to "<WASM UNNAMED>".


> I also looked at CallSite::GetFunctionName(). We have a wasm code path there
as
> well, whcih again calls GetWasmFunctionName. How come we have two different,
yet
> similar code paths? CallSite::GetFunctionName() converts undefined to null,
> which we don't do here. Why the difference?

Oh, you are right. If I see it correctly, CallSite::GetFunctionName was never
called for wasm functions, so that code path was dead. I now removed the special
handling from messages.js, and always call CallSite::GetFunctionName, which does
the right thing and returns either null or the function name as string.

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js#new...
src/js/messages.js:682: return funName + " (<WASM>[" + funcIndex + "]+" + pos +
")";
On 2016/05/13 07:41:10, Yang wrote:
> So we could get a return value like "<WASM UNNAMED> (<WASM>[0]+44)". Is that
> intended?

Yes.

[Clemens Hammacher, 2016-05-17 17:43:25]

https://codereview.chromium.org/1970503004/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1970503004/diff/100001/src/isolate.cc#newcode624
src/isolate.cc:624: fun_name = isolate_->factory()->InternalizeUtf8String(
On 2016/05/13 07:41:10, Yang wrote:
> We should not be using InternalizeUtf8String here. Use
NewStringFromStaticChars
> instead.

Done.

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js#new...
src/js/messages.js:635: var func_index = GET_PRIVATE(this,
callSiteWasmFunctionIndexSymbol);
On 2016/05/13 07:41:10, Yang wrote:
> CallSiteGetFunctionName is accessible from the stack trace API. This here
> changes the behavior, and we could be returning undefined for the function
name.
> Do we really want to expose that?
> 
> What would be nicer imo is to hide that, and return <WASM UNNAMED> directly.
In
> fact, we should do that in GetWasmFunctionName or
GetWasmFunctionNameFromTable.

I was assuming that it would be nice for users to be able to tell that a
function was unnamed. The string "<WASM UNNAMED>" would only be used for
printing such unnamed functions, e.g. on the stack trace.

As discussed offline, I now changed it to return null on
CallSite::getFunctionName (just as anonymous JavaScript function), and print it
as "<WASM UNNAMED>" on CallSite::toString().


> In another CL you commented that
> "We cannot completely hide the logic inside %WasmGetFunctionName, since the
> string representation differs in some cases from the information returned by
> CallSite.getFunctionName()."
> 
> I still haven't understood when that is the case, and why we can't simply have
> CallSiteGetFunctionName return what %WasmGetFunctionName returns.

OK, this is not about %WasmGetFunctionName vs. CallSite.getFunctionName(), but
about those two vs. CallSite.toString(). There needs to be some (tiny) logic in
messages.js to translate null (previously undefined) to "<WASM UNNAMED>".


> I also looked at CallSite::GetFunctionName(). We have a wasm code path there
as
> well, whcih again calls GetWasmFunctionName. How come we have two different,
yet
> similar code paths? CallSite::GetFunctionName() converts undefined to null,
> which we don't do here. Why the difference?

Oh, you are right. If I see it correctly, CallSite::GetFunctionName was never
called for wasm functions, so that code path was dead. I now removed the special
handling from messages.js, and always call CallSite::GetFunctionName, which does
the right thing and returns either null or the function name as string.

https://codereview.chromium.org/1970503004/diff/100001/src/js/messages.js#new...
src/js/messages.js:682: return funName + " (<WASM>[" + funcIndex + "]+" + pos +
")";
On 2016/05/13 07:41:10, Yang wrote:
> So we could get a return value like "<WASM UNNAMED> (<WASM>[0]+44)". Is that
> intended?

Yes.

[Yang, 2016-05-18 13:38:48]

LGTM with some comments.

https://codereview.chromium.org/1970503004/diff/120001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1970503004/diff/120001/src/frames.cc#newcode1363
src/frames.cc:1363: return name.is_null() ? nullptr : *name.ToHandleChecked();
This whole method seems to have only one user. Why don't we simply compute the
name at the callsite from wasm_object() and function_index(). JavaScriptFrame
doesn't offer this functionality either.

https://codereview.chromium.org/1970503004/diff/120001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/1970503004/diff/120001/src/isolate.cc#newcode619
src/isolate.cc:619: String* fun_name_raw = frame->function_name();
Like noted previously, this having WasmFrame::function_name() just seems to add
a lot of code, and we are dealing with the case where we do have no name there
and here.

https://codereview.chromium.org/1970503004/diff/120001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1970503004/diff/120001/src/messages.cc#newcod...
src/messages.cc:209: if (!wasm_obj_->IsUndefined())
brackets around the if-body.

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

https://codereview.chromium.org/1970503004/diff/120001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:983: if (func_names_arr_obj->IsUndefined()) return {};
Can we return Handle<String>::null() to be explicit?

[Clemens Hammacher, 2016-05-18 17:44:07]

https://codereview.chromium.org/1970503004/diff/120001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1970503004/diff/120001/src/frames.cc#newcode1363
src/frames.cc:1363: return name.is_null() ? nullptr : *name.ToHandleChecked();
On 2016/05/18 13:38:48, Yang wrote:
> This whole method seems to have only one user. Why don't we simply compute the
> name at the callsite from wasm_object() and function_index(). JavaScriptFrame
> doesn't offer this functionality either.

Makes sense. Done.

https://codereview.chromium.org/1970503004/diff/120001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/1970503004/diff/120001/src/messages.cc#newcod...
src/messages.cc:209: if (!wasm_obj_->IsUndefined())
On 2016/05/18 13:38:48, Yang wrote:
> brackets around the if-body.

Done.

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

https://codereview.chromium.org/1970503004/diff/120001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:983: if (func_names_arr_obj->IsUndefined()) return {};
On 2016/05/18 13:38:48, Yang wrote:
> Can we return Handle<String>::null() to be explicit?

Done.

[Clemens Hammacher, 2016-05-19 07:50:49]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-05-19 07:50:50]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1970503004/#ps160001
(title: "Yang's last comments")

[commit-bot: I haz the power, 2016-05-19 07:50:58]

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

[commit-bot: I haz the power, 2016-05-19 07:52:41]

Description was changed from

==========
[wasm] Differentiate unnamed and empty names

Empty function names are allowed and are output as such, unnamed
functions or functions with no valid UTF-8 name are output as
"<WASM UNNAMED>", while the CallSite object returns null as the
function name.

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

to

==========
[wasm] Differentiate unnamed and empty names

Empty function names are allowed and are output as such, unnamed
functions or functions with no valid UTF-8 name are output as
"<WASM UNNAMED>", while the CallSite object returns null as the
function name.

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

[commit-bot: I haz the power, 2016-05-19 07:52:43]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-19 07:54:19]

Description was changed from

==========
[wasm] Differentiate unnamed and empty names

Empty function names are allowed and are output as such, unnamed
functions or functions with no valid UTF-8 name are output as
"<WASM UNNAMED>", while the CallSite object returns null as the
function name.

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

to

==========
[wasm] Differentiate unnamed and empty names

Empty function names are allowed and are output as such, unnamed
functions or functions with no valid UTF-8 name are output as
"<WASM UNNAMED>", while the CallSite object returns null as the
function name.

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

Committed: https://crrev.com/3596cac87ecb107b723aecb9f457c1c9fb5c1df0
Cr-Commit-Position: refs/heads/master@{#36348}
==========

[commit-bot: I haz the power, 2016-05-19 07:54:20]

Patchset 9 (id:??) landed as
https://crrev.com/3596cac87ecb107b723aecb9f457c1c9fb5c1df0
Cr-Commit-Position: refs/heads/master@{#36348}