1321122 - Baldr: add Module.customSections

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

As added here:
  https://github.com/WebAssembly/design/pull/877

Comment 1

[Luke Wagner [:luke]]

Attachment: add-custom-sections

Overall, pretty easy.  Because this patch ended up wanting the ModuleEnvironment threaded everywhere (so it could be passed to Decoder::startSection()), I ended up moving the remaining dataSegments/funcNames Vectors too.  This simplified everything but involved moving the name-section code from WasmCompile.cpp into WasmValidate.cpp.  I realized after that this should've been a separate patch, but that part is just code motion.

Comment 2

[Luke Wagner [:luke]]

Attachment: out-of-line

This patch moves the start(Custom)Section funcs from WasmValidate.h to .cpp and properly comments and whitespaces them, since they are rather hefty.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8816359 [details] [diff] [review]
add-custom-sections

Review of attachment 8816359 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, thank you!

::: js/src/wasm/WasmJS.cpp
@@ +717,5 @@
> +        JS::DeflateStringToUTF8Buffer(flat, RangedPtr<char>(name.begin(), name.length()));
> +    }
> +
> +    AutoValueVector elems(cx);
> +    if (!elems.reserve(module->metadata().customSections.length()))

This is a pretty pessimistic memory pre-allocation, should we just append() in the loop instead?

@@ +725,5 @@
> +    RootedArrayBufferObject buf(cx);
> +    for (const CustomSection& sec : module->metadata().customSections) {
> +        if (name.length() != sec.name.length)
> +            continue;
> +        if (memcmp(name.begin(), bytecode + sec.name.offset, name.length()))

Is the section name in the bytecode also ensured to be encoded in utf8? The spec doesn't seem to say anything about it...

::: js/src/wasm/WasmValidate.h
@@ +716,5 @@
>  
>  MOZ_MUST_USE bool
>  DecodeLocalEntries(Decoder& d, ModuleKind kind, ValTypeVector* locals);
>  
> +// Decoding a DecodeModuleEnvironment decodes all sections up to the code

nit: Calling DecodeModuleEnvironment decodes ...

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8816361 [details] [diff] [review]
out-of-line

Review of attachment 8816361 [details] [diff] [review]:
-----------------------------------------------------------------

Much better and cleaner, thanks.

Comment 5

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
Thanks!

> This is a pretty pessimistic memory pre-allocation, should we just append()
> in the loop instead?

Yes, good point.

> Is the section name in the bytecode also ensured to be encoded in utf8? The
> spec doesn't seem to say anything about it...

It's kinda stated "for all names" in https://github.com/WebAssembly/design/blob/master/Web.md#names.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ec15a11e13
Baldr: add Module.customSections (r=bbouvier)

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8816359 [details] [diff] [review]
add-custom-sections

Approval Request Comment
[Feature/Bug causing the regression]: new feature in spec, not regression
[User impact if declined]: missing feature in spec in initial release
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: small and self-contained feature

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42ec15a11e13

Comment 9

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 8816359 [details] [diff] [review]
add-custom-sections

wasm update for aurora52

Comment 10

[Carsten Book [:Tomcat]]

needs rebasing for aurora

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8816359 [details] [diff] [review]
add-custom-sections

Oh dear, all the righteous validation refactoring has made this patch a major pain to backport and more risky (since the patch would need significant changes).  Since this is a relatively minor feature, I think it's probably best for this one to ride the trains with 53.