Bug 165159 – WebAssembly JS API: add Module.sections

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

As in https://github.com/WebAssembly/design/pull/877

[Radar WebKit Bug Importer]

<rdar://problem/29760326>

[JF Bastien]

Created attachment 297993 [details]
patch

I implemented this one since it's pretty simple, and likely to be used soon by Emscripten.

[Mark Lam]

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?

[JF Bastien]

(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.

[Mark Lam]

Comment on attachment 297993 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 297993 [details]
patch

Clearing flags on attachment: 297993

Committed r210282: <http://trac.webkit.org/changeset/210282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.