Bug 179343 – WebAssembly: improve stack trace

• Status: RESOLVED
• Resolution: FIXED
• Component: WebAssembly

[JF Bastien]

Our stack traces aren't great because:

  - They don't include a way to figure ouch which module the function is in
  - They treat stubs the same as user functions
  - They don't honor module name from name section, if present (which right now is never the case because toolchains don't generate the name section)

This makes it annoying to debug code.

Further, the proposed design has slightly changed since we originally implemented this:
  https://github.com/WebAssembly/design/blob/master/Web.md#developer-facing-display-conventions

[JF Bastien]

Created attachment 326154 [details]
patch

[JF Bastien]

Created attachment 326157 [details]
patch

[Build Bot]

Attachment 326157 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmNameSection.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 326157 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326157&action=review

r=me

> Source/JavaScriptCore/wasm/WasmModuleInformation.cpp:47
> +    , hash(sha1(source))

should we do this lazily?

> Source/JavaScriptCore/wasm/WasmNameSection.h:40
> +        for (size_t i = 0; i != hash.length(); ++i)

nit: we usually use < instead of != here

> Source/JavaScriptCore/wasm/WasmNameSection.h:43
> +    NameSection() = delete;

Isn't this implicit in you defining the above constructor?

> Source/JavaScriptCore/wasm/WasmNameSection.h:44
> +    NameSection(const NameSection&) = delete;

WTF_MAKE_NONCOPYABLE?

[JF Bastien]

Created attachment 328047 [details]
patch

(In reply to Saam Barati from comment #4)
> Comment on attachment 326157 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326157&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/wasm/WasmModuleInformation.cpp:47
> > +    , hash(sha1(source))
> 
> should we do this lazily?

Nah, I don't think it's worth it. I think we'll eventually want to drop sections which aren't referenced, so hashing won't be possible that late and we'll have to un-lazy.

> > Source/JavaScriptCore/wasm/WasmNameSection.h:40
> > +        for (size_t i = 0; i != hash.length(); ++i)
> 
> nit: we usually use < instead of != here

Done.

> > Source/JavaScriptCore/wasm/WasmNameSection.h:43
> > +    NameSection() = delete;
> 
> Isn't this implicit in you defining the above constructor?

Yeah, I was just being explicit, but I can remove.

> > Source/JavaScriptCore/wasm/WasmNameSection.h:44
> > +    NameSection(const NameSection&) = delete;
> 
> WTF_MAKE_NONCOPYABLE?

I'm not a fan of macros that replace a clear one-line thing :-)

[Saam Barati]

Comment on attachment 326157 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326157&action=review

>>> Source/JavaScriptCore/wasm/WasmNameSection.h:44
>>> +    NameSection(const NameSection&) = delete;
>> 
>> WTF_MAKE_NONCOPYABLE?
> 
> I'm not a fan of macros that replace a clear one-line thing :-)

Except the macro is doing more than what you're doing by hand. You aren't deleting the copy assignment operator. So I think you might not actually getting the guarantee I believe you want.

Also, regarding your motivation for skipping the macro: I disagree. The macro gives a name to this action. It's much more understandable because the name is obvious. As a reader of this code, even if you didn't understand C++ that well, the name of the macro describes what's going on. This syntax won't make sense to somebody who doesn't have at least decent understanding of C++.

[Saam Barati]

Comment on attachment 326157 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326157&action=review

>>>> Source/JavaScriptCore/wasm/WasmNameSection.h:44
>>>> +    NameSection(const NameSection&) = delete;
>>> 
>>> WTF_MAKE_NONCOPYABLE?
>> 
>> I'm not a fan of macros that replace a clear one-line thing :-)
> 
> Except the macro is doing more than what you're doing by hand. You aren't deleting the copy assignment operator. So I think you might not actually getting the guarantee I believe you want.
> 
> Also, regarding your motivation for skipping the macro: I disagree. The macro gives a name to this action. It's much more understandable because the name is obvious. As a reader of this code, even if you didn't understand C++ that well, the name of the macro describes what's going on. This syntax won't make sense to somebody who doesn't have at least decent understanding of C++.

I guess you could just want to prevent copy construction but not copy assignment, but that seems weird.

[WebKit Commit Bot]

Comment on attachment 328047 [details]
patch

Clearing flags on attachment: 328047

Committed r225378: <https://trac.webkit.org/changeset/225378>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35788939>

[Joseph Pecoraro]

Comment on attachment 328047 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328047&action=review

> Source/JavaScriptCore/wasm/WasmIndexOrName.cpp:49
> +        return String("wasm-stub");

You should use `ASCIILiteral("wasm-stub")` in this case to prevent a copy:
https://trac.webkit.org/wiki/EfficientStrings

> Source/JavaScriptCore/wasm/WasmIndexOrName.cpp:53
> +        return makeString(moduleName, ".wasm-function[", String::number(ion.m_indexName.index & ~IndexOrName::indexTag), "]");
> +    return makeString(moduleName, ".wasm-function[", String(ion.m_indexName.name->data(), ion.m_indexName.name->size()), "]");

Nit: You could used a char ']' at the end instead of a "]" (may avoid a strlen if the compiler doesn't optimize it out).

> Source/JavaScriptCore/wasm/WasmIndexOrName.h:46
> +    NameSection* nameSection() const { return m_nameSection ? m_nameSection.get() : nullptr; }

Style: This should just be `return m_nameSection.get()`, the ternary is unnecessary.

> Source/JavaScriptCore/wasm/WasmNameSection.h:47
> +        return std::make_pair(functionIndexSpace < functionNames.size() ? &functionNames[functionIndexSpace] : nullptr, RefPtr<NameSection>(this));

Style: `RefPtr<NameSection>(this)` is typically written as `makeRefPtr(this)`.

[Saam Barati]

This looks to be a 1-2% WasmBench regression on some Macs.

[Saam Barati]

My guess is SHA1 is hurting compile times.

[JF Bastien]

I'd measured this to be neutral, and am surprised it would regress. I can't repro, tried a few things, so here's a speculative disable:
  https://bugs.webkit.org/show_bug.cgi?id=180441