Bug 165159

Summary: WebAssembly JS API: add Module.sections
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ggaren, jfbastien, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 161709, 166688, 166701    
Attachments:
Description Flags
patch none

Description JF Bastien 2016-11-29 14:31:31 PST
As in https://github.com/WebAssembly/design/pull/877
Comment 1 Radar WebKit Bug Importer 2016-12-20 14:30:07 PST
<rdar://problem/29760326>
Comment 2 JF Bastien 2017-01-03 23:15:09 PST
Created attachment 297993 [details]
patch

I implemented this one since it's pretty simple, and likely to be used soon by Emscripten.
Comment 3 Mark Lam 2017-01-04 10:34:15 PST
Comment on attachment 297993 [details]
patch

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

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:603
> +auto ModuleParser::parseCustom(uint32_t sectionLength) -> PartialResult

Isn't this the same as "PartialResult ModuleParser::parseCustom(uint32_t sectionLength)"?  Why use this exotic form instead?  Seems like it only adds extra chars to type and is less clear to read.

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:612
> +    WASM_PARSER_FAIL_IF(!m_result.module->customSections.tryReserveCapacity(customSectionNumber), "can't allocate enough memory for ", customSectionNumber, "th custom section");
> +    WASM_PARSER_FAIL_IF(!parseVarUInt32(nameLen), "can't get ", customSectionNumber, "th custom section's name length");
> +    WASM_PARSER_FAIL_IF(!consumeUTF8String(section.name, nameLen), "nameLen get ", customSectionNumber, "th custom section's name of length ", nameLen);

Can we do better with the "Nth" error string here?  "1th", "2th", and "3th" doesn't read well.  How about rephrasing as " custom section 1's ..." or " custom section [1]'s" instead of "1th custom section's ..."?  I like the [] notation since it implies that [0] is also a valid section.

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:615
> +    WASM_PARSER_FAIL_IF(!section.payload.tryReserveCapacity(payloadBytes), "can't allocate enough memory for ", customSectionNumber, "th custom section's ", payloadBytes, " bytes");

Ditto with usage of "Nth" here.

> Source/JavaScriptCore/wasm/WasmModuleParser.cpp:619
> +        section.payload.uncheckedAppend(byte);

Ditto.

> Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:82
> +        if (section.name == sectionNameString) {
> +            auto buffer = ArrayBuffer::tryCreate(section.payload.data(), section.payload.size());
> +            if (!buffer)
> +                throwException(exec, throwScope, createOutOfMemoryError(exec));
> +
> +            Structure* arrayBufferStructure = InternalFunction::createSubclassStructure(exec, JSValue(), globalObject->arrayBufferStructure(ArrayBufferSharingMode::Default));
> +            RETURN_IF_EXCEPTION(throwScope, { });
> +
> +            result->push(exec, JSArrayBuffer::create(vm, arrayBufferStructure, WTFMove(buffer)));
> +            RETURN_IF_EXCEPTION(throwScope, { });
> +        }

Is it possible to have more than one section matching the same name?  If not, why do we not break out of the loop once we've found a match?
Comment 4 JF Bastien 2017-01-04 10:44:19 PST
(In reply to comment #3)
> Comment on attachment 297993 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297993&action=review
> 
> > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:603
> > +auto ModuleParser::parseCustom(uint32_t sectionLength) -> PartialResult
> 
> Isn't this the same as "PartialResult ModuleParser::parseCustom(uint32_t
> sectionLength)"?  Why use this exotic form instead?  Seems like it only adds
> extra chars to type and is less clear to read.

PartialResult needs to be qualified because it's defined in ModuleParser. The rest of this file follows Yusuke's lead in other parts of the JSC code :-)

I agree it's a bit surprising, but I think that's from the novelty of coming from older C++. IMO it'll be idiomatic C++ in a few years, at least when returning a class-defined type.


> > Source/JavaScriptCore/wasm/WasmModuleParser.cpp:612
> > +    WASM_PARSER_FAIL_IF(!m_result.module->customSections.tryReserveCapacity(customSectionNumber), "can't allocate enough memory for ", customSectionNumber, "th custom section");
> > +    WASM_PARSER_FAIL_IF(!parseVarUInt32(nameLen), "can't get ", customSectionNumber, "th custom section's name length");
> > +    WASM_PARSER_FAIL_IF(!consumeUTF8String(section.name, nameLen), "nameLen get ", customSectionNumber, "th custom section's name of length ", nameLen);
> 
> Can we do better with the "Nth" error string here?  "1th", "2th", and "3th"
> doesn't read well.  How about rephrasing as " custom section 1's ..." or "
> custom section [1]'s" instead of "1th custom section's ..."?  I like the []
> notation since it implies that [0] is also a valid section.

Let me file a bug to do this. It's currently using "th" all over the file, so I'd rather keep it consistent.
https://bugs.webkit.org/show_bug.cgi?id=166688


> > Source/JavaScriptCore/wasm/js/WebAssemblyModulePrototype.cpp:82
> > +        if (section.name == sectionNameString) {
> > +            auto buffer = ArrayBuffer::tryCreate(section.payload.data(), section.payload.size());
> > +            if (!buffer)
> > +                throwException(exec, throwScope, createOutOfMemoryError(exec));
> > +
> > +            Structure* arrayBufferStructure = InternalFunction::createSubclassStructure(exec, JSValue(), globalObject->arrayBufferStructure(ArrayBufferSharingMode::Default));
> > +            RETURN_IF_EXCEPTION(throwScope, { });
> > +
> > +            result->push(exec, JSArrayBuffer::create(vm, arrayBufferStructure, WTFMove(buffer)));
> > +            RETURN_IF_EXCEPTION(throwScope, { });
> > +        }
> 
> Is it possible to have more than one section matching the same name?  If
> not, why do we not break out of the loop once we've found a match?

Yes, sections can have the same name. The API therefore returns an Array of all the matches (which are ArrayBuffers). I filed a bug to update the spec w.r.t. the order of returned matches: it should be the same as the declaration order in the binary.
Comment 5 Mark Lam 2017-01-04 10:50:38 PST
Comment on attachment 297993 [details]
patch

r=me
Comment 6 WebKit Commit Bot 2017-01-04 11:27:59 PST
Comment on attachment 297993 [details]
patch

Clearing flags on attachment: 297993

Committed r210282: <http://trac.webkit.org/changeset/210282>
Comment 7 WebKit Commit Bot 2017-01-04 11:28:05 PST
All reviewed patches have been landed.  Closing bug.