Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export JSFunctions should have string names #883

Closed
titzer opened this issue Dec 1, 2016 · 7 comments · Fixed by #887
Closed

Export JSFunctions should have string names #883

titzer opened this issue Dec 1, 2016 · 7 comments · Fixed by #887

Comments

@titzer
Copy link

titzer commented Dec 1, 2016

I just noticed that in JS.md, we specify that the exported Function exotic objects, we specify the name of a function to be the index as a number. That seems to be at odds with ECMAScript, which seems to require that all function names be either a string or a symbol.

Should we change JS.md to specific the function index is stored as a string?

@rossberg
Copy link
Member

rossberg commented Dec 1, 2016

In particular, the SetFunctionName meta function explicitly makes this assumption. A ToString conversion seems in order.

@lukewagner
Copy link
Member

Yeah, I don't know what I was thinking when I wrote that because we assign a string too.

Actually, looking at the code, we don't assign the stringified index, we write the same function name we use in callstacks which will use the name provided by the names section. I guess that's a bad idea, since it means stripping the names section could break code (which is technically true b/c Error.stack, but more in the fuzzy space).

@titzer
Copy link
Author

titzer commented Dec 1, 2016

We were doing something slightly different: using the name of the first export of that function. I think Stringified function index makes the most sense here.

@lukewagner
Copy link
Member

Oh, that's a neat idea, and stable under name-stripping. Technically there is the corner case of exported functions derived from Table.get(i) which have no exported name; which perhaps is what brought up this question in the first place.

I guess a question is whether .name is principally used programmatically or just for debugging/logging. If the latter, we could do something friendlier string like wasm function with index ${i} (and name ${s}) (where the parenthetical was optional). But if the former, just stringified index will probably be best.

@rossberg
Copy link
Member

rossberg commented Dec 1, 2016

Some implementations include the name when invoking toString on the function, though nothing around that is properly standardised, and it's already violated by some other cases. There are some concrete TC39 proposals on the way that do that, too, though IIRC.

@lukewagner
Copy link
Member

Oh hah. Just because of how the usual hooks work out, we currently print out "function wasm-function[0]() {\n [native code]\n}" where 0 is the function index.

So noone's printing the binary->text in toSource here, are they? Because that would require saving (at least parts of) the bytecode even when the Module is dead and also put the binary->text algo on the normal security surface.

So if we're not printing wasm text format in toString, then it doesn't have to conform to any language and it's just an aesthetic choice so something like wasm function with index ${i} (and name ${s}) could make sense, keeping .name to the minimal ToString(index). How does that sound?

@lukewagner
Copy link
Member

Or even wasm function with index ${i} offset ${o} (and name ${n}) and then, if this same string is included into whatever overall Error.stack string format the engine already has, this could subsume #814.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants