1243252 - Baldr: import section

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

This bug adds import declarations:
  https://github.com/WebAssembly/design/blob/master/Modules.md
which pretty much mirror asm.js FFI functions.

Comment 1

[Luke Wagner [:luke]]

Attachment: tweak-create-new-utf8-chars

Otherwise, can't pass TwoByteChars (in next patch).

Comment 2

[Luke Wagner [:luke]]

Attachment: refactor-exports

This patch just refactors:
 - to fold createJSExport into dynamicallyLink since, from an external interface pov, these are both the same operation
 - WasmAstExport to use TwoByteChars instead of manual pointer and length.

Comment 3

[Luke Wagner [:luke]]

Attachment: add-imports

Again, fairly straightforward.  Of course, they can't be *called* yet with this patch, but there's still enough to write some basic tests.  call/call_import will be in a separate patch.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8712494 [details] [diff] [review]
refactor-exports

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

::: js/src/asmjs/WasmModule.h
@@ +538,3 @@
>  
> +    bool dynamicallyLink(JSContext* cx,
> +                         Handle<WasmModuleObject*> moduleObj, 

nit: trailing space

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

It sounds a bit arbitrary to me to have only two levels of depth in the module object {module: {name: ''}}. I've read a few issues on the WebAssembly repository but couldn't find a reason why we need two levels. In my opinion, there should either be one level (and users can use dots or whatever they want to represent submodules, e.g. "module.math.fround.call") or as many as we want (and then, we could join the levels by dots and use the dotted string as a unique identifier, e.g. {module:{math:{fround:{call: ()=>{}}}}} becomes "module.math.fround.call"). Can you explain me the rationale behind this choice, please?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8712495 [details] [diff] [review]
add-imports

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

Nice! According to https://github.com/WebAssembly/design/issues/522, we'll be able to re-export imports at the moment. I think that this patch doesn't implement this, but it can be done later in a subsequent patch/bug.

::: js/src/asmjs/Wasm.cpp
@@ +672,5 @@
> +            if (!GetProperty(cx, obj, name.func.get(), &v))
> +                return false;
> +        }
> +
> +        if (!IsFunctionObject(v))

Could you use IsFunctionObject in AsmJS.cpp ValidateFFI too, please?

::: js/src/asmjs/WasmModule.h
@@ -181,4 @@
>          return pod.interpExitCodeOffset_;
>      }
>      uint32_t jitExitCodeOffset() const {
> -        MOZ_ASSERT(pod.jitExitCodeOffset_);

Can't we keep these two assertions?

::: js/src/asmjs/WasmText.cpp
@@ +810,5 @@
> +            }
> +            WasmToken valueType;
> +            if (!c.ts.match(WasmToken::ValueType, &valueType, c.error))
> +                return nullptr;
> +            result = ToExprType(valueType.valueType());

These two cases are the exact same as the one in the switch in ParseFunc. Any chance this could be factored out?

::: js/src/jit-test/tests/wasm/binary.js
@@ +79,5 @@
>  }
>  
>  function moduleWithSections(sectionArray) {
>      var bytes = moduleHeaderThen();
>      for (section of sectionArray) {

preexisting nit: let section of sectionArray (or var, but not a global)

@@ +127,5 @@
> +
> +function importSection(imports) {
> +    var body = [];
> +    body.push(...varU32(imports.length));
> +    for (imp of imports) {

nit: let imp of imports

Comment 7

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Nice! According to https://github.com/WebAssembly/design/issues/522, we'll
> be able to re-export imports at the moment.

Only in v8 (not in spec/design) and I think that issue will fix it in v8 to match spec/design.  The current consensus is that imports and functions are in separate index spaces and so if we were to want to re-export imports that would be a separate kind of export statement (which would fit into the binary format I've added here: instead of a "func" export, there'd be some new "import" export).

> Can't we keep these two assertions?

One of my test cases has no function bodies so the exit stubs end up at offset 0 :)

> These two cases are the exact same as the one in the switch in ParseFunc.
> Any chance this could be factored out?

Yes, and it actually cascades and allows even more sharing (between local/param) in ParseFunc!

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> It sounds a bit arbitrary to me to have only two levels of depth in the
> module object {module: {name: ''}}.

The reasoning here is that to integrate with es6 modules, we want exactly two levels, mirroring the
  import foo from "bar";
statements ("foo" is the "function", "bar" is the "module").  However, wasm always wants to be loadable outside es6 modules so we have to decide how to interpret these two strings when all you're given is some import object.  The "" special case mirrors the (export "" index) case where "" means "the default export of the module".

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #8)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> > It sounds a bit arbitrary to me to have only two levels of depth in the
> > module object {module: {name: ''}}.
> 
> The reasoning here is that to integrate with es6 modules, we want exactly
> two levels, mirroring the
>   import foo from "bar";
> statements ("foo" is the "function", "bar" is the "module").  However, wasm
> always wants to be loadable outside es6 modules so we have to decide how to
> interpret these two strings when all you're given is some import object. 
> The "" special case mirrors the (export "" index) case where "" means "the
> default export of the module".

I see, thank you.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6fb892cffb
https://hg.mozilla.org/integration/mozilla-inbound/rev/885145b43150
https://hg.mozilla.org/integration/mozilla-inbound/rev/852f670e81a4

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/ab6fb892cffb
https://hg.mozilla.org/mozilla-central/rev/885145b43150
https://hg.mozilla.org/mozilla-central/rev/852f670e81a4