Add a module name to the name section #1055
Conversation
|
Do we expect people to have name sections with just a module name and nothing else (no functions or locals)? |
|
@jfbastien Seems unlikely to me, but allowing it only costs a byte and is backward compatible 🙂 |
|
@RyanLamansky not quite: I've heard that folks may want to take the presence of a I don't want to judge which usecase / heuristic is valid for now, I just want to understand if we expect their interaction to matter at all. |
|
I had thought that I'd probably just have emscripten always include a module name even in release mode, since that would cost very little and would ensure that the VM would always have at least a potentially-meaningful thing to print e.g. in stack traces (otherwise for the ArrayBuffer APIs there might not be anything other than the instantiation site). But that's a good point about debugging intent. I guess you could make the heuristic more complicated by checking for the existence of function or local names, but that's starting to get slightly ugly; and then when we add source maps and other external debug info which maybe used on demand, it makes even less sense. |
|
Yeah I like the idea of having the module name, I'm just not sure it should be in the name section... Which is sad in itself! Maybe @lukewagner has some wisdom on this topic? |
|
Nice connection @jfbastien! Technically, the predicate we use for "should we preserve the bytecode in instances?" is So overall, I think this is a good idea. |
BinaryEncoding.md
Outdated
| @@ -451,6 +451,8 @@ be skipped over by an engine. The current list of valid `name_type` codes are: | |||
| | --------- | ---- | ----------- | | |||
| | [Function](#function-names) | `1` | Assigns names to functions | | |||
| | [Local](#local-names) | `2` | Assigns names to locals in functions | | |||
| | [Module](#module-name) | `3` | Assigns a name to the module | | |||
There was a problem hiding this comment.
Could we make the code be 0 so that it looks like we meant to do this all along ;) ?
|
Made the name type code be 0 (of course we always intended it to be this way...) @jfbastien wrt debugging intention and weirdness, I'm ambivalent: on one hand having things be explicit is probably better than implicit (e.g. a. complex opaque heuristic to decide when the browser things you're going to debug). But OTOH it's not that bad I hope that before long we'll have more debug info than just the names section, at which point we won't need to rely on it, and the platform/spec won't have weird warts that were important at some time in the past. |
https://bugs.webkit.org/show_bug.cgi?id=172008 Reviewed by Keith Miller. Parse the WebAssembly module name properly, and skip unknown sections. This is useful because as toolchains support new types of names we want to keep displaying the information we know about and simply ignore new information. That capability was designed into WebAssembly's name section. Failure to commit this patch would mean that WebKit won't display stack trace information, which would make developers sad. Module names were added here: WebAssembly/design#1055 Note that this patch doesn't do anything with the parsed name! Two reasons for this: module names aren't supported in binaryen yet, so I can't write a simple binary test; and using the name is a slightly riskier change because it requires changing StackVisitor + StackFrame (where they print "[wasm code]") which requires figuring out the frame's Module. The latter bit isn't trivial because we only know wasm frames from their tag bits, and CodeBlocks are always nullptr. Binaryen bug: WebAssembly/binaryen#1010 I filed #174098 to use the module name. * wasm/WasmFormat.h: (JSC::Wasm::isValidNameType): * wasm/WasmNameSectionParser.cpp: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@219134 268f45cc-cd09-0410-ab3c-d52691b4dbfc
This will make distinguishing modules (e.g. in a stack trace containing dynamically-linked modules) much easier; especially when using the ArrayBuffer API.