Bug 171263 – WebAssembly: support name section

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[JF Bastien]

It's a debugging-only custom section which provides names for functions and such:
  https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#name-section

One place it's especially useful is in backtraces, which doesn't even require devtool support.

We've heard from developers that they'd like us to support it.

[JF Bastien]

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

[JF Bastien]

Created attachment 309349 [details]
patch

[Build Bot]

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.

[Build Bot]

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

[JF Bastien]

Created attachment 309478 [details]
patch

Wow I hit build issues all over!

[Build Bot]

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.

[Keith Miller]

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.

[JF Bastien]

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.

[Build Bot]

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.

[Keith Miller]

Comment on attachment 309615 [details]
patch

r=me.

[WebKit Commit Bot]

Comment on attachment 309615 [details]
patch

Clearing flags on attachment: 309615

Committed r216597: <http://trac.webkit.org/changeset/216597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.