Baldr: import section

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8712493 [details] [diff] [review]
tweak-create-new-utf8-chars

Otherwise, can't pass TwoByteChars (in next patch).
Attachment #8712493 - Flags: review?(jdemooij)
(Assignee)

Comment 2

3 years ago
Created attachment 8712494 [details] [diff] [review]
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.
Attachment #8712494 - Flags: review?(bbouvier)
(Assignee)

Comment 3

3 years ago
Created attachment 8712495 [details] [diff] [review]
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.
Attachment #8712495 - Flags: review?(bbouvier)
Attachment #8712493 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

3 years ago
Blocks: 1243633
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
Attachment #8712494 - Flags: review?(bbouvier) → review+
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 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
Attachment #8712495 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 7

3 years ago
(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!
(Assignee)

Comment 8

3 years ago
(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".
(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 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab6fb892cffb
https://hg.mozilla.org/mozilla-central/rev/885145b43150
https://hg.mozilla.org/mozilla-central/rev/852f670e81a4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.