Bug 171263

Created attachment 308510 [details] Initial implementation Here's a first shot at the name section. I haven't hooked it up to backtraces yet but that's easy. The problem is that binaryen only just switched to emit this 15 days ago [0], and the binary I wanted to test predates the switch. I'll get the latest binaryen and test locally instead. But not tonight :) [0]: https://github.com/WebAssembly/binaryen/commit/57a2bb96c7b8a98433446828aca845a9e9be8c3d

Created attachment 309349 [details] patch

Attachment 309349 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:247: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:248: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 309349 [details] patch Attachment 309349 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3697493 New failing tests: ChakraCore.yaml/ChakraCore/test/Strings/HTMLHelpers.js.default

Created attachment 309478 [details] patch Wow I hit build issues all over!

Attachment 309478 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:247: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:248: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 309478 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=309478&action=review Some comments before r+. > JSTests/ChangeLog:10 > + * wasm/function-tests/nameSection.wasm: Added. Why is there a raw wasm file? > Source/JavaScriptCore/CMakeLists.txt:944 > + remove extra new line. > Source/JavaScriptCore/CMakeLists.txt:-968 > - I would leave this new line in; that's what we do elsewhere. > Source/JavaScriptCore/wasm/WasmCallee.h:63 > + IndexOrName m_indexOrName; One downside of using IndexOrName here is that you have no way to get the index of the Callee back if that's what you want. Perhaps we don't need that but it also seems like if we have the index we can recover the name from the module but not the other way around. > Source/JavaScriptCore/wasm/WasmFormat.h:244 > +static bool isValidNameType(Int val) shouldn't this be inline not static? I guess static might be ok since this is a template function but it still seems weird. > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:607 > + Name nameName = { 'n', 'a', 'm', 'e' }; name name name name! > Source/JavaScriptCore/wasm/WasmName.h:35 > +typedef Vector<LChar> Name; nit: using Name = Vector<LChar>; > Source/JavaScriptCore/wasm/WasmNameSectionParser.cpp:61 > + WASM_PARSER_FAIL_IF(!parseVarUInt32(nameLen), "can't get ", function, "th function's name length for payload ", payloadNumber); > + WASM_PARSER_FAIL_IF(!consumeUTF8String(nameString, nameLen), "can't get ", function, "th function's name of length ", nameLen, " for payload ", payloadNumber); Nit: why are these ths? I would phase this as "unable to read function ", function, "'s name, which has length of ", nameLen, " and payload", payloadNumber > Source/JavaScriptCore/wasm/WasmNameSectionParser.cpp:78 > + WASM_PARSER_FAIL_IF(!parseVarUInt32(nameLen), "can't get ", local, "th local's name length for payload ", payloadNumber); > + WASM_PARSER_FAIL_IF(!consumeUTF8String(nameString, nameLen), "can't get ", local, "th local's name of length ", nameLen, " for payload ", payloadNumber); ditto.

Created attachment 309615 [details] patch > > JSTests/ChangeLog:10 > > + * wasm/function-tests/nameSection.wasm: Added. > > Why is there a raw wasm file? Two main reasons: 1. I wanted to test exactly what Emscripten generates for name section (it adds some more stuff than the names in the C++ source file). 2. I don't think this is particularly useful to add to the builder. In general it's suboptimal to check in binary files, but here I think it's the right thing to do. The repro instructions are in the .js file (original C++ file and build command line). > > Source/JavaScriptCore/wasm/WasmCallee.h:63 > > + IndexOrName m_indexOrName; > > One downside of using IndexOrName here is that you have no way to get the > index of the Callee back if that's what you want. Perhaps we don't need that > but it also seems like if we have the index we can recover the name from the > module but not the other way around. Yeah that seems fine for now, but we can revisit later if we want both. > > Source/JavaScriptCore/wasm/WasmFormat.h:244 > > +static bool isValidNameType(Int val) > > shouldn't this be inline not static? I guess static might be ok since this > is a template function but it still seems weird. True, I updated here and the similar thing in this file. > > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:607 > > + Name nameName = { 'n', 'a', 'm', 'e' }; > > name name name name! I know right! I chuckled when I wrote this. > > Source/JavaScriptCore/wasm/WasmName.h:35 > > +typedef Vector<LChar> Name; > > nit: using Name = Vector<LChar>; lol C++14. I always forget about "using" it. > > Source/JavaScriptCore/wasm/WasmNameSectionParser.cpp:61 > > + WASM_PARSER_FAIL_IF(!parseVarUInt32(nameLen), "can't get ", function, "th function's name length for payload ", payloadNumber); > > + WASM_PARSER_FAIL_IF(!consumeUTF8String(nameString, nameLen), "can't get ", function, "th function's name of length ", nameLen, " for payload ", payloadNumber); > > Nit: why are these ths? I would phase this as "unable to read function ", > function, "'s name, which has length of ", nameLen, " and payload", > payloadNumber Consistent with: https://bugs.webkit.org/show_bug.cgi?id=166688 Your phrasing is nicer though. Adopting here.

Attachment 309615 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:247: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/wasm/WasmFormat.h:248: static_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 309615 [details] patch r=me.

Comment on attachment 309615 [details] patch Clearing flags on attachment: 309615 Committed r216597: <http://trac.webkit.org/changeset/216597>

All reviewed patches have been landed. Closing bug.