1263205 - wasm: section header and name tweaks

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

Section header format tweak: https://github.com/WebAssembly/design/pull/639

Section name tweaks: https://github.com/WebAssembly/design/pull/638

Comment 1

[Dan Gohman [:sunfish]]

Attachment: wasm-section-tweaks.patch

Comment 2

[Dan Gohman [:sunfish]]

Attachment: wasm-section-names.patch

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8739506 [details] [diff] [review]
wasm-section-tweaks.patch

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

Ah, much simpler.  Idea, feel free to take it or leave it: we could have a struct Section { size_t startOffset; size_t size }; as the in/out param.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8739507 [details] [diff] [review]
wasm-section-names.patch

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

(and to be clear, we're waiting to land these patches until has been updated with Wasm.experimentalVersion == 0xb)

Comment 5

[Dan Gohman [:sunfish]]

Attachment: wasm-section-names.patch

Rebased on trunk and latest wasm-postorder patch. Carrying forward r+.

Comment 6

[Luke Wagner [:luke]]

Attachment: wasm-section-form.patch

This patch implements https://github.com/WebAssembly/design/commit/54da3d5c19b18d19ccb12af5dda00edb71859759

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8743573 [details] [diff] [review]
wasm-section-form.patch

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

Looks good! About allowing back duplication of signatures: is it because it was never disallowed in the spec?

::: js/src/asmjs/WasmBinaryToText.cpp
@@ +1297,5 @@
> +            if (!c.d.readValType(&type))
> +                return RenderFail(c, "bad expression type");
> +
> +            result = ToExprType(type);
> +        }

Can you add an `if (numRests > 1)` branch above with a RenderFail(NYI), please? It'd be nicer, for the future, than a decoding error somewhere further down.

Comment 8

[Luke Wagner [:luke]]

Attachment: wasm-section-form.patch

Updated

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9942b8651f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec70afdb3fa8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01f4f50db37
https://hg.mozilla.org/integration/mozilla-inbound/rev/019a39ca536e

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/f9942b8651f4
https://hg.mozilla.org/mozilla-central/rev/ec70afdb3fa8
https://hg.mozilla.org/mozilla-central/rev/d01f4f50db37
https://hg.mozilla.org/mozilla-central/rev/019a39ca536e